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

[Anand Jain <anand.jain@oracle.com>]

On 24/03/2022 02:16, Boris Burkov wrote:
> 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?
> 

It is ok as the device-add step creates a _new_ sprout filesystem which 
is RW-able. btrfs_setup_sprout() resets the seeding flag.

  super_flags = btrfs_super_flags(disk_super) &
  ~BTRFS_SUPER_FLAG_SEEDING;
  btrfs_set_super_flags(disk_super, super_flags);

Thanks, Anand

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