On Thu, Nov 02, 2017 at 12:29:17PM +0200, Nikolay Borisov wrote: > On 31.10.2017 19:44, David Sterba wrote: > > Overview of the main locks protecting various device-related structures. > > > > Signed-off-by: David Sterba <dsterba@xxxxxxxx> > > --- > > fs/btrfs/volumes.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 75aed8ec64bd..098affc58361 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -145,6 +145,72 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info, > > struct btrfs_bio **bbio_ret, > > int mirror_num, int need_raid_map); > > > > +/* > > + * Device locking > > + * ============== > > + * > > + * There are several mutexes that protect manipulation of devices and low-level > > + * structures like chunks but not block groups, extents or files > > This sentence suggests you are describing the various mutexes but it > seems you are also describing data structure below i.e. > global::fs_devs/btrfs_device::name In my understanding, documenting a mutex means defining the context and data that are protected in the context. So I can start with the mutex, or the data itself, but the mutex makes IMO more sense to start. > > + * - uuid_mutex (global lock) > > + * > > + * protects the fs_uuids list that tracks all per-fs fs_devices, resulting > > + * from the SCAN_DEV ioctl registration or from mount either implicitly > > + * (the first device) or requested by the device= mount option > > + * > > + * the mutex can be very coarse and can cover long-running operations > > + * > > + * protects: updates to fs_devices counters like missing devices, rw devices, > > + * seeding, structure cloning, openning/closing devices at mount/umount time > > Perhaps move the uuid_mutex description after btrfs_device::name. That > way mutexes are grouped together and data structures are grouped as well. The grouping is by mutex. > > + * > > + * global::fs_devs - add, remove, updates to the global list > > You seem to be describing a data structure here rather than a mutex As I read it, this means "if this mutex is taken, it protects global::fs_devs in this way". > > + * does not protect: manipulation of the fs_devices::devices list! > > but this sentence is written as if you've just described a mutex. The > end result is that it's not clear what mutex you are referring to here. That's the mutex in the section above "- uuid_mutex (global lock)". > > + * btrfs_device::name - renames (write side), read is RCU > > It's not clear how the write side is protected. same answer, it's under the 'uuid_mutex', so the write side is protected by that, with exception of reads protected by RCU. I wonder what kind of documentation structure do you expect as it seems to me you missed it completely. Which is a valuable feedback to me, but I don't know how to fix it, other than make the sections more visible with underlining or indenting or more whitespace. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
