Re: [PATCH 06/11] btrfs: document device locking

[David Sterba <dsterba@suse.cz>]

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@suse.com>
> > ---
> >  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.