On  6.11.2017 15:51, David Sterba wrote:

On Thu, Nov 02, 2017 at 12:29:17PM +0200, Nikolay Borisov wrote:

quoted

On 31.10.2017 19:44, David Sterba wrote:

quoted

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.

quoted

quoted

+ * - 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.

quoted

quoted

+ *
+ *   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".

quoted

quoted

+ *   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)".

quoted

quoted

+ *   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.

Right, I did a fresh read and indeed the structure now makes sense. I
have completely missed the nesting though. Maybe you can nest more
agressively, now every sentence is aligned to the header, perhaps we
want to have it one level deeper? Or use more than one - at the mutex
level ? I don't really have a clear-cut answer for you.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html