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

[David Sterba <dsterba@suse.cz>]

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.