On 23/09/2021 14:52, Nikolay Borisov wrote:

On 22.09.21 г. 14:41, Anand Jain wrote:

<snip>

quoted

quoted

quoted

@@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct

btrfs_fs_info *fs_info)
       INIT_LIST_HEAD(&seed_devices->devices);
       INIT_LIST_HEAD(&seed_devices->alloc_list);
   -    mutex_lock(&fs_devices->device_list_mutex);
+    *seed_devices_ret = seed_devices;
+
+    return 0;
+}
+
+/*
+ * Splice seed devices into the sprout fs_devices.
+ * Generate a new fsid for the sprouted readwrite btrfs.
+ */
+static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info,
+                struct btrfs_fs_devices *seed_devices)
+{

This function is missing a lockdep_assert_held annotation and it depends
on the device_list_mutex being held.

  You mean
     lockdep_assert_held(&device_list_mutex);
  and not
     lockdep_assert_held(&uuid_mutex);
  right?

I meant that the new function - btrfs_splice_sprout doesn't have any
lockdep annotation, and based on the old code it depends on
device_list_mutex being locked. This is based on the following hunk in
btrfs_init_new_device:

+	mutex_lock(&fs_devices->device_list_mutex);
+	if (seeding_dev) {
+		btrfs_splice_sprout(fs_info, seed_devices);

The way I understand this is btrfs_splice_sprout indeed requires
device_list_mutex being locked, no?

  Yes it requires both uuid_mutex and device_list_mutex. I will add.
  With comments.

Thx.

quoted

quoted

However looking at the resulting code it doesn't look good, because
btrfs_splice_sporut suggests you simply add the seed device to a bunch
of places, yet looking at the function's body it's evident it actually
finishes some parts of the initialization, changes the uuid of the
fs_devices. I'm not convinced it really makes the code better or at the
very least the 'splice_sprout' needs to be changed, because splicing is
a minot part of what this function really does.

The purpose of the split of btrfs_prepare_sprout() was to use a common
device_list_mutex. So I tend to avoid any other changes, but I think I
will do it now based on the comments.

Thanks, Anand

quoted

<snip>