diff mbox series

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

Message ID f9d69d94feeab53df416837d5e8bcc85da4df394.1598792561.git.anand.jain@oracle.com
State New, archived
Headers show
Series btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups | expand

Commit Message

Anand Jain Aug. 30, 2020, 2:41 p.m. UTC
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(-)

Comments

Nikolay Borisov Aug. 31, 2020, 8:37 a.m. UTC | #1
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.

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.

>  }
>  
>  static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
>
Anand Jain Sept. 1, 2020, 8:54 a.m. UTC | #2
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,
>>
diff mbox series

Patch

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);
 }
 
 static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,