Re: [PATCH 05/11] btrfs: btrfs_init_devices_late: use sprout device_list_mutex

Anand Jain
2020-09-01T08:54:33Z

On 31/8/20 4:37 pm, Nikolay Borisov wrote:
>
>
> On 30.08.20 г. 17:41 ч., Anand Jain wrote:
>> In a mounted sprout FS, all threads now are using the
>> sprout::device_list_mutex, and this is the only piece of code using
>> the seed::device_list_mutex. This patch converts to use the sprouts
>> fs_info->fs_devices->device_list_mutex.
>>
>> The same reasoning holds true here, that device delete is holding
>> the sprout::device_list_mutex.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/volumes.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 9921b43ef839..7639a048c6cf 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7184,16 +7184,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
>> mutex_lock(&fs_devices->device_list_mutex);
>> list_for_each_entry(device, &fs_devices->devices, dev_list)
>> device->fs_info = fs_info;
>> - mutex_unlock(&fs_devices->device_list_mutex);
>>
>> list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
>> - mutex_lock(&seed_devs->device_list_mutex);
>> list_for_each_entry(device, &seed_devs->devices, dev_list)
>> device->fs_info = fs_info;
>> - mutex_unlock(&seed_devs->device_list_mutex);
>>
>> seed_devs->fs_info = fs_info;
>> }
>> + mutex_unlock(&fs_devices->device_list_mutex);
>
> Instead of doing the looping here to set fs_info I think it makes more
> sense to move the initialization of fs_info for the seed fs_info/seed
> devices to open_seed_devices.
>

But that will be out of the purpose of this patch. May be in
a different patch.

And seed_devs->fs_info = fs_info co-exists with the
device->fs_info = fs_info as well.

> As a matter of fact at the point where btrfs_init_devices_late is called
> btrfs_read_chunk_tree would have already been called which would have
> resulted in all present seed devices be added to fs_info::seed_list. So
> acquiring the lock here serves no purpose really.

I tried to tests few times before. If any of the user initiated device
operation could race with the mount thread. The ans was no. So the
lock is not really required. But then there was mount and unmount
racing bug, which was very strange to me. With that bug, if unmount
wins the race, user initiated forget thread may free the devices. Now
its fixed. So should be safe. But I am not too sure if that covers all
the threads that could potentially race.

So for now, its ok to consolidate the lock to
sprout::device_list_mutex at least.

Thanks, Anand

>
>> }
>>
>> static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
>>