On 2020/1/3 上午12:17, Josef Bacik wrote:
> On 1/2/20 6:27 AM, Qu Wenruo wrote:
>> There are 4 locations where device size or used space get updated:
>> - Chunk allocation
>> - Chunk removal
>> - Device grow
>> - Device shrink
>>
>> Now also update per-profile available space at those timings.
>>
>> For __btrfs_alloc_chunk() we can't acquire device_list_mutex as in
>> btrfs_finish_chunk_alloc() we could hold device_list_mutex and cause
>> dead lock.
>>
> 
> These are protecting two different things though, holding the
> chunk_mutex doesn't keep things from being removed from the device list.

Further looking into the lock schema, Nikolay's comment is right,
chunk_mutex protects alloc_list (rw devices).

Device won't disappear from alloc_list, as in btrfs_rm_device() we took
chunk_mutex before removing writeable device.

In fact, device_list_mutex is unrelated to alloc_list.

So other calc_per_profile_avaiable() call sites are in fact not safe, I
shouldn't take device_list_mutex, but chunk_mutex which are much easier
to get.

> 
> Looking at patch 1 can't we just do the device list traversal under RCU
> and then not have to worry about the locking at all?  Thanks,

I guess we need SRCU, since in __btrfs_alloc_chunk() context we need to
sleep for search dev extent tree.

And I'm not confident enough for some corner case, maybe some RCU sync
case would cause unexpected performance degrade as the RCU sync can be
more expensive than simple mutex lock.

Thanks,
Qu

> 
> Josef