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

[Nikolay Borisov <nborisov@suse.com>]

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?

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