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

[David Sterba <dsterba@suse.cz>]

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.