Re: [PATCH] btrfs: do not skip re-registration for the mounted device

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

On 2/7/24 08:08, Anand Jain wrote:
> 
> 
> 
> On 2/5/24 18:27, David Sterba wrote:
>> On Mon, Feb 05, 2024 at 07:45:05PM +0800, Anand Jain wrote:
>>> We skip device registration for a single device. However, we do not do
>>> that if the device is already mounted, as it might be coming in again
>>> for scanning a different path.
>>>
>>> This patch is lightly tested; for verification if it fixes.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> I still have some unknowns about the problem. Pls test if this fixes
>>> the problem.

Successfully tested with fstests (-g volume) and temp-fsid test cases.

Can someone verify if this patch fixes the problem? Also, when problem
occurs please provide kernel messages with Btrfs debugging support
option compiled in.

Thanks, Anand


>>>
>>>   fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++----------
>>>   fs/btrfs/volumes.h |  1 -
>>>   2 files changed, 34 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 474ab7ed65ea..192c540a650c 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1299,6 +1299,31 @@ int btrfs_forget_devices(dev_t devt)
>>>       return ret;
>>>   }
>>> +static bool btrfs_skip_registration(struct btrfs_super_block 
>>> *disk_super,
>>> +                    dev_t devt, bool mount_arg_dev)
>>> +{
>>> +    struct btrfs_fs_devices *fs_devices;
>>> +
>>> +    list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>>> +        struct btrfs_device *device;
>>> +
>>> +        mutex_lock(&fs_devices->device_list_mutex);
>>> +        list_for_each_entry(device, &fs_devices->devices, dev_list) {
>>> +            if (device->devt == devt) {
>>> +                mutex_unlock(&fs_devices->device_list_mutex);
>>> +                return false;
>>> +            }
>>> +        }
>>> +        mutex_unlock(&fs_devices->device_list_mutex);
>>
>> This is locking and unlocking again before going to device_list_add, so
>> if something changes regarding the registered device then it's not up to
>> date.
>>
> 

We are in the uuid_mutex, a potentially racing thread will have to
acquire this mutex to delete from the list. So there can't a race.



> Right. A race might happen, but it is not an issue. At worst, there
> will be a stale device in the cache, which gets removed or re-used
> in the next mkfs or mount of the same device.
> 
> However, this is a rough cut that we need to fix. I am reviewing
> your approach as well. I'm fine with any fix.
> 
> 
>>
>>> +    }
>>> +
>>> +    if (!mount_arg_dev && btrfs_super_num_devices(disk_super) == 1 &&
>>> +        !(btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING))
>>> +        return true;
>>
>> The way I implemented it is to check the above conditions as a
>> prerequisite but leave the heavy work for device_list_add that does all
>> the uuid and device list locking and we are quite sure it survives all
>> the races between scanning and mounts.
>>
> 
> Hm. But isn't that the bug we need to fix? That we skipped the device
> scan thread that wanted to replace the device path from /dev/root to
> /dev/sdx?
> 
> And we skipped, because it was not a mount thread
> (%mount_arg_dev=false), and the device is already mounted and the
> devt will match?
> 
> So my fix also checked if devt is a match, then allow it to scan
> (so that the device path can be updated, such as /dev/root to /dev/sdx).
> 
> To confirm the bug, I asked for the debug kernel messages, I don't
> this we got it. Also, the existing kernel log shows no such issue.
> 
> 
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   /*
>>>    * Look for a btrfs signature on a device. This may be called out 
>>> of the mount path
>>>    * and we are not allowed to call set_blocksize during the scan. 
>>> The superblock
>>> @@ -1316,6 +1341,7 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>       struct btrfs_device *device = NULL;
>>>       struct bdev_handle *bdev_handle;
>>>       u64 bytenr, bytenr_orig;
>>> +    dev_t devt = 0;
>>>       int ret;
>>>       lockdep_assert_held(&uuid_mutex);
>>> @@ -1355,18 +1381,16 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>           goto error_bdev_put;
>>>       }
>>> -    if (!mount_arg_dev && btrfs_super_num_devices(disk_super) == 1 &&
>>> -        !(btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING)) {
>>> -        dev_t devt;
>>> +    ret = lookup_bdev(path, &devt);
>>> +    if (ret)
>>> +        btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
>>> +               path, ret);
>>> -        ret = lookup_bdev(path, &devt);
>>
>> Do we actually need this check? It was added with the patch skipping the
>> registration, so it's validating the block device but how can we pass
>> something that is not a valid block device?
>>
> 
> Do you mean to check if the lookup_bdev() is successful? Hm. It should
> be okay not to check, but we do that consistently in other places.
> 
>> Besides there's a call to bdev_open_by_path() that in turn does the
>> lookup_bdev so checking it here is redundant. It's not related to the
>> fix itself but I deleted it in my fix.
>>
> 
> Oh no. We need %devt to be set because:
> 
> It will match if that device is already mounted/scanned.
> It will also free stale entries.
> 
> Thx, Anand
> 
>>> -        if (ret)
>>> -            btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
>>> -                   path, ret);
>>> -        else
>>> +    if (btrfs_skip_registration(disk_super, devt, mount_arg_dev)) {
>>> +        pr_debug("BTRFS: skip registering single non-seed device %s\n",
>>> +              path);
>>> +        if (devt)
>>>               btrfs_free_stale_devices(devt, NULL);
>>> -
>>> -        pr_debug("BTRFS: skip registering single non-seed device 
>>> %s\n", path);
>>>           device = NULL;
>>>           goto free_disk_super;
>>>       }