btrfs: don't commit the super block when unmounting a shutdown filesystem

[PATCH] btrfs: don't commit the super block when unmounting a shutdown filesystem

Posted by Miquel Sabaté Solà 2 months ago

When unmounting a filesystem we will try, among many other things, to
commit the super block. On a filesystem that was shutdown, though, this
will always fail with -EROFS as writes are forbidden on this context;
and an error will be reported.

Don't commit the super block on this situation, which should be fine as
the filesystem is frozen before shutdown and, therefore, it should be at
a consistent state.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/disk-io.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 600287ac8eb7..cd2ce6348d88 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4380,9 +4380,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 		 */
 		btrfs_flush_workqueue(fs_info->delayed_workers);
 
-		ret = btrfs_commit_super(fs_info);
-		if (ret)
-			btrfs_err(fs_info, "commit super ret %d", ret);
+		/*
+		 * If the filesystem is shutdown, then an attempt to commit the
+		 * super block (or any write) will just fail. Since we freeze
+		 * the filesystem before shutting it down, the filesystem should
+		 * be in a consistent state and not committing the super block
+		 * should be fine.
+		 */
+		if (!btrfs_is_shutdown(fs_info)) {
+			ret = btrfs_commit_super(fs_info);
+			if (ret)
+				btrfs_err(fs_info, "commit super ret %d", ret);
+		}
 	}
 
 	kthread_stop(fs_info->transaction_kthread);
-- 
2.53.0

Re: [PATCH] btrfs: don't commit the super block when unmounting a shutdown filesystem

Posted by Filipe Manana 1 month, 4 weeks ago

On Mon, Feb 16, 2026 at 12:23 AM Miquel Sabaté Solà <mssola@mssola.com> wrote:
>
> When unmounting a filesystem we will try, among many other things, to
> commit the super block. On a filesystem that was shutdown, though, this
> will always fail with -EROFS as writes are forbidden on this context;
> and an error will be reported.
>
> Don't commit the super block on this situation, which should be fine as
> the filesystem is frozen before shutdown and, therefore, it should be at
> a consistent state.
>
> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
> ---
>  fs/btrfs/disk-io.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 600287ac8eb7..cd2ce6348d88 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4380,9 +4380,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>                  */
>                 btrfs_flush_workqueue(fs_info->delayed_workers);
>
> -               ret = btrfs_commit_super(fs_info);
> -               if (ret)
> -                       btrfs_err(fs_info, "commit super ret %d", ret);
> +               /*
> +                * If the filesystem is shutdown, then an attempt to commit the
> +                * super block (or any write) will just fail. Since we freeze
> +                * the filesystem before shutting it down, the filesystem should
> +                * be in a consistent state and not committing the super block
> +                * should be fine.

Looks good to me, but I'll rephrase this with:

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index cd2ce6348d88..84829f5e97a8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4383,9 +4383,8 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
                /*
                 * If the filesystem is shutdown, then an attempt to commit the
                 * super block (or any write) will just fail. Since we freeze
-                * the filesystem before shutting it down, the filesystem should
-                * be in a consistent state and not committing the super block
-                * should be fine.
+                * the filesystem before shutting it down, the filesystem is in
+                * a consistent state and we don't need to commit super blocks.
                 */

The way it's phrased gives the reader an idea that we are not sure
about what we are doing, which sounds bad.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Pushed it to the github for-next branch, thanks.

> +                */
> +               if (!btrfs_is_shutdown(fs_info)) {
> +                       ret = btrfs_commit_super(fs_info);
> +                       if (ret)
> +                               btrfs_err(fs_info, "commit super ret %d", ret);
> +               }
>         }
>
>         kthread_stop(fs_info->transaction_kthread);
> --
> 2.53.0
>
>

Re: [PATCH] btrfs: don't commit the super block when unmounting a shutdown filesystem

Posted by Miquel Sabaté Solà 1 month, 4 weeks ago

Filipe Manana @ 2026-02-17 18:11 GMT:

> On Mon, Feb 16, 2026 at 12:23 AM Miquel Sabaté Solà <mssola@mssola.com> wrote:
>>
>> When unmounting a filesystem we will try, among many other things, to
>> commit the super block. On a filesystem that was shutdown, though, this
>> will always fail with -EROFS as writes are forbidden on this context;
>> and an error will be reported.
>>
>> Don't commit the super block on this situation, which should be fine as
>> the filesystem is frozen before shutdown and, therefore, it should be at
>> a consistent state.
>>
>> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>> ---
>>  fs/btrfs/disk-io.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 600287ac8eb7..cd2ce6348d88 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -4380,9 +4380,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>>                  */
>>                 btrfs_flush_workqueue(fs_info->delayed_workers);
>>
>> -               ret = btrfs_commit_super(fs_info);
>> -               if (ret)
>> -                       btrfs_err(fs_info, "commit super ret %d", ret);
>> +               /*
>> +                * If the filesystem is shutdown, then an attempt to commit the
>> +                * super block (or any write) will just fail. Since we freeze
>> +                * the filesystem before shutting it down, the filesystem should
>> +                * be in a consistent state and not committing the super block
>> +                * should be fine.
>
> Looks good to me, but I'll rephrase this with:
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index cd2ce6348d88..84829f5e97a8 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4383,9 +4383,8 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>                 /*
>                  * If the filesystem is shutdown, then an attempt to commit the
>                  * super block (or any write) will just fail. Since we freeze
> -                * the filesystem before shutting it down, the filesystem should
> -                * be in a consistent state and not committing the super block
> -                * should be fine.
> +                * the filesystem before shutting it down, the filesystem is in
> +                * a consistent state and we don't need to commit super blocks.
>                  */
>
> The way it's phrased gives the reader an idea that we are not sure
> about what we are doing, which sounds bad.

Yes, I also like the new wording better, thanks!

>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>
> Pushed it to the github for-next branch, thanks.
>
>> +                */
>> +               if (!btrfs_is_shutdown(fs_info)) {
>> +                       ret = btrfs_commit_super(fs_info);
>> +                       if (ret)
>> +                               btrfs_err(fs_info, "commit super ret %d", ret);
>> +               }
>>         }
>>
>>         kthread_stop(fs_info->transaction_kthread);
>> --
>> 2.53.0
>>
>>