Re: [PATCH RFC V5 2/2] btrfs: consolidate device_list_mutex in prepare_sprout to its parent

On 17/09/2021 23:37, David Sterba wrote:

On Tue, Aug 31, 2021 at 09:21:29AM +0800, Anand Jain wrote:

quoted

btrfs_prepare_sprout() moves seed devices into its own struct fs_devices,
so that its parent function btrfs_init_new_device() can add the new sprout
device to fs_info->fs_devices.

Both btrfs_prepare_sprout() and btrfs_init_new_device() needs
device_list_mutex. But they are holding it sequentially, thus creates a
small window to an opportunity to race. Close this opportunity and hold
device_list_mutex common to both btrfs_init_new_device() and
btrfs_prepare_sprout().

I don't se what exactly would go wrong with the separate device list
locking, but I see at least one potential problem with the new code.

quoted

Signed-off-by: Anand Jain <redacted>
---
RFC because IMO the cleanup of device_list_mutex makes sense even though
there isn't another thread that could race potentially race as of now.

Depends on
  [PATCH v2] btrfs: fix lockdep warning while mounting sprout fs
which removed the device_list_mutex from clone_fs_devices() otherwise
this patch will cause a double mutex error.

v2: fix the missing mutex_unlock in the error return
v3: -
v4: -
v5: - (Except for the change in below SO comments)

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fa9fe47b5b68..53ead67b625c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c

@@ -2369,6 +2369,8 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
  	u64 super_flags;
  
  	lockdep_assert_held(&uuid_mutex);
+	lockdep_assert_held(&fs_devices->device_list_mutex);
+
  	if (!fs_devices->seeding)
  		return -EINVAL;
  

@@ -2400,7 +2402,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
  	INIT_LIST_HEAD(&seed_devices->alloc_list);

quoted

  	mutex_init(&seed_devices->device_list_mutex);

 BTW mutex_init here will go, as the sprout's private
 fs_devices::device_list_mutex is unused. It is a pending cleanup.

A few lines before this one there's alloc_fs_devices and
clone_fs_devices, both allocating memory. This would happen under a big
lock as device_list_mutex also protects superblock write. This is a
pattern to avoid.

 Oh. That's right. Thx. One way is to flag NOFS alloc.

A rough idea would be to split btrfs_prepare_sprout into parts where the
allocations are not done under the lock and the locked part. It could be
partially inlined to btrfs_init_new_device.

 I think you mean something like this...

 btrfs_init_new_device()
 <snip>
   if seeding_dev
      alloc_prepare_sprout
   mutex_lock(&fs_devices->device_list_mutex);
   if seeding_dev
      finish_prepare_sprout
   <snip>
   mutex_unlock(&fs_devices->device_list_mutex);

 I am trying.

Thanks, Anand

quoted

  
-	mutex_lock(&fs_devices->device_list_mutex);
  	list_splice_init_rcu(&fs_devices->devices, &seed_devices->devices,
  			      synchronize_rcu);
  	list_for_each_entry(device, &seed_devices->devices, dev_list)

@@ -2416,7 +2417,6 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
  	generate_random_uuid(fs_devices->fsid);
  	memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE);
  	memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
-	mutex_unlock(&fs_devices->device_list_mutex);
  
  	super_flags = btrfs_super_flags(disk_super) &
  		      ~BTRFS_SUPER_FLAG_SEEDING;

@@ -2591,10 +2591,12 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  	device->dev_stats_valid = 1;
  	set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE);
  
+	mutex_lock(&fs_devices->device_list_mutex);
  	if (seeding_dev) {
  		btrfs_clear_sb_rdonly(sb);
  		ret = btrfs_prepare_sprout(fs_info);
  		if (ret) {
+			mutex_unlock(&fs_devices->device_list_mutex);
  			btrfs_abort_transaction(trans, ret);
  			goto error_trans;
  		}

@@ -2604,7 +2606,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
  
  	device->fs_devices = fs_devices;
  
-	mutex_lock(&fs_devices->device_list_mutex);
  	mutex_lock(&fs_info->chunk_mutex);
  	list_add_rcu(&device->dev_list, &fs_devices->devices);
  	list_add(&device->dev_alloc_list, &fs_devices->alloc_list);

-- 
2.31.1