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

[Boris Burkov <boris@bur.io>]

On Wed, Mar 23, 2022 at 12:52:15AM +0000, Naohiro Aota wrote:
> On Mon, Mar 21, 2022 at 04:56:17PM -0700, 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
> > 
> > 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. 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.
> > 
> > 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);
> > -
> 
> After this line, it updates the metadata e.g, with
> init_first_rw_device() and writes them out with
> btrfs_commit_transaction(). Is that OK to do so with the SB_RDONLY
> flag set?

Good question. As far as I can tell, the functions don't explicitly
check sb_rdonly, though that could be because they expect that to be
checked before you ever try to commit a transaction, for example..

If there is an issue, it's probably somewhat subtle, because the basic
behavior does work.

> 
> >  		/* 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:
> > -- 
> > 2.30.2
> >