Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

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

On 23/09/2021 14:52, Nikolay Borisov wrote:
> 
> 
> On 22.09.21 г. 14:41, Anand Jain wrote:
> 
> <snip>
> 
>>>> @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct
>>>> btrfs_fs_info *fs_info)
>>>>        INIT_LIST_HEAD(&seed_devices->devices);
>>>>        INIT_LIST_HEAD(&seed_devices->alloc_list);
>>>>    -    mutex_lock(&fs_devices->device_list_mutex);
>>>> +    *seed_devices_ret = seed_devices;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Splice seed devices into the sprout fs_devices.
>>>> + * Generate a new fsid for the sprouted readwrite btrfs.
>>>> + */
>>>> +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info,
>>>> +                struct btrfs_fs_devices *seed_devices)
>>>> +{
>>>
>>> This function is missing a lockdep_assert_held annotation and it depends
>>> on the device_list_mutex being held.
>>
>>   You mean
>>      lockdep_assert_held(&device_list_mutex);
>>   and not
>>      lockdep_assert_held(&uuid_mutex);
>>   right?
> 
> I meant that the new function - btrfs_splice_sprout doesn't have any
> lockdep annotation, and based on the old code it depends on
> device_list_mutex being locked. This is based on the following hunk in
> btrfs_init_new_device:
> 
> +	mutex_lock(&fs_devices->device_list_mutex);
> +	if (seeding_dev) {
> +		btrfs_splice_sprout(fs_info, seed_devices);
> 
> The way I understand this is btrfs_splice_sprout indeed requires
> device_list_mutex being locked, no?

  Yes it requires both uuid_mutex and device_list_mutex. I will add.
  With comments.

Thx.


>>> However looking at the resulting code it doesn't look good, because
>>> btrfs_splice_sporut suggests you simply add the seed device to a bunch
>>> of places, yet looking at the function's body it's evident it actually
>>> finishes some parts of the initialization, changes the uuid of the
>>> fs_devices. I'm not convinced it really makes the code better or at the
>>> very least the 'splice_sprout' needs to be changed, because splicing is
>>> a minot part of what this function really does.
>>
>> The purpose of the split of btrfs_prepare_sprout() was to use a common
>> device_list_mutex. So I tend to avoid any other changes, but I think I
>> will do it now based on the comments.
>>
>> Thanks, Anand
>>>
>>>
>>> <snip>
>>>
>>