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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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.

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:
>
>