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

[Boris Burkov <boris@bur.io>]

On Wed, Oct 16, 2024 at 08:42:14AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/10/16 08:30, Qu Wenruo 写道:
> > 
> > 
> > 在 2024/10/16 08:08, Boris Burkov 写道:
> > > 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
> > > 
> > > The first mount mounts the FS readonly, which results in not setting
> > > BTRFS_FS_OPEN, and setting the readonly bit on the sb. 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.
> > > 
> > > 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.
> > > 
> > > A new fstest I have written reproduces the bug and confirms the fix.
> > > 
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > 
> > The fix looks good to me, small and keeps the super block ro flag
> > consistent.
> > 
> > IIRC the old behavior of sprout is, adding device will immediately mark
> > the fs RW, which is a big surprise changing the RO/RW status.
> > 
> > So the extra Rw remount requirement looks very reasonable to me.
> 
> Forgot to mention, although it's a trivial change in the behavior, if we
> are determined to go this path, the man page of the "SEEDING DEVICE"
> chapter also need to be updated.

I just checked the man page and everything there looks correct, at least
to me. It quite clearly states that after the 'device add' the fs is
ready to be remounted read-write (via cycle mount or remount).

Please let me know if there is some specific update you want me to make
that I missed, though!

BTW, this patch does change the rdonly flag behavior, so I will update
the test to look at that, as you suggested.

> 
> Thanks,
> Qu
> > 
> > Reviewed-by: Qu Wenruo <wqu@suse.com>
> > 
> > Thanks,
> > Qu
> > 
> > > ---
> > > Note that this is a resend of an old unmerged fix:
> > > https://lore.kernel.org/linux-
> > > btrfs/16c05d39566858bb8bc1e03bd19947cf2b601b98.1647906815.git.boris@bur.io/T/#u
> > > Some other ideas for fixing it by modifying how we set BTRFS_FS_OPEN
> > > were also explored but not merged around that time:
> > > https://lore.kernel.org/linux-btrfs/
> > > cover.1654216941.git.anand.jain@oracle.com/
> > > 
> > > I don't have a strong preference, but I would really like to see this
> > > trivial bug fixed. For what it is worth, we have been carrying this
> > > patch internally at Meta since I first sent it with no incident.
> > > ---
> > >   fs/btrfs/volumes.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > > index dc9f54849f39..84e861dcb350 100644
> > > --- a/fs/btrfs/volumes.c
> > > +++ b/fs/btrfs/volumes.c
> > > @@ -2841,8 +2841,6 @@ int btrfs_init_new_device(struct btrfs_fs_info
> > > *fs_info, const char *device_path
> > >       set_blocksize(device->bdev_file, 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)) {
> > > @@ -2985,8 +2983,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:
> > 
> > 
>