Re: [PATCH v8] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

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

On 9/11/21 4:02 am, David Sterba wrote:
> On Fri, Oct 15, 2021 at 06:34:51AM +0800, Anand Jain wrote:
>> On 14/10/2021 23:30, Josef Bacik wrote:
>>> On Wed, Oct 13, 2021 at 04:01:37PM +0800, Anand Jain wrote:
>>>>    	seed_devices = alloc_fs_devices(NULL, NULL);
>>>>    	if (IS_ERR(seed_devices))
>>>> -		return PTR_ERR(seed_devices);
>>>> +		return seed_devices;
>>>>    
>>>>    	/*
>>>>    	 * It's necessary to retain a copy of the original seed fs_devices in
>>>> @@ -2411,9 +2404,10 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>>>>    	old_devices = clone_fs_devices(fs_devices);
>>>>    	if (IS_ERR(old_devices)) {
>>>>    		kfree(seed_devices);
>>>> -		return PTR_ERR(old_devices);
>>>> +		return old_devices;
>>>>    	}
>>>>    
>>>> +	lockdep_assert_held(&uuid_mutex);
>>>
>>> There's no reason to move this down here, you can leave it at the top of this
>>> function.  Fix that up and you can add
>>
>>    I thought placing the lockdep_assert_held()s just before the critical
>>    section that wanted the lock makes it easy to reason. In this case, it
>>    is the next line that is
>>
>>         list_add(&old_devices->fs_list, &fs_uuids);
>>
>>    No? I can move it back if it is unnecessary.
> 
> I think the most common placement is near the top of the function so
> it's immediately visible that the lock is assumed to be held. If it's
> too deep in the function, it could be overlooked.

  Yeah agreed. V9 had it moved back to the top of the function as before
  and added a comment so that we don't have to wonder why uuid_mutex is 
essential.