Re: [PATCH 1/4] btrfs: Use rcu when iterating devices in btrfs_init_new_device

Nikolay Borisov
2020-07-23T07:45:32Z

On 22.07.20 г. 17:47 ч., David Sterba wrote:
> On Wed, Jul 22, 2020 at 01:36:15PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 22.07.20 г. 12:17 ч., Johannes Thumshirn wrote:
>>> On 22/07/2020 10:09, Nikolay Borisov wrote:
>>>> When adding a new device there's a mandatory check to see if a device is
>>>> being duplicated to the filesystem it's added to. Since this is a
>>>> read-only operations not necessary to take device_list_mutex and can simply
>>>> make do with an rcu-readlock. No semantics changes.
>>>
>>> Hmm what are the actual locking rules for the device list? For instance looking
>>> at add_missing_dev() in volumes.c addition to the list is unprotected (both from
>>> read_one_chunk() and read_one_dev()). Others (i.e. btrfs_init_new_device()) do
>>> a list_add_rcu(). clone_fs_devices() takes the device_list_mutex and so on.
>>
>> Bummer, the locking rules are supposed to be those documented atop
>> volume.c, namely:
>>
>> * fs_devices::device_list_mutex (per-fs, with RCU)
>>
>> 18 * ------------------------------------------------
>>
>> 17 * protects updates to fs_devices::devices, ie. adding and
>> deleting
>> 16 *
>>
>> 15 * simple list traversal with read-only actions can be done with
>> RCU protection
>> 14 *
>>
>> 13 * may be used to exclude some operations from running
>> concurrently without any
>> 12 * modifications to the list (see write_all_supers)
>>
>>
>>
>> However, your observations seem correct and rather annoying. Let me go
>> and try and figure this out...
>
> For device list it's important to know from which context is the list
> used.
>
> On unmoutned filesystems, the devices can be added by scanning ioctl.
>
> On mounted filesystem, before the mount is finished, the devices cannot
> be concurrently used (single-threaded context) and uuid_mutex is
> temporarily protecting the devices agains scan ioctl.
>
> On finished mount device list must be protected by device_list_mutex
> from the same filesystem changes (ioctls, superblock write). Device
> scanning is a no-op here, but still needs to use the uuid_mutex.
>
> Enter RCU. For performance reasons we don't want to take
> device_list_mutex for read-only operations like show_devname or lookups
> of device id, where it's not critical that the returned information is
> stale.
>
> The mentioned helpers are used by various functions and the context is
> not obvious or documented, but it should be clear once the caller chain
> is known.
>
> I can turn that into comments but please let me know if this is
> sufficient as explanation or if you need more.
>

Yes having those different contexts spelled out is really beneficial.