Re: [PATCH] btrfs: do not clear read-only when adding sprout device

[Boris Burkov <boris@bur.io>]

On Wed, Mar 23, 2022 at 06:44:42PM +0800, Anand Jain wrote:
> On 22/03/2022 07:56, Boris Burkov wrote:
> > If you follow the seed/sprout wiki, it suggests the following workflow:
> > 
> > btrfstune -S 1 seed_dev
> > mount seed_dev mnt
> > btrfs device add sprout_dev
> 
> > mount -o remount,rw mnt
> or
>  umount mnt
>  mount sprout mnt

Agreed. FWIW, I tested that umount/mount is not vulnerable to the bug.
However, a user might be using one of the documented workflows anyway,
and would need it for an fs they add a sprout to while it is in use.

> 
> > The first mount mounts the FS readonly, which results in not setting
> > BTRFS_FS_OPEN, and setting the readonly bit on the sb.
> 
>  Why not set the BTRFS_FS_OPEN?
> 

One reason is that there is other logic that runs when transitioning
from ro->rw in remount besides just setting BTRFS_FS_OPEN. By not
improperly clearing ro, we let that logic do its thing naturally.

> @@ -3904,8 +3904,11 @@ int __cold open_ctree(struct super_block *sb, struct
> btrfs_fs_devices *fs_device
>                 goto fail_qgroup;
>         }
> 
> -       if (sb_rdonly(sb))
> +       if (sb_rdonly(sb)) {
> +               btrfs_set_sb_rdonly(sb);
> +               set_bit(BTRFS_FS_OPEN, &fs_info->flags);
>                 goto clear_oneshot;
> +       }
> 
>         ret = btrfs_start_pre_rw_mount(fs_info);
>         if (ret) {
> 
> > The device add
> > somewhat surprisingly clears the readonly bit on the sb (though the
> > mount is still practically readonly, from the users perspective...).
> > Finally, the remount checks the readonly bit on the sb against the flag
> > and sees no change, so it does not run the code intended to run on
> > ro->rw transitions, leaving BTRFS_FS_OPEN unset.
> 
>  Originally, the step 'btrfs device add sprout_dev' provided seed
>  fs writeable without a remount.

That is very interesting. Do you remember why we changed this behavior
to the current behavior of leaving the seed readonly until the
remount/mount cycle?

> 
>  I think the btrfs_clear_sb_rdonly(sb) in btrfs_init_new_device()
>  was part of it.
> 
>  Removing it doesn't seem to affect the seed-sprout functionality
>  (did I miss anything?) either the -o remount,rw
>  or mount recycle will get it writeable.

My current understanding (probably flawed..):
before this patch: we clear the rdonly bit, but the fs is still readonly
until remount (should figure out exactly how)
after this patch: behavior is the same, except the rdonly bit gets
cleared along with the mounting, not the device add.

> 
> > As a result, when the cleaner_kthread runs, it sees no BTRFS_FS_OPEN and
> > does no work. This results in leaking deleted snapshots until we run out
> > of space.
> 
> 
> > I propose fixing it at the first departure from what feels reasonable:
> > when we clear the readonly bit on the sb during device add. I have a
> > reproducer of the issue here:
> > https://raw.githubusercontent.com/boryas/scripts/main/sh/seed/mkseed.sh
> > and confirm that this patch fixes it, and seems to work OK, otherwise. I
> > will admit that I couldn't dig up the original rationale for clearing
> > the bit here (it dates back to the original seed/sprout commit without
> > explicit explanation) so it's hard to imagine all the ramifications of
> > the change.
> 
>  We got fstests -g seed to test the seed-sprout stuff. Your test case
>  here fits in it. IMO.

Thanks for the tip, I'll add it there, regardless of how we fix it.

> 
> Thanks, Anand
> 
> 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/volumes.c | 4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 3fd17e87815a..75d7eeb26fe6 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2675,8 +2675,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
> >   	set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
> >   	if (seeding_dev) {
> > -		btrfs_clear_sb_rdonly(sb);
> > -
> >   		/* GFP_KERNEL allocation must not be under device_list_mutex */
> >   		seed_devices = btrfs_init_sprout(fs_info);
> >   		if (IS_ERR(seed_devices)) {
> > @@ -2819,8 +2817,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
> >   	mutex_unlock(&fs_info->chunk_mutex);
> >   	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
> >   error_trans:
> > -	if (seeding_dev)
> > -		btrfs_set_sb_rdonly(sb);
> >   	if (trans)
> >   		btrfs_end_transaction(trans);
> >   error_free_zone:
>