On 1/3/20 4:52 AM, Qu Wenruo wrote:
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.
Sigh sorry I was reading it like we were doing fs_devices->devices, which is all
handled via RCU. But you're right, alloc_list is covered by the chunk mutex, so
just make sure the chunk mutex is taken every where and you should be good to
go. Thanks,
Josef
