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.
