Device fixes and cleanups

This fixes potential bio leaks, in several error paths. Unfortunatelly the device structure freeing is opencoded in many places and I missed them when introducing the flush_bio.

Most of the time, devices get freed through call_rcu(..., free_device), so it at least it's not that easy to hit the leak, but it's still possible through the path that frees stale devices.

Fixes e0ae99941423 ("btrfs: preallocate device flush bio")

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/btrfs/volumes.c +10 -0

index ea8b20839ac0..08fb4b5609b7 100644
@@ -189,6 +189,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 				    struct btrfs_device, dev_list);
 		list_del(&device->dev_list);
 		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
 		kfree(device);
 	}
 	kfree(fs_devices);

@@ -578,6 +579,7 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 				fs_devs->num_devices--;
 				list_del(&dev->dev_list);
 				rcu_string_free(dev->name);
+				bio_put(dev->flush_bio);
 				kfree(dev);
 			}
 			break;

@@ -630,6 +632,7 @@ static noinline int device_list_add(const char *path,
 
 		name = rcu_string_strdup(path, GFP_NOFS);
 		if (!name) {
+			bio_put(device->flush_bio);
 			kfree(device);
 			return -ENOMEM;
 		}

@@ -742,6 +745,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 			name = rcu_string_strdup(orig_dev->name->str,
 					GFP_KERNEL);
 			if (!name) {
+				bio_put(device->flush_bio);
 				kfree(device);
 				goto error;
 			}

@@ -807,6 +811,7 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step)
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
 		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
 		kfree(device);
 	}
 

@@ -2337,6 +2342,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	name = rcu_string_strdup(device_path, GFP_KERNEL);
 	if (!name) {
+		bio_put(device->flush_bio);
 		kfree(device);
 		ret = -ENOMEM;
 		goto error;

@@ -2346,6 +2352,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
 		kfree(device);
 		ret = PTR_ERR(trans);
 		goto error;

@@ -2489,6 +2496,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	if (trans)
 		btrfs_end_transaction(trans);
 	rcu_string_free(device->name);
+	bio_put(device->flush_bio);
 	kfree(device);
 error:
 	blkdev_put(bdev, FMODE_EXCL);

@@ -2555,6 +2563,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
 	name = rcu_string_strdup(device_path, GFP_KERNEL);
 	if (!name) {
+		bio_put(device->flush_bio);
 		kfree(device);
 		ret = -ENOMEM;
 		goto error;

@@ -6271,6 +6280,7 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 
 		ret = find_next_devid(fs_info, &tmp);
 		if (ret) {
+			bio_put(dev->flush_bio);
 			kfree(dev);
 			return ERR_PTR(ret);
 		}

Quoted 8 lines · David Sterba

This fixes potential bio leaks, in several error paths. Unfortunatelly the device structure freeing is opencoded in many places and I missed them when introducing the flush_bio.

Most of the time, devices get freed through call_rcu(..., free_device), so it at least it's not that easy to hit the leak, but it's still possible through the path that frees stale devices.

Quoted 2 lines

Fixes e0ae99941423 ("btrfs: preallocate device flush bio")

Signed-off-by David Sterba <[email protected]>

Verified that every kfree(device) has a matching bio_put via:

grep -ir -B2 "kfree(dev.*)" fs/btrfs/volumes.c

This fixes potential bio leaks, in several error paths. Unfortunatelly the device structure freeing is opencoded in many places and I missed them when introducing the flush_bio.

Most of the time, devices get freed through call_rcu(..., free_device), so it at least it's not that easy to hit the leak, but it's still possible through the path that frees stale devices.

Fixes e0ae99941423 ("btrfs: preallocate device flush bio")

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/volumes.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

fs/btrfs/volumes.c +10 -0

index ea8b20839ac0..08fb4b5609b7 100644
@@ -189,6 +189,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
  				    struct btrfs_device, dev_list);
  		list_del(&device->dev_list);
  		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
  		kfree(device);
  	}
  	kfree(fs_devices);

@@ -578,6 +579,7 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
  				fs_devs->num_devices--;
  				list_del(&dev->dev_list);
  				rcu_string_free(dev->name);
+				bio_put(dev->flush_bio);
  				kfree(dev);
  			}
  			break;

@@ -630,6 +632,7 @@ static noinline int device_list_add(const char *path,
 
  		name = rcu_string_strdup(path, GFP_NOFS);
  		if (!name) {
+			bio_put(device->flush_bio);
  			kfree(device);
  			return -ENOMEM;
  		}

@@ -742,6 +745,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
  			name = rcu_string_strdup(orig_dev->name->str,
  					GFP_KERNEL);
  			if (!name) {
+				bio_put(device->flush_bio);
  				kfree(device);
  				goto error;
  			}

@@ -807,6 +811,7 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step)
  		list_del_init(&device->dev_list);
  		fs_devices->num_devices--;
  		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
  		kfree(device);
  	}
 

@@ -2337,6 +2342,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
  	name = rcu_string_strdup(device_path, GFP_KERNEL);
  	if (!name) {
+		bio_put(device->flush_bio);
  		kfree(device);
  		ret = -ENOMEM;
  		goto error;

@@ -2346,6 +2352,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  	trans = btrfs_start_transaction(root, 0);
  	if (IS_ERR(trans)) {
  		rcu_string_free(device->name);
+		bio_put(device->flush_bio);
  		kfree(device);
  		ret = PTR_ERR(trans);
  		goto error;

@@ -2489,6 +2496,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  	if (trans)
  		btrfs_end_transaction(trans);
  	rcu_string_free(device->name);
+	bio_put(device->flush_bio);
  	kfree(device);
  error:
  	blkdev_put(bdev, FMODE_EXCL);

@@ -2555,6 +2563,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
  	name = rcu_string_strdup(device_path, GFP_KERNEL);
  	if (!name) {
+		bio_put(device->flush_bio);
  		kfree(device);
  		ret = -ENOMEM;
  		goto error;

@@ -6271,6 +6280,7 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 
  		ret = find_next_devid(fs_info, &tmp);
  		if (ret) {
+			bio_put(dev->flush_bio);
  			kfree(dev);
  			return ERR_PTR(ret);
  		}

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/volumes.c | 39 +++++++++++----------------------------
  1 file changed, 11 insertions(+), 28 deletions(-)

fs/btrfs/volumes.c +11 -28

index 35c546aade83..f206e83d4f52 100644
@@ -195,9 +195,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
  		device = list_entry(fs_devices->devices.next,
  				    struct btrfs_device, dev_list);
  		list_del(&device->dev_list);
-		rcu_string_free(device->name);
-		bio_put(device->flush_bio);
-		kfree(device);
+		free_device(device);
  	}
  	kfree(fs_devices);
  }

@@ -590,9 +588,7 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
  			} else {
  				fs_devs->num_devices--;
  				list_del(&dev->dev_list);
-				rcu_string_free(dev->name);
-				bio_put(dev->flush_bio);
-				kfree(dev);
+				free_device(dev);
  			}
  			break;
  		}

@@ -644,8 +640,7 @@ static noinline int device_list_add(const char *path,
 
  		name = rcu_string_strdup(path, GFP_NOFS);
  		if (!name) {
-			bio_put(device->flush_bio);
-			kfree(device);
+			free_device(device);
  			return -ENOMEM;
  		}
  		rcu_assign_pointer(device->name, name);

@@ -757,8 +752,7 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
  			name = rcu_string_strdup(orig_dev->name->str,
  					GFP_KERNEL);
  			if (!name) {
-				bio_put(device->flush_bio);
-				kfree(device);
+				free_device(device);
  				goto error;
  			}
  			rcu_assign_pointer(device->name, name);

@@ -822,9 +816,7 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step)
  		}
  		list_del_init(&device->dev_list);
  		fs_devices->num_devices--;
-		rcu_string_free(device->name);
-		bio_put(device->flush_bio);
-		kfree(device);
+		free_device(device);
  	}
 
  	if (fs_devices->seed) {

@@ -842,9 +834,7 @@ static void free_device_rcu(struct rcu_head *head)
  	struct btrfs_device *device;
 
  	device = container_of(head, struct btrfs_device, rcu);
-	rcu_string_free(device->name);
-	bio_put(device->flush_bio);
-	kfree(device);
+	free_device(device);
  }
 
  static void btrfs_close_bdev(struct btrfs_device *device)

@@ -2354,8 +2344,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
  	name = rcu_string_strdup(device_path, GFP_KERNEL);
  	if (!name) {
-		bio_put(device->flush_bio);
-		kfree(device);
+		free_device(device);
  		ret = -ENOMEM;
  		goto error;
  	}

@@ -2363,9 +2352,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
  	trans = btrfs_start_transaction(root, 0);
  	if (IS_ERR(trans)) {
-		rcu_string_free(device->name);
-		bio_put(device->flush_bio);
-		kfree(device);
+		free_device(device);
  		ret = PTR_ERR(trans);
  		goto error;
  	}

@@ -2507,9 +2494,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  		sb->s_flags |= MS_RDONLY;
  	if (trans)
  		btrfs_end_transaction(trans);
-	rcu_string_free(device->name);
-	bio_put(device->flush_bio);
-	kfree(device);
+	free_device(device);
  error:
  	blkdev_put(bdev, FMODE_EXCL);
  	if (seeding_dev && !unlocked) {

@@ -2575,8 +2560,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
  	name = rcu_string_strdup(device_path, GFP_KERNEL);
  	if (!name) {
-		bio_put(device->flush_bio);
-		kfree(device);
+		free_device(device);
  		ret = -ENOMEM;
  		goto error;
  	}

@@ -6292,8 +6276,7 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 
  		ret = find_next_devid(fs_info, &tmp);
  		if (ret) {
-			bio_put(dev->flush_bio);
-			kfree(dev);
+			free_device(dev);
  			return ERR_PTR(ret);
  		}
  	}

Signed-off-by David Sterba <[email protected]>

---
  fs/btrfs/volumes.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

fs/btrfs/volumes.c +3 -4

index f206e83d4f52..75aed8ec64bd 100644
@@ -2344,17 +2344,15 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
  	name = rcu_string_strdup(device_path, GFP_KERNEL);
  	if (!name) {
-		free_device(device);
  		ret = -ENOMEM;
-		goto error;
+		goto error_free_device;
  	}
  	rcu_assign_pointer(device->name, name);
 
  	trans = btrfs_start_transaction(root, 0);
  	if (IS_ERR(trans)) {
-		free_device(device);
  		ret = PTR_ERR(trans);
-		goto error;
+		goto error_free_device;
  	}
 
  	q = bdev_get_queue(bdev);

@@ -2494,6 +2492,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  		sb->s_flags |= MS_RDONLY;
  	if (trans)
  		btrfs_end_transaction(trans);
+error_free_device:
  	free_device(device);
  error:
  	blkdev_put(bdev, FMODE_EXCL);

Overview of the main locks protecting various device-related structures.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/volumes.c +0 -0

index 75aed8ec64bd..098affc58361 100644

fs/btrfs/volumes.c +43 -0

+ *
+ * - fs_devices::device_list_mutex (per-fs, with RCU)
+ *
+ *   protects updates to fs_devices::devices, ie. adding and deleting
+ *
+ *   simple list traversal with read-only actions can be done with RCU
+ *   protection
+ *
+ *   may be used to exclude some operations from running concurrently without
+ *   any modifications to the list (see write_all_supers)
+ *
+ * - volume_mutex
+ *
+ *   coarse lock owned by a mounted filesystem; used to exclude some operations
+ *   that cannot run in parallel and affect the higher-level properties of the
+ *   filesystem like: device add/deleting/resize/replace, or balance
+ *
+ * - balance_mutex
+ *
+ *   protects balance structures (status, state) and context accessed from
+ *   several places (internally, ioctl)
+ *
+ * - chunk_mutex
+ *
+ *   protects chunks, adding or removing during allocation, trim or when
+ *   a new device is added/removed
+ *
+ * - cleaner_mutex
+ *
+ *   a big lock that is held by the cleaner thread and prevents running
+ *   subvolume cleaning together with relocation or delayed iputs
+ *
+ *
+ * Lock nesting
+ * ------------
+ *
+ * uuid_mutex
+ *   volume_mutex
+ *     device_list_mutex
+ *       chunk_mutex
+ *     balance_mutex
+ *
+ */
 DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);
 struct list_head *btrfs_get_fs_uuids(void)

Quoted 15 lines · David Sterba

Overview of the main locks protecting various device-related structures.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/volumes.c +0 -0

index 75aed8ec64bd..098affc58361 100644
@@ -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);
 

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 153

Quoted 6 lines

fs/btrfs/volumes.c +6 -0

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

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 164

Quoted 10 lines

fs/btrfs/volumes.c +10 -0

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

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 165

Quoted 2 lines

fs/btrfs/volumes.c +2 -0

+ *
+ *   global::fs_devs - add, remove, updates to the global list

You seem to be describing a data structure here rather than a mutex

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 168

Quoted 1 line

fs/btrfs/volumes.c +1 -0

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

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 170

Quoted 1 line

fs/btrfs/volumes.c +1 -0

+ *   btrfs_device::name - renames (write side), read is RCU

It's not clear how the write side is protected.

Quoted 16 lines · Nikolay Borisov

Quoted 15 lines · David Sterba

Overview of the main locks protecting various device-related structures.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/volumes.c +0 -0

index 75aed8ec64bd..098affc58361 100644
@@ -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);
 

Quoted 10 lines

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 153

Quoted 6 lines

fs/btrfs/volumes.c +6 -0

+/*
+ * 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 11 lines

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 164

Quoted 10 lines

fs/btrfs/volumes.c +10 -0

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

Quoted 2 lines

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 4 lines

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 165

Quoted 2 lines

fs/btrfs/volumes.c +2 -0

+ *
+ *   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 4 lines

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 168

Quoted 1 line

fs/btrfs/volumes.c +1 -0

+ *   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 3 lines

fs/btrfs/volumes.c static int __btrfs_map_block(struct btrfs_fs_info *fs_info, · line 170

Quoted 1 line

fs/btrfs/volumes.c +1 -0

+ *   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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

+ * - fs_devices::device_list_mutex (per-fs, with RCU)
+ *
+ * protects updates to fs_devices::devices, ie. adding and deleting
+ *
+ * simple list traversal with read-only actions can be done with RCU
+ * protection
+ *
+ * may be used to exclude some operations from running concurrently without
+ * any modifications to the list (see write_all_supers)

+ * - volume_mutex
+ *

+ *   coarse lock owned by a mounted filesystem; used to exclude some operations
+ *   that cannot run in parallel and affect the higher-level properties of the
+ *   filesystem like: device add/deleting/resize/replace, or balance

+ * - chunk_mutex
+ *
+ * protects chunks, adding or removing during allocation, trim or when
+ * a new device is added/removed

+ * Lock nesting
+ * ------------
+ *
+ * uuid_mutex
+ * volume_mutex
+ * device_list_mutex
+ * chunk_mutex
+ * balance_mutex

Thanks for writing this.

Quoted 18 lines

+ * - fs_devices::device_list_mutex (per-fs, with RCU)
+ *
+ * protects updates to fs_devices::devices, ie. adding and deleting
+ *
+ * simple list traversal with read-only actions can be done with RCU
+ * protection
+ *
+ * may be used to exclude some operations from running concurrently
without
+ * any modifications to the list (see write_all_supers)

+ * - volume_mutex
+ *
+ * coarse lock owned by a mounted filesystem; used to exclude some
operations
+ * that cannot run in parallel and affect the higher-level
properties of the
+ * filesystem like: device add/deleting/resize/replace, or balance

Quoted 4 lines

+ * - chunk_mutex
+ *
+ * protects chunks, adding or removing during allocation, trim or when
+ * a new device is added/removed

::

Quoted 5 lines

+ * Lock nesting
+ * ------------
+ *
+ * uuid_mutex
+ * volume_mutex

Quoted 3 lines

+ * device_list_mutex
+ * chunk_mutex
+ * balance_mutex

If we have a list of operations that would consume these locks then we can map it accordingly for better clarity.

To me it looks like we have too many locks.
- we don't have to differentiate the mounted and unmounted context for device locks.
- Two lock would be sufficient, one for the device list (add/rm,replace,..) and another for device property changes (resize, trim,..).

Thanks, Anand

Signature · 3 lines

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

Quoted 5 lines

Quoted 5 lines

+ * Lock nesting
+ * ------------
+ *
+ * uuid_mutex
+ * volume_mutex

I think it should be nested like this, but as of now its not. Ref [1]
[1]
btrfs: move volume_mutex into the btrfs_rm_device()

Thanks for writing this.

Quoted 12 lines

+ * - fs_devices::device_list_mutex (per-fs, with RCU)
+ *
+ * protects updates to fs_devices::devices, ie. adding and deleting
+ *
+ * simple list traversal with read-only actions can be done with RCU
+ * protection
+ *
+ * may be used to exclude some operations from running concurrently without
+ * any modifications to the list (see write_all_supers)

+ * - volume_mutex
+ *

Quoted 8 lines

+ *   coarse lock owned by a mounted filesystem; used to exclude some operations
+ *   that cannot run in parallel and affect the higher-level properties of the
+ *   filesystem like: device add/deleting/resize/replace, or balance

+ * - chunk_mutex
+ *
+ * protects chunks, adding or removing during allocation, trim or when
+ * a new device is added/removed

::

Quoted 8 lines

+ * Lock nesting
+ * ------------
+ *
+ * uuid_mutex
+ * volume_mutex
+ * device_list_mutex
+ * chunk_mutex
+ * balance_mutex

If we have a list of operations that would consume these locks then we can map it accordingly for better clarity.

To me it looks like we have too many locks.

- we don't have to differentiate the mounted and unmounted context for device locks.

- Two lock would be sufficient, one for the device list (add/rm,replace,..) and another for device property changes (resize, trim,..).

We take the fs_devices::device_list_mutex mutex in write_all_supers which will prevent any add/del changes to the device list. Therefore we don't need to use the RCU variant list_for_each_entry_rcu in any of the called functions.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/disk-io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

fs/btrfs/disk-io.c +0 -0

index efce9a2fa9be..042cf46e4cd0 100644

fs/btrfs/disk-io.c +4 -4

 	/* send down all the barriers */
 	head = &info->fs_devices->devices;
-	list_for_each_entry_rcu(dev, head, dev_list) {
+	list_for_each_entry(dev, head, dev_list) {
 		if (dev->missing)
 			continue;
 		if (!dev->bdev)

@@ -3411,7 +3412,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 	}
 
 	/* wait for all the barriers */
-	list_for_each_entry_rcu(dev, head, dev_list) {
+	list_for_each_entry(dev, head, dev_list) {
 		if (dev->missing)
 			continue;
 		if (!dev->bdev) {

@@ -3510,7 +3511,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 		}
 	}
 
-	list_for_each_entry_rcu(dev, head, dev_list) {
+	list_for_each_entry(dev, head, dev_list) {
 		if (!dev->bdev) {
 			total_errors++;
 			continue;

@@ -3551,7 +3552,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	}
 
 	total_errors = 0;
-	list_for_each_entry_rcu(dev, head, dev_list) {
+	list_for_each_entry(dev, head, dev_list) {
 		if (!dev->bdev)
 			continue;
 		if (!dev->in_fs_metadata || !dev->writeable)

Quoted 18 lines · David Sterba

We take the fs_devices::device_list_mutex mutex in write_all_supers which will prevent any add/del changes to the device list. Therefore we don't need to use the RCU variant list_for_each_entry_rcu in any of the called functions.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/disk-io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

fs/btrfs/disk-io.c +0 -0

index efce9a2fa9be..042cf46e4cd0 100644
@@ -3396,9 +3396,10 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 	int errors_wait = 0;
 	blk_status_t ret;
 

fs/btrfs/disk-io.c static int barrier_all_devices(struct btrfs_fs_info *info) · line 3399

Quoted 1 line

fs/btrfs/disk-io.c +1 -0

+	WARN_ON(!mutex_is_locked(&info->fs_devices->device_list_mutex));

Don't we want lockdep_assert_held ? I'm assuming enough testing with lockdep on is performed so it will be caught in time if this invariant is broken.