[PATCH v3 0/16] btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups

[PATCH v3 0/16] btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups

[Anand Jain]

v3:
Makes bug fixing patches suitable for the backports. They are patch 1-2.
patch 1 fix the put of kobject null issue, fixed by checking the
        state_initalized.
patch 2 fixes the replacement of seed device in a sprout filesystem.

The rest of the patches remains the same, except for a conflict fix in patch 4.

The patch set has passed xfstests -g volume and seed
The new patch (seed delete testing) btrfs/219 has been modified to suit
the older kernels. Which is also attached to this thread.

-------- v2 cover-letter -------
v2:
patch 1-3 are cleanups and is a split from the v1-patch-1/11.
patch 4-5 are from v1 with conflict fix.
patch 6 is the core bug fix, which fixes the null sysfs obj being freed
        which was in v1 also but together with other required
	preparatory.
patch 7 fixes a new bug found that we leaked sysfs object in the fail
        exit path.
patch 8,9,10,11,12,13,14,15 were in v1 as well. 8,9 consolidates
	device_list_mutex to use the main fs_info->fs_devices.
	10-14 are cleanups. 15 fixes bug exposed after replacing of seed
	and trying to mount without the seed.

v2 has been tested with xfstests -g volume

------ v1 cover-letter ------------
In this patch set:
Rebased on latest misc-next which contains the seed_list from Nicolas.
This patchset has been sent before as two sets, here it is been merged.

Patches 1/11 fix the null kobject being put, which is observed with kernel
compiled with memory poisoning.
Patches 2/11 and 3/11 are cleanups to maintain better flow in the
functions btrfs_sysfs_add_devices_dir() and btrfs_sysfs_remove_devices_dir().
Patches 4/11 and 5/11 drops the last two users of seed device_list_mutex,
so now they hold main filesystem device_list_mutex. 
Patches [6-10]/11 are cleanups.
Patches 11/11 block the replacement of seed device in a sprouted FS, we already
block replacing a seed device on a non-sprouted device.

This set has passed through fstests/volume.

---- original cover-letter ---------
In a sprouted file system, the seed's fs_devices are cloned and 
listed under the sprout's fs_devices. The fs_info::fs_devices
points to the sprout::fs_devices and various critical operations
like device-delete holds the top-level main device_list_mutex
which is sprout::fs_devices::device_list_mutex and _not_ the seed level
mutex such as sprout::fs_devices::seed::fs_devices::device_list_mutex.

Also all related readers (except for two threads- reada and init_devices_late)
hold the sprout::fs_devices::device_list_mutex too. And those two threads
which are missing to hold the correct lock are being fixed here.

I take the approach to fix the read end instead of fixing the writer end
which are not holding the seed level mutex because to keep things
simple and there isn't much benefit burning extra CPU cycles in going
through the lock/unlock process as we traverse through the
fs_devices::seed fs_devices (for example as in reada and init_devices_late
threads).

The first two patches (1/7, 2/7) fixes the threads using the
seed::device_list_mutex.

And rest of the patches ([3-7]/7) are cleanups and these patches
are independent by themself.

These patchset has been tested with full xfstests btrfs test cases.

*** BLURB HERE ***

Anand Jain (16):
  btrfs: fix put of uninitialized kobject after seed device delete
  btrfs: fix replace of seed device
  btrfs: add btrfs_sysfs_add_device helper
  btrfs: add btrfs_sysfs_remove_device helper
  btrfs: btrfs_sysfs_remove_devices_dir drop return value
  btrfs: refactor btrfs_sysfs_add_devices_dir
  btrfs: refactor btrfs_sysfs_remove_devices_dir
  btrfs: initialize sysfs devid and device link for seed device
  btrfs: handle fail path for btrfs_sysfs_add_fs_devices
  btrfs: reada: use sprout device_list_mutex
  btrfs: btrfs_init_devices_late: use sprout device_list_mutex
  btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev
  btrfs: cleanup btrfs_remove_chunk
  btrfs: cleanup btrfs_assign_next_active_device()
  btrfs: cleanup unnecessary goto in open_seed_device
  btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file
    global declare

 fs/btrfs/dev-replace.c |  66 ++++++++---------
 fs/btrfs/reada.c       |   5 +-
 fs/btrfs/sysfs.c       | 159 +++++++++++++++++++++++++----------------
 fs/btrfs/sysfs.h       |   6 +-
 fs/btrfs/volumes.c     |  40 +++++------
 5 files changed, 147 insertions(+), 129 deletions(-)

-- 
2.25.1

[PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[Anand Jain]

The following test case leads to null kobject-being-freed error.

 mount seed /mnt
 add sprout to /mnt
 umount /mnt
 mount sprout to /mnt
 delete seed

 kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called.
 WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350
 RIP: 0010:kobject_put+0x80/0x350
 ::
 Call Trace:
 btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs]
 btrfs_rm_device.cold+0xa8/0x298 [btrfs]
 btrfs_ioctl+0x206c/0x22a0 [btrfs]
 ksys_ioctl+0xe2/0x140
 __x64_sys_ioctl+0x1e/0x29
 do_syscall_64+0x96/0x150
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
 RIP: 0033:0x7f4047c6288b
 ::

This is because, at the end of the seed device-delete, we try to remove
the seed's devid sysfs entry. But for the seed devices under the sprout
fs, we don't initialize the devid kobject yet. So add a kobject state
check, which takes care of the Warning.

Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/sysfs.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 190e59152be5..438a367371c4 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
 					  disk_kobj->name);
 		}
 
-		kobject_del(&one_device->devid_kobj);
-		kobject_put(&one_device->devid_kobj);
+		if (one_device->devid_kobj.state_initialized) {
+			kobject_del(&one_device->devid_kobj);
+			kobject_put(&one_device->devid_kobj);
 
-		wait_for_completion(&one_device->kobj_unregister);
+			wait_for_completion(&one_device->kobj_unregister);
+		}
 
 		return 0;
 	}
@@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
 			sysfs_remove_link(fs_devices->devices_kobj,
 					  disk_kobj->name);
 		}
-		kobject_del(&one_device->devid_kobj);
-		kobject_put(&one_device->devid_kobj);
+		if (one_device->devid_kobj.state_initialized) {
+			kobject_del(&one_device->devid_kobj);
+			kobject_put(&one_device->devid_kobj);
 
-		wait_for_completion(&one_device->kobj_unregister);
+			wait_for_completion(&one_device->kobj_unregister);
+		}
 	}
 
 	return 0;
-- 
2.25.1

[PATCH 02/16] btrfs: fix replace of seed device

[Anand Jain]

If you replace a seed device in a sprouted fs, it appears to have
successfully replaced the seed device, but if you look closely, it didn't.

Here is an example.

mkfs.btrfs -fq /dev/sda
btrfstune -S1 /dev/sda
mount /dev/sda /btrfs
btrfs dev add /dev/sdb /btrfs
umount /btrfs; btrfs dev scan --forget
mount -o device=/dev/sda /dev/sdb /btrfs
btrfs rep start -f /dev/sda /dev/sdc /btrfs; echo $?
0

  BTRFS info (device sdb): dev_replace from /dev/sda (devid 1) to /dev/sdc started
  BTRFS info (device sdb): dev_replace from /dev/sda (devid 1) to /dev/sdc finished

btrfs fi show
Label: none  uuid: ab2c88b7-be81-4a7e-9849-c3666e7f9f4f
	Total devices 2 FS bytes used 256.00KiB
	devid    1 size 3.00GiB used 520.00MiB path /dev/sdc
	devid    2 size 3.00GiB used 896.00MiB path /dev/sdb

Label: none  uuid: 10bd3202-0415-43af-96a8-d5409f310a7e
	Total devices 1 FS bytes used 128.00KiB
	devid    1 size 3.00GiB used 536.00MiB path /dev/sda

So as per the replace start command and kernel log replace was successful.

Now let's try to clean mount.

umount /btrfs;  btrfs dev scan --forget

mount -o device=/dev/sdc /dev/sdb /btrfs
mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/sdb, missing codepage or helper program, or other error.

[  636.157517] BTRFS error (device sdc): failed to read chunk tree: -2
[  636.180177] BTRFS error (device sdc): open_ctree failed

That's because per dev items it is still looking for the original seed
device.

btrfs in dump-tree -d /dev/sdb

	item 0 key (DEV_ITEMS DEV_ITEM 1) itemoff 16185 itemsize 98
		devid 1 total_bytes 3221225472 bytes_used 545259520
		io_align 4096 io_width 4096 sector_size 4096 type 0
		generation 6 start_offset 0 dev_group 0
		seek_speed 0 bandwidth 0
		uuid 59368f50-9af2-4b17-91da-8a783cc418d4  <--- seed uuid
		fsid 10bd3202-0415-43af-96a8-d5409f310a7e  <--- seed fsid
	item 1 key (DEV_ITEMS DEV_ITEM 2) itemoff 16087 itemsize 98
		devid 2 total_bytes 3221225472 bytes_used 939524096
		io_align 4096 io_width 4096 sector_size 4096 type 0
		generation 0 start_offset 0 dev_group 0
		seek_speed 0 bandwidth 0
		uuid 56a0a6bc-4630-4998-8daf-3c3030c4256a  <- sprout uuid
		fsid ab2c88b7-be81-4a7e-9849-c3666e7f9f4f <- sprout fsid

But the replaced target has the following uuid+fsid in its superblock
which doesn't match with the expected uuid+fsid in its devitem.

btrfs in dump-super /dev/sdc | egrep '^generation|dev_item.uuid|dev_item.fsid|devid'
generation	20
dev_item.uuid	59368f50-9af2-4b17-91da-8a783cc418d4
dev_item.fsid	ab2c88b7-be81-4a7e-9849-c3666e7f9f4f [match]
dev_item.devid	1

So if you provide the original seed device the mount shall be successful.
Which so long happening in the test case btrfs/163.

btrfs dev scan --forget
mount -o device=/dev/sda /dev/sdb /btrfs

Fix in this patch:
If a seed is not sprouted then there is no replacement of it, because of
its RO filesystem with an RO device. Similarly, in the case of a sprouted
filesystem, the seed device is still RO. So, mark it as you can't replace
a seed device, you can only add a new device and then delete the seed
device. If replace is attempted then returns -EINVAL.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/dev-replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 580e60fe07d0..7637b1f02c63 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -230,7 +230,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	int ret = 0;
 
 	*device_out = NULL;
-	if (fs_info->fs_devices->seeding) {
+	if (srcdev->fs_devices->seeding) {
 		btrfs_err(fs_info, "the filesystem is a seed filesystem!");
 		return -EINVAL;
 	}
-- 
2.25.1

[PATCH 03/16] btrfs: add btrfs_sysfs_add_device helper

[Anand Jain]

btrfs_sysfs_add_devices_dir() adds device link and devid kobject
(sysfs entries) for a device or all the devices in the btrfs_fs_devices.
In preparation to add these sysfs entries for the seed as well, add
a btrfs_sysfs_add_device() helper function and avoid code duplication.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v3: Use device->fs_info instead of device->fs_devices->fs_info
    essentially both points to the same addr, but former is more
    efficient.
    Fix whitespace, which didn't appear red in the git diff earlier,
    strange.

 fs/btrfs/sysfs.c | 79 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 438a367371c4..4377b991f6d6 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1275,44 +1275,71 @@ static struct kobj_type devid_ktype = {
 	.release	= btrfs_release_devid_kobj,
 };
 
-int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
-				struct btrfs_device *one_device)
+static int btrfs_sysfs_add_device(struct btrfs_device *device)
 {
-	int error = 0;
-	struct btrfs_device *dev;
+	int ret;
 	unsigned int nofs_flag;
+	struct kobject *devices_kobj;
+	struct kobject *devinfo_kobj;
 
-	nofs_flag = memalloc_nofs_save();
-	list_for_each_entry(dev, &fs_devices->devices, dev_list) {
+	/*
+	 * make sure we use the fs_info::fs_devices to fetch the kobjects
+	 * even for the seed fs_devices
+	 */
+	devices_kobj = device->fs_info->fs_devices->devices_kobj;
+	devinfo_kobj = device->fs_info->fs_devices->devinfo_kobj;
+	ASSERT(devices_kobj);
+	ASSERT(devinfo_kobj);
 
-		if (one_device && one_device != dev)
-			continue;
+	nofs_flag = memalloc_nofs_save();
 
-		if (dev->bdev) {
-			struct hd_struct *disk;
-			struct kobject *disk_kobj;
+	if (device->bdev) {
+		struct hd_struct *disk;
+		struct kobject *disk_kobj;
 
-			disk = dev->bdev->bd_part;
-			disk_kobj = &part_to_dev(disk)->kobj;
+		disk = device->bdev->bd_part;
+		disk_kobj = &part_to_dev(disk)->kobj;
 
-			error = sysfs_create_link(fs_devices->devices_kobj,
-						  disk_kobj, disk_kobj->name);
-			if (error)
-				break;
+		ret = sysfs_create_link(devices_kobj, disk_kobj,
+					disk_kobj->name);
+		if (ret) {
+			btrfs_warn(device->fs_info,
+			   "sysfs create device link failed %d devid %llu",
+				   ret, device->devid);
+			goto out;
 		}
+	}
 
-		init_completion(&dev->kobj_unregister);
-		error = kobject_init_and_add(&dev->devid_kobj, &devid_ktype,
-					     fs_devices->devinfo_kobj, "%llu",
-					     dev->devid);
-		if (error) {
-			kobject_put(&dev->devid_kobj);
-			break;
-		}
+	init_completion(&device->kobj_unregister);
+	ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype,
+				   devinfo_kobj, "%llu", device->devid);
+	if (ret) {
+		kobject_put(&device->devid_kobj);
+		btrfs_warn(device->fs_info,
+			   "sysfs devinfo init failed %d devid %llu",
+			   ret, device->devid);
 	}
+
+out:
 	memalloc_nofs_restore(nofs_flag);
+	return ret;
+}
 
-	return error;
+int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
+				struct btrfs_device *one_device)
+{
+	int ret;
+
+	if (one_device)
+		return btrfs_sysfs_add_device(one_device);
+
+	list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
+		ret = btrfs_sysfs_add_device(one_device);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 void btrfs_kobject_uevent(struct block_device *bdev, enum kobject_action action)
-- 
2.25.1

[PATCH 04/16] btrfs: add btrfs_sysfs_remove_device helper

[Anand Jain]

btrfs_sysfs_remove_devices_dir() removes device link and devid kobject
(sysfs entries) for a device or all the devices in the btrfs_fs_devices.
In preparation to remove these sysfs entries for the seed as well, add
a btrfs_sysfs_remove_device() helper function and avoid code
duplication.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/sysfs.c | 61 +++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 4377b991f6d6..33703fe01c72 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1149,50 +1149,43 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-/* when one_device is NULL, it removes all device links */
-
-int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
-		struct btrfs_device *one_device)
+static void btrfs_sysfs_remove_device(struct btrfs_device *device)
 {
 	struct hd_struct *disk;
 	struct kobject *disk_kobj;
+	struct kobject *devices_kobj;
 
-	if (!fs_devices->devices_kobj)
-		return -EINVAL;
-
-	if (one_device) {
-		if (one_device->bdev) {
-			disk = one_device->bdev->bd_part;
-			disk_kobj = &part_to_dev(disk)->kobj;
-			sysfs_remove_link(fs_devices->devices_kobj,
-					  disk_kobj->name);
-		}
+	/*
+	 * Seed fs_devices devices_kobj aren't used, fetch kobject from the
+	 * fs_info::fs_devices.
+	 */
+	devices_kobj = device->fs_info->fs_devices->devices_kobj;
+	ASSERT(devices_kobj);
 
-		if (one_device->devid_kobj.state_initialized) {
-			kobject_del(&one_device->devid_kobj);
-			kobject_put(&one_device->devid_kobj);
+	if (device->bdev) {
+		disk = device->bdev->bd_part;
+		disk_kobj = &part_to_dev(disk)->kobj;
+		sysfs_remove_link(devices_kobj, disk_kobj->name);
+	}
 
-			wait_for_completion(&one_device->kobj_unregister);
-		}
+	if (device->devid_kobj.state_initialized) {
+		kobject_del(&device->devid_kobj);
+		kobject_put(&device->devid_kobj);
+		wait_for_completion(&device->kobj_unregister);
+	}
+}
 
+/* when 2nd argument device is NULL, it removes all devices link */
+int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
+				   struct btrfs_device *one_device)
+{
+	if (one_device) {
+		btrfs_sysfs_remove_device(one_device);
 		return 0;
 	}
 
-	list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
-
-		if (one_device->bdev) {
-			disk = one_device->bdev->bd_part;
-			disk_kobj = &part_to_dev(disk)->kobj;
-			sysfs_remove_link(fs_devices->devices_kobj,
-					  disk_kobj->name);
-		}
-		if (one_device->devid_kobj.state_initialized) {
-			kobject_del(&one_device->devid_kobj);
-			kobject_put(&one_device->devid_kobj);
-
-			wait_for_completion(&one_device->kobj_unregister);
-		}
-	}
+	list_for_each_entry(one_device, &fs_devices->devices, dev_list)
+		btrfs_sysfs_remove_device(one_device);
 
 	return 0;
 }
-- 
2.25.1

[PATCH 05/16] btrfs: btrfs_sysfs_remove_devices_dir drop return value

[Anand Jain]

btrfs_sysfs_remove_devices_dir() return value is unused declare it as
void.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/sysfs.c | 8 +++-----
 fs/btrfs/sysfs.h | 4 ++--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 33703fe01c72..5d7cb6a81f0d 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1176,18 +1176,16 @@ static void btrfs_sysfs_remove_device(struct btrfs_device *device)
 }
 
 /* when 2nd argument device is NULL, it removes all devices link */
-int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
-				   struct btrfs_device *one_device)
+void btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
+				    struct btrfs_device *one_device)
 {
 	if (one_device) {
 		btrfs_sysfs_remove_device(one_device);
-		return 0;
+		return;
 	}
 
 	list_for_each_entry(one_device, &fs_devices->devices, dev_list)
 		btrfs_sysfs_remove_device(one_device);
-
-	return 0;
 }
 
 static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index 4217823e255c..bc995b0c1889 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -16,8 +16,8 @@ char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
 const char *btrfs_feature_set_name(enum btrfs_feature_set set);
 int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
 		struct btrfs_device *one_device);
-int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
-                struct btrfs_device *one_device);
+void btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
+				    struct btrfs_device *one_device);
 int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs);
 void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs);
 void btrfs_sysfs_update_sprout_fsid(struct btrfs_fs_devices *fs_devices);
-- 
2.25.1

[PATCH 06/16] btrfs: refactor btrfs_sysfs_add_devices_dir

[Anand Jain]

When we add device we need to add a device to the sysfs, so instead of
using the btrfs_sysfs_add_devices_dir() 2nd argument to specify whether
to add a device or all of fs_devices, call the helper function directly
btrfs_sysfs_add_device() and thus make it non static.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/dev-replace.c |  2 +-
 fs/btrfs/sysfs.c       | 15 ++++++---------
 fs/btrfs/sysfs.h       |  3 +--
 fs/btrfs/volumes.c     |  2 +-
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 7637b1f02c63..2e54b8f4d053 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -512,7 +512,7 @@ static int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	atomic64_set(&dev_replace->num_uncorrectable_read_errors, 0);
 	up_write(&dev_replace->rwsem);
 
-	ret = btrfs_sysfs_add_devices_dir(tgt_device->fs_devices, tgt_device);
+	ret = btrfs_sysfs_add_device(tgt_device);
 	if (ret)
 		btrfs_err(fs_info, "kobj add dev failed %d", ret);
 
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5d7cb6a81f0d..1da1c3ba2143 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1266,7 +1266,7 @@ static struct kobj_type devid_ktype = {
 	.release	= btrfs_release_devid_kobj,
 };
 
-static int btrfs_sysfs_add_device(struct btrfs_device *device)
+int btrfs_sysfs_add_device(struct btrfs_device *device)
 {
 	int ret;
 	unsigned int nofs_flag;
@@ -1316,16 +1316,13 @@ static int btrfs_sysfs_add_device(struct btrfs_device *device)
 	return ret;
 }
 
-int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
-				struct btrfs_device *one_device)
+int btrfs_sysfs_add_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	int ret;
+	struct btrfs_device *device;
 
-	if (one_device)
-		return btrfs_sysfs_add_device(one_device);
-
-	list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
-		ret = btrfs_sysfs_add_device(one_device);
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+		ret = btrfs_sysfs_add_device(device);
 		if (ret)
 			return ret;
 	}
@@ -1420,7 +1417,7 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 	struct btrfs_fs_devices *fs_devs = fs_info->fs_devices;
 	struct kobject *fsid_kobj = &fs_devs->fsid_kobj;
 
-	error = btrfs_sysfs_add_devices_dir(fs_devs, NULL);
+	error = btrfs_sysfs_add_fs_devices(fs_devs);
 	if (error)
 		return error;
 
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index bc995b0c1889..56c29257b5a7 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -14,8 +14,7 @@ enum btrfs_feature_set {
 
 char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
 const char *btrfs_feature_set_name(enum btrfs_feature_set set);
-int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
-		struct btrfs_device *one_device);
+int btrfs_sysfs_add_device(struct btrfs_device *device);
 void btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
 				    struct btrfs_device *one_device);
 int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index df3a31fcc4d7..f858c4d9a6ae 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2606,7 +2606,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	mutex_unlock(&fs_info->chunk_mutex);
 
 	/* Add sysfs device entry */
-	btrfs_sysfs_add_devices_dir(fs_devices, device);
+	btrfs_sysfs_add_device(device);
 
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-- 
2.25.1

[PATCH 07/16] btrfs: refactor btrfs_sysfs_remove_devices_dir

[Anand Jain]

Similar to btrfs_sysfs_add_devices_dir()'s refactor, refactor
btrfs_sysfs_remove_devices_dir() so that we don't have to use the 2nd
argument to indicate whether to free all devices or just one device.

Make btrfs_sysfs_remove_device() global as device operations outside of
sysfs.c now calls this instead of btrfs_sysfs_remove_devices_dir().

btrfs_sysfs_remove_devices_dir() is renamed to
btrfs_sysfs_remove_fs_devices() to suite its new role.

Now, no one outside of sysfs.c calls btrfs_sysfs_remove_fs_devices()
so it is redeclared as static. And the same function had to be moved
before its first caller.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c |  2 +-
 fs/btrfs/sysfs.c       | 27 +++++++++++----------------
 fs/btrfs/sysfs.h       |  3 +--
 fs/btrfs/volumes.c     |  8 ++++----
 4 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 2e54b8f4d053..70b4385a327c 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -743,7 +743,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 
 	/* replace the sysfs entry */
-	btrfs_sysfs_remove_devices_dir(fs_info->fs_devices, src_device);
+	btrfs_sysfs_remove_device(src_device);
 	btrfs_sysfs_update_devid(tgt_device);
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &src_device->dev_state))
 		btrfs_scratch_superblocks(fs_info, src_device->bdev,
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 1da1c3ba2143..c5b8870b12bf 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -935,6 +935,14 @@ void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs)
 	}
 }
 
+void btrfs_sysfs_remove_fs_devices(struct btrfs_fs_devices *fs_devices)
+{
+	struct btrfs_device *device;
+
+	list_for_each_entry(device, &fs_devices->devices, dev_list)
+		btrfs_sysfs_remove_device(device);
+}
+
 void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 {
 	struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
@@ -962,7 +970,7 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
 	addrm_unknown_feature_attrs(fs_info, false);
 	sysfs_remove_group(fsid_kobj, &btrfs_feature_attr_group);
 	sysfs_remove_files(fsid_kobj, btrfs_attrs);
-	btrfs_sysfs_remove_devices_dir(fs_info->fs_devices, NULL);
+	btrfs_sysfs_remove_fs_devices(fs_info->fs_devices);
 }
 
 static const char * const btrfs_feature_set_names[FEAT_MAX] = {
@@ -1149,7 +1157,7 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static void btrfs_sysfs_remove_device(struct btrfs_device *device)
+void btrfs_sysfs_remove_device(struct btrfs_device *device)
 {
 	struct hd_struct *disk;
 	struct kobject *disk_kobj;
@@ -1175,19 +1183,6 @@ static void btrfs_sysfs_remove_device(struct btrfs_device *device)
 	}
 }
 
-/* when 2nd argument device is NULL, it removes all devices link */
-void btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
-				    struct btrfs_device *one_device)
-{
-	if (one_device) {
-		btrfs_sysfs_remove_device(one_device);
-		return;
-	}
-
-	list_for_each_entry(one_device, &fs_devices->devices, dev_list)
-		btrfs_sysfs_remove_device(one_device);
-}
-
 static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
 					         struct kobj_attribute *a,
 					         char *buf)
@@ -1423,7 +1418,7 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
 
 	error = sysfs_create_files(fsid_kobj, btrfs_attrs);
 	if (error) {
-		btrfs_sysfs_remove_devices_dir(fs_devs, NULL);
+		btrfs_sysfs_remove_fs_devices(fs_devs);
 		return error;
 	}
 
diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
index 56c29257b5a7..bacef43f7267 100644
--- a/fs/btrfs/sysfs.h
+++ b/fs/btrfs/sysfs.h
@@ -15,8 +15,7 @@ enum btrfs_feature_set {
 char *btrfs_printable_features(enum btrfs_feature_set set, u64 flags);
 const char *btrfs_feature_set_name(enum btrfs_feature_set set);
 int btrfs_sysfs_add_device(struct btrfs_device *device);
-void btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
-				    struct btrfs_device *one_device);
+void btrfs_sysfs_remove_device(struct btrfs_device *device);
 int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs);
 void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs);
 void btrfs_sysfs_update_sprout_fsid(struct btrfs_fs_devices *fs_devices);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f858c4d9a6ae..afdde29bce34 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2039,7 +2039,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
-		u64 devid)
+		    u64 devid)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *cur_devices;
@@ -2143,7 +2143,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (device->bdev) {
 		cur_devices->open_devices--;
 		/* remove sysfs entry */
-		btrfs_sysfs_remove_devices_dir(fs_devices, device);
+		btrfs_sysfs_remove_device(device);
 	}
 
 	num_devices = btrfs_super_num_devices(fs_info->super_copy) - 1;
@@ -2244,7 +2244,7 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 
 	mutex_lock(&fs_devices->device_list_mutex);
 
-	btrfs_sysfs_remove_devices_dir(fs_devices, tgtdev);
+	btrfs_sysfs_remove_device(tgtdev);
 
 	if (tgtdev->bdev)
 		fs_devices->open_devices--;
@@ -2680,7 +2680,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	return ret;
 
 error_sysfs:
-	btrfs_sysfs_remove_devices_dir(fs_devices, device);
+	btrfs_sysfs_remove_device(device);
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	mutex_lock(&fs_info->chunk_mutex);
 	list_del_rcu(&device->dev_list);
-- 
2.25.1

[PATCH 08/16] btrfs: initialize sysfs devid and device link for seed device

[Anand Jain]

We don't initialize the sysfs devid kobject and device-link yet for the
seed devices in an sprouted filesystem.
So this patch initializes the seed device devid kobject and the device
link in the sysfs.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/sysfs.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index c5b8870b12bf..5b81bb60c86e 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -938,9 +938,15 @@ void btrfs_sysfs_remove_fsid(struct btrfs_fs_devices *fs_devs)
 void btrfs_sysfs_remove_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device;
+	struct btrfs_fs_devices *seed;
 
 	list_for_each_entry(device, &fs_devices->devices, dev_list)
 		btrfs_sysfs_remove_device(device);
+
+	list_for_each_entry(seed, &fs_devices->seed_list, seed_list) {
+		list_for_each_entry(device, &seed->devices, dev_list)
+			btrfs_sysfs_remove_device(device);
+	}
 }
 
 void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info)
@@ -1315,6 +1321,7 @@ int btrfs_sysfs_add_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	int ret;
 	struct btrfs_device *device;
+	struct btrfs_fs_devices *seed;
 
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		ret = btrfs_sysfs_add_device(device);
@@ -1322,6 +1329,14 @@ int btrfs_sysfs_add_fs_devices(struct btrfs_fs_devices *fs_devices)
 			return ret;
 	}
 
+	list_for_each_entry(seed, &fs_devices->seed_list, seed_list) {
+		list_for_each_entry(device, &seed->devices, dev_list) {
+			ret = btrfs_sysfs_add_device(device);
+			if (ret)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 09/16] btrfs: handle fail path for btrfs_sysfs_add_fs_devices

[Anand Jain]

btrfs_sysfs_add_fs_devices() is called by btrfs_sysfs_add_mounted().
btrfs_sysfs_add_mounted() assumes that btrfs_sysfs_add_fs_devices() will
either add sysfs entries for all the devices or none. So this patch keeps up
to its caller expecatation and cleans up the created sysfs entries if it
has to fail at some device in the list.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v3: fix the goto fail
 fs/btrfs/sysfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5b81bb60c86e..71c3a98b799f 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1326,18 +1326,22 @@ int btrfs_sysfs_add_fs_devices(struct btrfs_fs_devices *fs_devices)
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
 		ret = btrfs_sysfs_add_device(device);
 		if (ret)
-			return ret;
+			goto fail;
 	}
 
 	list_for_each_entry(seed, &fs_devices->seed_list, seed_list) {
 		list_for_each_entry(device, &seed->devices, dev_list) {
 			ret = btrfs_sysfs_add_device(device);
 			if (ret)
-				return ret;
+				goto fail;
 		}
 	}
 
 	return 0;
+
+fail:
+	btrfs_sysfs_remove_fs_devices(fs_devices);
+	return ret;
 }
 
 void btrfs_kobject_uevent(struct block_device *bdev, enum kobject_action action)
-- 
2.25.1

[PATCH 10/16] btrfs: reada: use sprout device_list_mutex

[Anand Jain]

On an fs mounted using a sprout-device, the seed fs_devices are maintained
in a linked list under fs_info->fs_devices. Each seed's fs_devices also
have device_list_mutex initialized to protect against the potential race
with delete threads. But the delete thread (at btrfs_rm_device()) is holding
the fs_info::fs_devices::device_list_mutex mutex which is sprout's
device_list_mutex instead of seed's device_list_mutex. Moreover, there
aren't any significient benefits in using the seed::device_list_mutex
instead of sprout::device_list_mutex.

So this patch converts them of using the seed::device_list_mutex to
sprout::device_list_mutex.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/reada.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index e20972230823..9d4f5316a7e8 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -776,13 +776,11 @@ static int reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices)
 
 	do {
 		enqueued = 0;
-		mutex_lock(&fs_devices->device_list_mutex);
 		list_for_each_entry(device, &fs_devices->devices, dev_list) {
 			if (atomic_read(&device->reada_in_flight) <
 			    MAX_IN_FLIGHT)
 				enqueued += reada_start_machine_dev(device);
 		}
-		mutex_unlock(&fs_devices->device_list_mutex);
 		total += enqueued;
 	} while (enqueued && total < 10000);
 
@@ -795,10 +793,13 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info)
 	int i;
 	u64 enqueued = 0;
 
+	mutex_lock(&fs_devices->device_list_mutex);
+
 	enqueued += reada_start_for_fsdevs(fs_devices);
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list)
 		enqueued += reada_start_for_fsdevs(seed_devs);
 
+	mutex_unlock(&fs_devices->device_list_mutex);
 	if (enqueued == 0)
 		return;
 
-- 
2.25.1

[PATCH 11/16] btrfs: btrfs_init_devices_late: use sprout device_list_mutex

[Anand Jain]

In a mounted sprout FS, all threads now are using the
sprout::device_list_mutex, and this is the only piece of code using
the seed::device_list_mutex. This patch converts to use the sprouts
fs_info->fs_devices->device_list_mutex.

The same reasoning holds true here, that device delete is holding
the sprout::device_list_mutex.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index afdde29bce34..f70b79eaa76d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7193,16 +7193,14 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info)
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry(device, &fs_devices->devices, dev_list)
 		device->fs_info = fs_info;
-	mutex_unlock(&fs_devices->device_list_mutex);
 
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
-		mutex_lock(&seed_devs->device_list_mutex);
 		list_for_each_entry(device, &seed_devs->devices, dev_list)
 			device->fs_info = fs_info;
-		mutex_unlock(&seed_devs->device_list_mutex);
 
 		seed_devs->fs_info = fs_info;
 	}
+	mutex_unlock(&fs_devices->device_list_mutex);
 }
 
 static u64 btrfs_dev_stats_value(const struct extent_buffer *eb,
-- 
2.25.1

[PATCH 12/16] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev

[Anand Jain]

In the function btrfs_init_dev_replace_tgtdev(), the local
variable struct list_head *devices is used only once, instead
just open code the same.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 70b4385a327c..f7325a748f89 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -224,7 +224,6 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_device *device;
 	struct block_device *bdev;
-	struct list_head *devices;
 	struct rcu_string *name;
 	u64 devid = BTRFS_DEV_REPLACE_DEVID;
 	int ret = 0;
@@ -244,8 +243,7 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 
 	sync_blockdev(bdev);
 
-	devices = &fs_info->fs_devices->devices;
-	list_for_each_entry(device, devices, dev_list) {
+	list_for_each_entry(device, &fs_info->fs_devices->devices, dev_list) {
 		if (device->bdev == bdev) {
 			btrfs_err(fs_info,
 				  "target device is in the filesystem!");
-- 
2.25.1

[PATCH 13/16] btrfs: cleanup btrfs_remove_chunk

[Anand Jain]

In the function btrfs_remove_chunk() remove the local variable
%fs_devices, instead use the assigned pointer directly.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f70b79eaa76d..f16c4a854a6c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2922,7 +2922,6 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	struct map_lookup *map;
 	u64 dev_extent_len = 0;
 	int i, ret = 0;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 
 	em = btrfs_get_chunk_map(fs_info, chunk_offset, 1);
 	if (IS_ERR(em)) {
@@ -2944,14 +2943,14 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	 * a device replace operation that replaces the device object associated
 	 * with map stripes (dev-replace.c:btrfs_dev_replace_finishing()).
 	 */
-	mutex_lock(&fs_devices->device_list_mutex);
+	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	for (i = 0; i < map->num_stripes; i++) {
 		struct btrfs_device *device = map->stripes[i].dev;
 		ret = btrfs_free_dev_extent(trans, device,
 					    map->stripes[i].physical,
 					    &dev_extent_len);
 		if (ret) {
-			mutex_unlock(&fs_devices->device_list_mutex);
+			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 			btrfs_abort_transaction(trans, ret);
 			goto out;
 		}
@@ -2967,12 +2966,12 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 
 		ret = btrfs_update_device(trans, device);
 		if (ret) {
-			mutex_unlock(&fs_devices->device_list_mutex);
+			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 			btrfs_abort_transaction(trans, ret);
 			goto out;
 		}
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
+	mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 
 	ret = btrfs_free_chunk(trans, chunk_offset);
 	if (ret) {
-- 
2.25.1

[PATCH 14/16] btrfs: cleanup btrfs_assign_next_active_device()

[Anand Jain]

Cleanup btrfs_assign_next_active_device(), drop %this_dev.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f16c4a854a6c..f2ade4af33fd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1960,16 +1960,13 @@ static struct btrfs_device * btrfs_find_next_active_device(
  * this_dev) which is active.
  */
 void __cold btrfs_assign_next_active_device(struct btrfs_device *device,
-				     struct btrfs_device *this_dev)
+					    struct btrfs_device *next_device)
 {
 	struct btrfs_fs_info *fs_info = device->fs_info;
-	struct btrfs_device *next_device;
 
-	if (this_dev)
-		next_device = this_dev;
-	else
+	if (!next_device)
 		next_device = btrfs_find_next_active_device(fs_info->fs_devices,
-								device);
+							    device);
 	ASSERT(next_device);
 
 	if (fs_info->sb->s_bdev &&
-- 
2.25.1

[PATCH 15/16] btrfs: cleanup unnecessary goto in open_seed_device

[Anand Jain]

open_seed_devices() does goto to just return. So drop goto and just return
instead.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f2ade4af33fd..385fbd2b2d76 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6750,19 +6750,17 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 	ret = open_fs_devices(fs_devices, FMODE_READ, fs_info->bdev_holder);
 	if (ret) {
 		free_fs_devices(fs_devices);
-		fs_devices = ERR_PTR(ret);
-		goto out;
+		return ERR_PTR(ret);
 	}
 
 	if (!fs_devices->seeding) {
 		close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
-		fs_devices = ERR_PTR(-EINVAL);
-		goto out;
+		return ERR_PTR(-EINVAL);
 	}
 
 	list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list);
-out:
+
 	return fs_devices;
 }
 
-- 
2.25.1

[PATCH 16/16] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare

[Anand Jain]

There isn't any convoluted child functions inside the
btrfs_dev_replace_update_device_in_mapping_tree() function. With the
function moved before where it is called, we can drop its file local
declare.

No functional changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c | 56 ++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index f7325a748f89..02b7b3edf9a3 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -64,10 +64,6 @@
 
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				       int scrub_ret);
-static void btrfs_dev_replace_update_device_in_mapping_tree(
-						struct btrfs_fs_info *fs_info,
-						struct btrfs_device *srcdev,
-						struct btrfs_device *tgtdev);
 static int btrfs_dev_replace_kthread(void *data);
 
 int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
@@ -597,6 +593,32 @@ static void btrfs_rm_dev_replace_unblocked(struct btrfs_fs_info *fs_info)
 	wake_up(&fs_info->dev_replace.replace_wait);
 }
 
+static void btrfs_dev_replace_update_device_in_mapping_tree(
+						struct btrfs_fs_info *fs_info,
+						struct btrfs_device *srcdev,
+						struct btrfs_device *tgtdev)
+{
+	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
+	struct extent_map *em;
+	struct map_lookup *map;
+	u64 start = 0;
+	int i;
+
+	write_lock(&em_tree->lock);
+	do {
+		em = lookup_extent_mapping(em_tree, start, (u64)-1);
+		if (!em)
+			break;
+		map = em->map_lookup;
+		for (i = 0; i < map->num_stripes; i++)
+			if (srcdev == map->stripes[i].dev)
+				map->stripes[i].dev = tgtdev;
+		start = em->start + em->len;
+		free_extent_map(em);
+	} while (start);
+	write_unlock(&em_tree->lock);
+}
+
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 				       int scrub_ret)
 {
@@ -759,32 +781,6 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static void btrfs_dev_replace_update_device_in_mapping_tree(
-						struct btrfs_fs_info *fs_info,
-						struct btrfs_device *srcdev,
-						struct btrfs_device *tgtdev)
-{
-	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
-	struct extent_map *em;
-	struct map_lookup *map;
-	u64 start = 0;
-	int i;
-
-	write_lock(&em_tree->lock);
-	do {
-		em = lookup_extent_mapping(em_tree, start, (u64)-1);
-		if (!em)
-			break;
-		map = em->map_lookup;
-		for (i = 0; i < map->num_stripes; i++)
-			if (srcdev == map->stripes[i].dev)
-				map->stripes[i].dev = tgtdev;
-		start = em->start + em->len;
-		free_extent_map(em);
-	} while (start);
-	write_unlock(&em_tree->lock);
-}
-
 /*
  * Read progress of device replace status according to the state and last
  * stored position. The value format is the same as for
-- 
2.25.1

[PATCH v2 0/2] fstests: btrfs seed device device operation tests

[Anand Jain]

Tests and updates seed device operations test cases.

Anand Jain (2):
  btrfs: add a test case for btrfs seed device delete
  btrfs/163: replace sprout instead of seed

 tests/btrfs/163     | 21 +++++++++---
 tests/btrfs/163.out |  5 ++-
 tests/btrfs/219     | 83 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/219.out | 15 ++++++++
 tests/btrfs/group   |  1 +
 5 files changed, 119 insertions(+), 6 deletions(-)
 create mode 100755 tests/btrfs/219
 create mode 100644 tests/btrfs/219.out

-- 
2.25.1

[PATCH v2 1/2] btrfs: add a test case for btrfs seed device delete

[Anand Jain]

This is a regression test for the issue fixed by the kernel patch
   btrfs: fix put of uninitialized kobject after seed device delete

In this test case, we verify the seed device delete on a sprouted
filesystem.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2 drop the sysfs layout check as it breaks the test-case backward
compatibility.

 tests/btrfs/219     | 83 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/219.out | 15 ++++++++
 tests/btrfs/group   |  1 +
 3 files changed, 99 insertions(+)
 create mode 100755 tests/btrfs/219
 create mode 100644 tests/btrfs/219.out

diff --git a/tests/btrfs/219 b/tests/btrfs/219
new file mode 100755
index 000000000000..86f2a6991bd7
--- /dev/null
+++ b/tests/btrfs/219
@@ -0,0 +1,83 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 Oracle. All Rights Reserved.
+#
+# FS QA Test 219
+#
+# Test for seed device-delete on a sprouted FS.
+# Requires kernel patch
+#    btrfs: fix put of uninitialized kobject after seed device delete
+#
+# Steps:
+#  Create a seed FS. Add a RW device to make it sprout FS and then delete
+#  the seed device.
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_test
+_require_scratch_dev_pool 2
+
+_scratch_dev_pool_get 2
+
+seed=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
+sprout=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
+
+_mkfs_dev $seed
+_mount $seed $SCRATCH_MNT
+
+$XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
+_scratch_unmount
+$BTRFS_TUNE_PROG -S 1 $seed
+
+# Mount the seed device and add the rw device
+_mount -o ro $seed $SCRATCH_MNT
+$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
+_scratch_unmount
+
+# Now remount
+_mount $sprout $SCRATCH_MNT
+$XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
+
+echo --- before delete ----
+od -x $SCRATCH_MNT/foo
+od -x $SCRATCH_MNT/bar
+
+$BTRFS_UTIL_PROG device delete $seed $SCRATCH_MNT
+_scratch_unmount
+_btrfs_forget_or_module_reload
+_mount $sprout $SCRATCH_MNT
+
+echo --- after delete ----
+od -x $SCRATCH_MNT/foo
+od -x $SCRATCH_MNT/bar
+
+_scratch_dev_pool_put
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/219.out b/tests/btrfs/219.out
new file mode 100644
index 000000000000..d39e0d8ffafd
--- /dev/null
+++ b/tests/btrfs/219.out
@@ -0,0 +1,15 @@
+QA output created by 219
+--- before delete ----
+0000000 abab abab abab abab abab abab abab abab
+*
+4000000
+0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
+*
+4000000
+--- after delete ----
+0000000 abab abab abab abab abab abab abab abab
+*
+4000000
+0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
+*
+4000000
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 3295856d0c8c..3633fa66abe4 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -221,3 +221,4 @@
 216 auto quick seed
 217 auto quick trim dangerous
 218 auto quick volume
+219 auto quick volume seed
-- 
2.25.1

[PATCH 2/2] btrfs/163: replace sprout instead of seed

[Anand Jain]

Make this test case inline with the kernel patch [1] changes
[1] btrfs: fix replace of seed device

So use the sprout device as the replace target instead of the seed device.
This change is compatible with the older kernels.

While at this, this patch also fixes a typo fix as well.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/163     | 21 ++++++++++++++++-----
 tests/btrfs/163.out |  5 ++++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/tests/btrfs/163 b/tests/btrfs/163
index 24c725afb6b9..354d88502d47 100755
--- a/tests/btrfs/163
+++ b/tests/btrfs/163
@@ -4,11 +4,15 @@
 #
 # FS QA Test 163
 #
-# Test case to verify that a seed device can be replaced
+# Test case to verify that a sprouted device can be replaced
 #  Create a seed device
 #  Create a sprout device
 #  Remount RW
-#  Run device replace on the seed device
+#  Run device replace on the sprout device
+#
+# Depends on the kernel patch
+#   btrfs: fail replace of seed device
+
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
 echo "QA output created by $seq"
@@ -39,6 +43,7 @@ _supported_fs btrfs
 _supported_os Linux
 _require_command "$BTRFS_TUNE_PROG" btrfstune
 _require_scratch_dev_pool 3
+_require_btrfs_forget_or_module_loadable
 
 _scratch_dev_pool_get 3
 
@@ -52,7 +57,7 @@ create_seed()
 	run_check _mount $dev_seed $SCRATCH_MNT
 	$XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 4M" $SCRATCH_MNT/foobar >\
 		/dev/null
-	echo -- gloden --
+	echo -- golden --
 	od -x $SCRATCH_MNT/foobar
 	_run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
 	_scratch_unmount
@@ -64,22 +69,28 @@ add_sprout()
 {
 	_run_btrfs_util_prog device add -f $dev_sprout $SCRATCH_MNT
 	_run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
+	_mount -o remount,rw $dev_sprout $SCRATCH_MNT
+	$XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 4M" $SCRATCH_MNT/foobar2 >\
+		/dev/null
 }
 
 replace_seed()
 {
-	_run_btrfs_util_prog replace start -fB $dev_seed $dev_replace_tgt $SCRATCH_MNT
+	_run_btrfs_util_prog replace start -fB $dev_sprout $dev_replace_tgt $SCRATCH_MNT
 	_run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
 	_scratch_unmount
-	run_check _mount $dev_replace_tgt $SCRATCH_MNT
+	_btrfs_forget_or_module_reload
+	run_check _mount -o device=$dev_seed $dev_replace_tgt $SCRATCH_MNT
 	echo -- sprout --
 	od -x $SCRATCH_MNT/foobar
+	od -x $SCRATCH_MNT/foobar2
 	_scratch_unmount
 
 }
 
 seed_is_mountable()
 {
+	_btrfs_forget_or_module_reload
 	run_check _mount $dev_seed $SCRATCH_MNT
 	_run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
 	_scratch_unmount
diff --git a/tests/btrfs/163.out b/tests/btrfs/163.out
index 91f6f5b6f48a..351ef7b040b2 100644
--- a/tests/btrfs/163.out
+++ b/tests/btrfs/163.out
@@ -1,5 +1,5 @@
 QA output created by 163
--- gloden --
+-- golden --
 0000000 abab abab abab abab abab abab abab abab
 *
 20000000
@@ -7,3 +7,6 @@ QA output created by 163
 0000000 abab abab abab abab abab abab abab abab
 *
 20000000
+0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
+*
+20000000
-- 
2.25.1

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[Johannes Thumshirn]

On 04/09/2020 19:37, Anand Jain wrote:
> The following test case leads to null kobject-being-freed error.
> 
>  mount seed /mnt
>  add sprout to /mnt
>  umount /mnt
>  mount sprout to /mnt
>  delete seed
> 
>  kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called.
>  WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350
>  RIP: 0010:kobject_put+0x80/0x350
>  ::
>  Call Trace:
>  btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs]
>  btrfs_rm_device.cold+0xa8/0x298 [btrfs]
>  btrfs_ioctl+0x206c/0x22a0 [btrfs]
>  ksys_ioctl+0xe2/0x140
>  __x64_sys_ioctl+0x1e/0x29
>  do_syscall_64+0x96/0x150
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>  RIP: 0033:0x7f4047c6288b
>  ::
> 
> This is because, at the end of the seed device-delete, we try to remove
> the seed's devid sysfs entry. But for the seed devices under the sprout
> fs, we don't initialize the devid kobject yet. So add a kobject state
> check, which takes care of the Warning.
> 
> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/sysfs.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 190e59152be5..438a367371c4 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>  					  disk_kobj->name);
>  		}
>  
> -		kobject_del(&one_device->devid_kobj);
> -		kobject_put(&one_device->devid_kobj);
> +		if (one_device->devid_kobj.state_initialized) {
> +			kobject_del(&one_device->devid_kobj);
> +			kobject_put(&one_device->devid_kobj);
>  
> -		wait_for_completion(&one_device->kobj_unregister);
> +			wait_for_completion(&one_device->kobj_unregister);
> +		}
>  
>  		return 0;
>  	}
> @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>  			sysfs_remove_link(fs_devices->devices_kobj,
>  					  disk_kobj->name);
>  		}
> -		kobject_del(&one_device->devid_kobj);
> -		kobject_put(&one_device->devid_kobj);
> +		if (one_device->devid_kobj.state_initialized) {
> +			kobject_del(&one_device->devid_kobj);
> +			kobject_put(&one_device->devid_kobj);
>  
> -		wait_for_completion(&one_device->kobj_unregister);
> +			wait_for_completion(&one_device->kobj_unregister);
> +		}
>  	}
>  
>  	return 0;
> 

Hmm this is a lot of duplicated code. It was before as well, this patch only adds
to the code duplication.

Can't we do sth like this:

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[Johannes Thumshirn]

On 07/09/2020 13:29, Johannes Thumshirn wrote:
> On 04/09/2020 19:37, Anand Jain wrote:
>> The following test case leads to null kobject-being-freed error.
>>
>>  mount seed /mnt
>>  add sprout to /mnt
>>  umount /mnt
>>  mount sprout to /mnt
>>  delete seed
>>
>>  kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called.
>>  WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350
>>  RIP: 0010:kobject_put+0x80/0x350
>>  ::
>>  Call Trace:
>>  btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs]
>>  btrfs_rm_device.cold+0xa8/0x298 [btrfs]
>>  btrfs_ioctl+0x206c/0x22a0 [btrfs]
>>  ksys_ioctl+0xe2/0x140
>>  __x64_sys_ioctl+0x1e/0x29
>>  do_syscall_64+0x96/0x150
>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>  RIP: 0033:0x7f4047c6288b
>>  ::
>>
>> This is because, at the end of the seed device-delete, we try to remove
>> the seed's devid sysfs entry. But for the seed devices under the sprout
>> fs, we don't initialize the devid kobject yet. So add a kobject state
>> check, which takes care of the Warning.
>>
>> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>  fs/btrfs/sysfs.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 190e59152be5..438a367371c4 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>>  					  disk_kobj->name);
>>  		}
>>  
>> -		kobject_del(&one_device->devid_kobj);
>> -		kobject_put(&one_device->devid_kobj);
>> +		if (one_device->devid_kobj.state_initialized) {
>> +			kobject_del(&one_device->devid_kobj);
>> +			kobject_put(&one_device->devid_kobj);
>>  
>> -		wait_for_completion(&one_device->kobj_unregister);
>> +			wait_for_completion(&one_device->kobj_unregister);
>> +		}
>>  
>>  		return 0;
>>  	}
>> @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>>  			sysfs_remove_link(fs_devices->devices_kobj,
>>  					  disk_kobj->name);
>>  		}
>> -		kobject_del(&one_device->devid_kobj);
>> -		kobject_put(&one_device->devid_kobj);
>> +		if (one_device->devid_kobj.state_initialized) {
>> +			kobject_del(&one_device->devid_kobj);
>> +			kobject_put(&one_device->devid_kobj);
>>  
>> -		wait_for_completion(&one_device->kobj_unregister);
>> +			wait_for_completion(&one_device->kobj_unregister);
>> +		}
>>  	}
>>  
>>  	return 0;
>>
> 
> Hmm this is a lot of duplicated code. It was before as well, this patch only adds
> to the code duplication.
> 
> Can't we do sth like this:
> 


Hit sent too early I'm sorry. I meant add this (untested) as a prep patch:

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 190e59152be5..84114a1ec502 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1151,44 +1151,40 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
 
 /* when one_device is NULL, it removes all device links */
 
+
+static void btrfs_sysfs_remove_one_devices_dir(struct btrfs_device *dev)
+{
+       if (one_device->bdev) {
+               struct hd_struct *disk;
+               struct kobject *disk_kobj;
+
+               disk = one_device->bdev->bd_part;
+               disk_kobj = &part_to_dev(disk)->kobj;
+               sysfs_remove_link(fs_devices->devices_kobj,
+                                 disk_kobj->name);
+       }
+
+       kobject_del(&one_device->devid_kobj);
+       kobject_put(&one_device->devid_kobj);
+
+       wait_for_completion(&one_device->kobj_unregister);
+
+       return;
+}
+
 int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
                struct btrfs_device *one_device)
 {
-       struct hd_struct *disk;
-       struct kobject *disk_kobj;
-
        if (!fs_devices->devices_kobj)
                return -EINVAL;
 
        if (one_device) {
-               if (one_device->bdev) {
-                       disk = one_device->bdev->bd_part;
-                       disk_kobj = &part_to_dev(disk)->kobj;
-                       sysfs_remove_link(fs_devices->devices_kobj,
-                                         disk_kobj->name);
-               }
-
-               kobject_del(&one_device->devid_kobj);
-               kobject_put(&one_device->devid_kobj);
-
-               wait_for_completion(&one_device->kobj_unregister);
-
+               btrfs_sysfs_remove_one_devices_dir(one_device);
                return 0;
        }
 
-       list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
-
-               if (one_device->bdev) {
-                       disk = one_device->bdev->bd_part;
-                       disk_kobj = &part_to_dev(disk)->kobj;
-                       sysfs_remove_link(fs_devices->devices_kobj,
-                                         disk_kobj->name);
-               }
-               kobject_del(&one_device->devid_kobj);
-               kobject_put(&one_device->devid_kobj);
-
-               wait_for_completion(&one_device->kobj_unregister);
-       }
+       list_for_each_entry(one_device, &fs_devices->devices, dev_list)
+               btrfs_sysfs_remove_one_devices_dir(one_device);
 
        return 0;
 }

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[Anand Jain]

On 7/9/20 7:30 pm, Johannes Thumshirn wrote:
> On 07/09/2020 13:29, Johannes Thumshirn wrote:
>> On 04/09/2020 19:37, Anand Jain wrote:
>>> The following test case leads to null kobject-being-freed error.
>>>
>>>   mount seed /mnt
>>>   add sprout to /mnt
>>>   umount /mnt
>>>   mount sprout to /mnt
>>>   delete seed
>>>
>>>   kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called.
>>>   WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350
>>>   RIP: 0010:kobject_put+0x80/0x350
>>>   ::
>>>   Call Trace:
>>>   btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs]
>>>   btrfs_rm_device.cold+0xa8/0x298 [btrfs]
>>>   btrfs_ioctl+0x206c/0x22a0 [btrfs]
>>>   ksys_ioctl+0xe2/0x140
>>>   __x64_sys_ioctl+0x1e/0x29
>>>   do_syscall_64+0x96/0x150
>>>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>   RIP: 0033:0x7f4047c6288b
>>>   ::
>>>
>>> This is because, at the end of the seed device-delete, we try to remove
>>> the seed's devid sysfs entry. But for the seed devices under the sprout
>>> fs, we don't initialize the devid kobject yet. So add a kobject state
>>> check, which takes care of the Warning.
>>>
>>> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/sysfs.c | 16 ++++++++++------
>>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>> index 190e59152be5..438a367371c4 100644
>>> --- a/fs/btrfs/sysfs.c
>>> +++ b/fs/btrfs/sysfs.c
>>> @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>>>   					  disk_kobj->name);
>>>   		}
>>>   
>>> -		kobject_del(&one_device->devid_kobj);
>>> -		kobject_put(&one_device->devid_kobj);
>>> +		if (one_device->devid_kobj.state_initialized) {
>>> +			kobject_del(&one_device->devid_kobj);
>>> +			kobject_put(&one_device->devid_kobj);
>>>   
>>> -		wait_for_completion(&one_device->kobj_unregister);
>>> +			wait_for_completion(&one_device->kobj_unregister);
>>> +		}
>>>   
>>>   		return 0;
>>>   	}
>>> @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>>>   			sysfs_remove_link(fs_devices->devices_kobj,
>>>   					  disk_kobj->name);
>>>   		}
>>> -		kobject_del(&one_device->devid_kobj);
>>> -		kobject_put(&one_device->devid_kobj);
>>> +		if (one_device->devid_kobj.state_initialized) {
>>> +			kobject_del(&one_device->devid_kobj);
>>> +			kobject_put(&one_device->devid_kobj);
>>>   
>>> -		wait_for_completion(&one_device->kobj_unregister);
>>> +			wait_for_completion(&one_device->kobj_unregister);
>>> +		}
>>>   	}
>>>   
>>>   	return 0;
>>>
>>
>> Hmm this is a lot of duplicated code. It was before as well, this patch only adds
>> to the code duplication.
>>
>> Can't we do sth like this:
>>
> 
> 
> Hit sent too early I'm sorry. I meant add this (untested) as a prep patch:


David and I decided to avoid cleanups in the patch 1/16, and are
pushed into the patch 3-7/16, mainly to make the bug fix patch easy
to backport.

Thanks, Anand


> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 190e59152be5..84114a1ec502 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1151,44 +1151,40 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
>   
>   /* when one_device is NULL, it removes all device links */
>   
> +
> +static void btrfs_sysfs_remove_one_devices_dir(struct btrfs_device *dev)
> +{
> +       if (one_device->bdev) {
> +               struct hd_struct *disk;
> +               struct kobject *disk_kobj;
> +
> +               disk = one_device->bdev->bd_part;
> +               disk_kobj = &part_to_dev(disk)->kobj;
> +               sysfs_remove_link(fs_devices->devices_kobj,
> +                                 disk_kobj->name);
> +       }
> +
> +       kobject_del(&one_device->devid_kobj);
> +       kobject_put(&one_device->devid_kobj);
> +
> +       wait_for_completion(&one_device->kobj_unregister);
> +
> +       return;
> +}
> +
>   int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>                  struct btrfs_device *one_device)
>   {
> -       struct hd_struct *disk;
> -       struct kobject *disk_kobj;
> -
>          if (!fs_devices->devices_kobj)
>                  return -EINVAL;
>   
>          if (one_device) {
> -               if (one_device->bdev) {
> -                       disk = one_device->bdev->bd_part;
> -                       disk_kobj = &part_to_dev(disk)->kobj;
> -                       sysfs_remove_link(fs_devices->devices_kobj,
> -                                         disk_kobj->name);
> -               }
> -
> -               kobject_del(&one_device->devid_kobj);
> -               kobject_put(&one_device->devid_kobj);
> -
> -               wait_for_completion(&one_device->kobj_unregister);
> -
> +               btrfs_sysfs_remove_one_devices_dir(one_device);
>                  return 0;
>          }
>   
> -       list_for_each_entry(one_device, &fs_devices->devices, dev_list) {
> -
> -               if (one_device->bdev) {
> -                       disk = one_device->bdev->bd_part;
> -                       disk_kobj = &part_to_dev(disk)->kobj;
> -                       sysfs_remove_link(fs_devices->devices_kobj,
> -                                         disk_kobj->name);
> -               }
> -               kobject_del(&one_device->devid_kobj);
> -               kobject_put(&one_device->devid_kobj);
> -
> -               wait_for_completion(&one_device->kobj_unregister);
> -       }
> +       list_for_each_entry(one_device, &fs_devices->devices, dev_list)
> +               btrfs_sysfs_remove_one_devices_dir(one_device);
>   
>          return 0;
>   }
> 

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[Johannes Thumshirn]

On 07/09/2020 14:00, Anand Jain wrote:
> David and I decided to avoid cleanups in the patch 1/16, and are
> pushed into the patch 3-7/16, mainly to make the bug fix patch easy
> to backport.

Yep I know, but one prep patch in front shoukld be ok even, when you 
need to backport something to stable. I think 4/16 can go in front of
the series and be backported as well. Sorry that I didn't scan the whole
series before replying to the 1st patch.

Byte,
	Johannes

Re: [PATCH 04/16] btrfs: add btrfs_sysfs_remove_device helper

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[David Sterba]

On Mon, Sep 07, 2020 at 03:09:20PM +0000, Johannes Thumshirn wrote:
> On 07/09/2020 14:00, Anand Jain wrote:
> > David and I decided to avoid cleanups in the patch 1/16, and are
> > pushed into the patch 3-7/16, mainly to make the bug fix patch easy
> > to backport.
> 
> Yep I know, but one prep patch in front shoukld be ok even, when you 
> need to backport something to stable. I think 4/16 can go in front of
> the series and be backported as well. Sorry that I didn't scan the whole
> series before replying to the 1st patch.

There is some duplication but the bugfix is clear and minimal, just
adding the condition around some code which I'd prefer for a stable
patch. The prep or cleanup creates a dependency and this needs to be
tracked in the real fix either by subject or in Fixes: and commit id.
When the fix can be in one patch it's all easier.

Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete

[David Sterba]

On Sat, Sep 05, 2020 at 01:34:21AM +0800, Anand Jain wrote:
> The following test case leads to null kobject-being-freed error.
> 
>  mount seed /mnt
>  add sprout to /mnt
>  umount /mnt
>  mount sprout to /mnt
>  delete seed
> 
>  kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called.
>  WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350
>  RIP: 0010:kobject_put+0x80/0x350
>  ::
>  Call Trace:
>  btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs]
>  btrfs_rm_device.cold+0xa8/0x298 [btrfs]
>  btrfs_ioctl+0x206c/0x22a0 [btrfs]
>  ksys_ioctl+0xe2/0x140
>  __x64_sys_ioctl+0x1e/0x29
>  do_syscall_64+0x96/0x150
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>  RIP: 0033:0x7f4047c6288b
>  ::
> 
> This is because, at the end of the seed device-delete, we try to remove
> the seed's devid sysfs entry. But for the seed devices under the sprout
> fs, we don't initialize the devid kobject yet. So add a kobject state
> check, which takes care of the Warning.
> 
> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes

Please note that the correct format of the Fixes: tag is

Fixes: 668e48af7a94 ("btrfs: sysfs, add devid/dev_state kobject and device attributes")

> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Added to misc-next, thanks.

Re: [PATCH 13/16] btrfs: cleanup btrfs_remove_chunk

[David Sterba]

On Sat, Sep 05, 2020 at 01:34:33AM +0800, Anand Jain wrote:
> In the function btrfs_remove_chunk() remove the local variable
> %fs_devices, instead use the assigned pointer directly.

Local variable that caches some pointer is ok if it's used multiple
time, this patch does the opposite.

Re: [PATCH 13/16] btrfs: cleanup btrfs_remove_chunk

[Anand Jain]

On 9/9/20 6:50 pm, David Sterba wrote:
> On Sat, Sep 05, 2020 at 01:34:33AM +0800, Anand Jain wrote:
>> In the function btrfs_remove_chunk() remove the local variable
>> %fs_devices, instead use the assigned pointer directly.
> 
> Local variable that caches some pointer is ok if it's used multiple
> time, this patch does the opposite.
> 

Oh. By this, it was easy to find and confirm that the device_list_mutex
is of the main device fs_info->fs_deices. I should have mentioned that
in the changelog.

Thanks, Anand

Re: [PATCH v3 0/16] btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups

[David Sterba]

On Sat, Sep 05, 2020 at 01:34:20AM +0800, Anand Jain wrote:
> v3:
> Makes bug fixing patches suitable for the backports. They are patch 1-2.
> patch 1 fix the put of kobject null issue, fixed by checking the
>         state_initalized.
> patch 2 fixes the replacement of seed device in a sprout filesystem.

These go to misc-next,

> The rest of the patches remains the same, except for a conflict fix in patch 4.
> 
> The patch set has passed xfstests -g volume and seed
> The new patch (seed delete testing) btrfs/219 has been modified to suit
> the older kernels. Which is also attached to this thread.

and the rest will follow soon. I changed some subjects and updated some
changelogs that I found not clear enough but the whole series is
straightforward so no need for resends. Thanks.

Re: [PATCH v2 1/2] btrfs: add a test case for btrfs seed device delete

[Filipe Manana]

On Sat, Sep 5, 2020 at 12:25 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> This is a regression test for the issue fixed by the kernel patch
>    btrfs: fix put of uninitialized kobject after seed device delete

Now that the patch is in Linus' tree, we could have the commit id as well.
Just a few comments below.

>
> In this test case, we verify the seed device delete on a sprouted
> filesystem.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2 drop the sysfs layout check as it breaks the test-case backward
> compatibility.
>
>  tests/btrfs/219     | 83 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/219.out | 15 ++++++++
>  tests/btrfs/group   |  1 +
>  3 files changed, 99 insertions(+)
>  create mode 100755 tests/btrfs/219
>  create mode 100644 tests/btrfs/219.out
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> new file mode 100755
> index 000000000000..86f2a6991bd7
> --- /dev/null
> +++ b/tests/btrfs/219
> @@ -0,0 +1,83 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Oracle. All Rights Reserved.
> +#
> +# FS QA Test 219
> +#
> +# Test for seed device-delete on a sprouted FS.
> +# Requires kernel patch
> +#    btrfs: fix put of uninitialized kobject after seed device delete
> +#
> +# Steps:
> +#  Create a seed FS. Add a RW device to make it sprout FS and then delete
> +#  the seed device.
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic

s/generic/btrfs

> +_supported_os Linux

This should go away, _supported_os is gone now.

> +_require_test
> +_require_scratch_dev_pool 2
> +
> +_scratch_dev_pool_get 2
> +
> +seed=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
> +sprout=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')

$AWK_PROG should be used instead.

> +
> +_mkfs_dev $seed
> +_mount $seed $SCRATCH_MNT
> +
> +$XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null

Why the direct IO write? Why not buffered IO?
I just tried the test, and it passes too with a buffered write (no -d).
If there's any reason for using direct IO, it should be mentioned in a
comment, and _require_odirect added at the top.

> +_scratch_unmount
> +$BTRFS_TUNE_PROG -S 1 $seed
> +
> +# Mount the seed device and add the rw device
> +_mount -o ro $seed $SCRATCH_MNT
> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
> +_scratch_unmount
> +
> +# Now remount
> +_mount $sprout $SCRATCH_MNT
> +$XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null

Same comment here regarding the use of direct IO.

> +
> +echo --- before delete ----
> +od -x $SCRATCH_MNT/foo
> +od -x $SCRATCH_MNT/bar
> +
> +$BTRFS_UTIL_PROG device delete $seed $SCRATCH_MNT
> +_scratch_unmount
> +_btrfs_forget_or_module_reload
> +_mount $sprout $SCRATCH_MNT
> +
> +echo --- after delete ----
> +od -x $SCRATCH_MNT/foo
> +od -x $SCRATCH_MNT/bar
> +
> +_scratch_dev_pool_put
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/219.out b/tests/btrfs/219.out
> new file mode 100644
> index 000000000000..d39e0d8ffafd
> --- /dev/null
> +++ b/tests/btrfs/219.out
> @@ -0,0 +1,15 @@
> +QA output created by 219
> +--- before delete ----
> +0000000 abab abab abab abab abab abab abab abab
> +*
> +4000000
> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> +*
> +4000000
> +--- after delete ----
> +0000000 abab abab abab abab abab abab abab abab
> +*
> +4000000
> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> +*
> +4000000
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 3295856d0c8c..3633fa66abe4 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -221,3 +221,4 @@
>  216 auto quick seed
>  217 auto quick trim dangerous
>  218 auto quick volume
> +219 auto quick volume seed

New tests were added in the meanwhile.
For the next version don't forget to renumber the test to 224.

Other than those minor comments, it looks fine and it works.

Thanks.

> --
> 2.25.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/2] btrfs/163: replace sprout instead of seed

[Filipe Manana]

On Sat, Sep 5, 2020 at 12:25 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> Make this test case inline with the kernel patch [1] changes
> [1] btrfs: fix replace of seed device

Same comment as in the previous patch. Now that this is in Linus'
tree, it would be good to mention the commit id too.
>
> So use the sprout device as the replace target instead of the seed device.
> This change is compatible with the older kernels.
>
> While at this, this patch also fixes a typo fix as well.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/163     | 21 ++++++++++++++++-----
>  tests/btrfs/163.out |  5 ++++-
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/tests/btrfs/163 b/tests/btrfs/163
> index 24c725afb6b9..354d88502d47 100755
> --- a/tests/btrfs/163
> +++ b/tests/btrfs/163
> @@ -4,11 +4,15 @@
>  #
>  # FS QA Test 163
>  #
> -# Test case to verify that a seed device can be replaced
> +# Test case to verify that a sprouted device can be replaced
>  #  Create a seed device
>  #  Create a sprout device
>  #  Remount RW
> -#  Run device replace on the seed device
> +#  Run device replace on the sprout device
> +#
> +# Depends on the kernel patch
> +#   btrfs: fail replace of seed device
> +
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
>  echo "QA output created by $seq"
> @@ -39,6 +43,7 @@ _supported_fs btrfs
>  _supported_os Linux
>  _require_command "$BTRFS_TUNE_PROG" btrfstune
>  _require_scratch_dev_pool 3
> +_require_btrfs_forget_or_module_loadable
>
>  _scratch_dev_pool_get 3
>
> @@ -52,7 +57,7 @@ create_seed()
>         run_check _mount $dev_seed $SCRATCH_MNT
>         $XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 4M" $SCRATCH_MNT/foobar >\
>                 /dev/null
> -       echo -- gloden --
> +       echo -- golden --
>         od -x $SCRATCH_MNT/foobar
>         _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>         _scratch_unmount
> @@ -64,22 +69,28 @@ add_sprout()
>  {
>         _run_btrfs_util_prog device add -f $dev_sprout $SCRATCH_MNT
>         _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
> +       _mount -o remount,rw $dev_sprout $SCRATCH_MNT
> +       $XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 4M" $SCRATCH_MNT/foobar2 >\
> +               /dev/null

Same comment as for the other patch.
Why the direct IO? The test passes with buffered IO as well.
If there's a reason for direct IO, it should be mentioned in a comment
and _require_odirect added above.

>  }
>
>  replace_seed()
>  {
> -       _run_btrfs_util_prog replace start -fB $dev_seed $dev_replace_tgt $SCRATCH_MNT
> +       _run_btrfs_util_prog replace start -fB $dev_sprout $dev_replace_tgt $SCRATCH_MNT

So now the function should be renamed from replace_seed() to
replace_sprout() as well. Shouldn't it?

Other than that, it looks good and it works as expected.

Thanks.

>         _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>         _scratch_unmount
> -       run_check _mount $dev_replace_tgt $SCRATCH_MNT
> +       _btrfs_forget_or_module_reload
> +       run_check _mount -o device=$dev_seed $dev_replace_tgt $SCRATCH_MNT
>         echo -- sprout --
>         od -x $SCRATCH_MNT/foobar
> +       od -x $SCRATCH_MNT/foobar2
>         _scratch_unmount
>
>  }
>
>  seed_is_mountable()
>  {
> +       _btrfs_forget_or_module_reload
>         run_check _mount $dev_seed $SCRATCH_MNT
>         _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>         _scratch_unmount
> diff --git a/tests/btrfs/163.out b/tests/btrfs/163.out
> index 91f6f5b6f48a..351ef7b040b2 100644
> --- a/tests/btrfs/163.out
> +++ b/tests/btrfs/163.out
> @@ -1,5 +1,5 @@
>  QA output created by 163
> --- gloden --
> +-- golden --
>  0000000 abab abab abab abab abab abab abab abab
>  *
>  20000000
> @@ -7,3 +7,6 @@ QA output created by 163
>  0000000 abab abab abab abab abab abab abab abab
>  *
>  20000000
> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> +*
> +20000000
> --
> 2.25.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2 1/2] btrfs: add a test case for btrfs seed device delete

[Anand Jain]

On 15/10/20 11:45 pm, Filipe Manana wrote:
> On Sat, Sep 5, 2020 at 12:25 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> This is a regression test for the issue fixed by the kernel patch
>>     btrfs: fix put of uninitialized kobject after seed device delete
> 
> Now that the patch is in Linus' tree, we could have the commit id as well.
> Just a few comments below.
> 
>>
>> In this test case, we verify the seed device delete on a sprouted
>> filesystem.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2 drop the sysfs layout check as it breaks the test-case backward
>> compatibility.
>>
>>   tests/btrfs/219     | 83 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/219.out | 15 ++++++++
>>   tests/btrfs/group   |  1 +
>>   3 files changed, 99 insertions(+)
>>   create mode 100755 tests/btrfs/219
>>   create mode 100644 tests/btrfs/219.out
>>
>> diff --git a/tests/btrfs/219 b/tests/btrfs/219
>> new file mode 100755
>> index 000000000000..86f2a6991bd7
>> --- /dev/null
>> +++ b/tests/btrfs/219
>> @@ -0,0 +1,83 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2020 Oracle. All Rights Reserved.
>> +#
>> +# FS QA Test 219
>> +#
>> +# Test for seed device-delete on a sprouted FS.
>> +# Requires kernel patch
>> +#    btrfs: fix put of uninitialized kobject after seed device delete
>> +#
>> +# Steps:
>> +#  Create a seed FS. Add a RW device to make it sprout FS and then delete
>> +#  the seed device.
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1       # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +       cd /
>> +       rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic
> 
> s/generic/btrfs
> 
>> +_supported_os Linux
> 
> This should go away, _supported_os is gone now.
> 
>> +_require_test
>> +_require_scratch_dev_pool 2
>> +
>> +_scratch_dev_pool_get 2
>> +
>> +seed=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
>> +sprout=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
> 
> $AWK_PROG should be used instead.
> 
>> +
>> +_mkfs_dev $seed
>> +_mount $seed $SCRATCH_MNT
>> +
>> +$XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
> 
> Why the direct IO write? Why not buffered IO?
> I just tried the test, and it passes too with a buffered write (no -d).
> If there's any reason for using direct IO, it should be mentioned in a
> comment, and _require_odirect added at the top.
> 

  Ah. No there isn't any reason for using direct IO. I will take it out.



>> +_scratch_unmount
>> +$BTRFS_TUNE_PROG -S 1 $seed
>> +
>> +# Mount the seed device and add the rw device
>> +_mount -o ro $seed $SCRATCH_MNT
>> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
>> +_scratch_unmount
>> +
>> +# Now remount
>> +_mount $sprout $SCRATCH_MNT
>> +$XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
> 
> Same comment here regarding the use of direct IO.
> 
>> +
>> +echo --- before delete ----
>> +od -x $SCRATCH_MNT/foo
>> +od -x $SCRATCH_MNT/bar
>> +
>> +$BTRFS_UTIL_PROG device delete $seed $SCRATCH_MNT
>> +_scratch_unmount
>> +_btrfs_forget_or_module_reload
>> +_mount $sprout $SCRATCH_MNT
>> +
>> +echo --- after delete ----
>> +od -x $SCRATCH_MNT/foo
>> +od -x $SCRATCH_MNT/bar
>> +
>> +_scratch_dev_pool_put
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/219.out b/tests/btrfs/219.out
>> new file mode 100644
>> index 000000000000..d39e0d8ffafd
>> --- /dev/null
>> +++ b/tests/btrfs/219.out
>> @@ -0,0 +1,15 @@
>> +QA output created by 219
>> +--- before delete ----
>> +0000000 abab abab abab abab abab abab abab abab
>> +*
>> +4000000
>> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
>> +*
>> +4000000
>> +--- after delete ----
>> +0000000 abab abab abab abab abab abab abab abab
>> +*
>> +4000000
>> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
>> +*
>> +4000000
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index 3295856d0c8c..3633fa66abe4 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -221,3 +221,4 @@
>>   216 auto quick seed
>>   217 auto quick trim dangerous
>>   218 auto quick volume
>> +219 auto quick volume seed
> 
> New tests were added in the meanwhile.
> For the next version don't forget to renumber the test to 224.
> 
> Other than those minor comments, it looks fine and it works.
> 

  Rest of the comments are accepted. I am sending v3.

Thanks, Anand

> Thanks.
> 
>> --
>> 2.25.1
>>
> 
> 

Re: [PATCH 2/2] btrfs/163: replace sprout instead of seed

[Anand Jain]

On 15/10/20 11:49 pm, Filipe Manana wrote:
> On Sat, Sep 5, 2020 at 12:25 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Make this test case inline with the kernel patch [1] changes
>> [1] btrfs: fix replace of seed device
> 
> Same comment as in the previous patch. Now that this is in Linus'
> tree, it would be good to mention the commit id too.

  Added in v3.

>>
>> So use the sprout device as the replace target instead of the seed device..
>> This change is compatible with the older kernels.
>>
>> While at this, this patch also fixes a typo fix as well.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   tests/btrfs/163     | 21 ++++++++++++++++-----
>>   tests/btrfs/163.out |  5 ++++-
>>   2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/btrfs/163 b/tests/btrfs/163
>> index 24c725afb6b9..354d88502d47 100755
>> --- a/tests/btrfs/163
>> +++ b/tests/btrfs/163
>> @@ -4,11 +4,15 @@
>>   #
>>   # FS QA Test 163
>>   #
>> -# Test case to verify that a seed device can be replaced
>> +# Test case to verify that a sprouted device can be replaced
>>   #  Create a seed device
>>   #  Create a sprout device
>>   #  Remount RW
>> -#  Run device replace on the seed device
>> +#  Run device replace on the sprout device
>> +#
>> +# Depends on the kernel patch
>> +#   btrfs: fail replace of seed device
>> +
>>   seq=`basename $0`
>>   seqres=$RESULT_DIR/$seq
>>   echo "QA output created by $seq"
>> @@ -39,6 +43,7 @@ _supported_fs btrfs
>>   _supported_os Linux
>>   _require_command "$BTRFS_TUNE_PROG" btrfstune
>>   _require_scratch_dev_pool 3
>> +_require_btrfs_forget_or_module_loadable
>>
>>   _scratch_dev_pool_get 3
>>
>> @@ -52,7 +57,7 @@ create_seed()
>>          run_check _mount $dev_seed $SCRATCH_MNT
>>          $XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 4M" $SCRATCH_MNT/foobar >\
>>                  /dev/null
>> -       echo -- gloden --
>> +       echo -- golden --
>>          od -x $SCRATCH_MNT/foobar
>>          _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>>          _scratch_unmount
>> @@ -64,22 +69,28 @@ add_sprout()
>>   {
>>          _run_btrfs_util_prog device add -f $dev_sprout $SCRATCH_MNT
>>          _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>> +       _mount -o remount,rw $dev_sprout $SCRATCH_MNT
>> +       $XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 4M" $SCRATCH_MNT/foobar2 >\
>> +               /dev/null
> 
> Same comment as for the other patch.
> Why the direct IO? The test passes with buffered IO as well.
> If there's a reason for direct IO, it should be mentioned in a comment
> and _require_odirect added above.
> 

  Yes. No need of the directio here. Thanks.

>>   }
>>
>>   replace_seed()
>>   {
>> -       _run_btrfs_util_prog replace start -fB $dev_seed $dev_replace_tgt $SCRATCH_MNT
>> +       _run_btrfs_util_prog replace start -fB $dev_sprout $dev_replace_tgt $SCRATCH_MNT
> 
> So now the function should be renamed from replace_seed() to
> replace_sprout() as well. Shouldn't it?
> 

  Good catch will fix the function name.

> Other than that, it looks good and it works as expected.
> 

  All comments are fixed in v3.


Thanks, Anand

> Thanks.
> 
>>          _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>>          _scratch_unmount
>> -       run_check _mount $dev_replace_tgt $SCRATCH_MNT
>> +       _btrfs_forget_or_module_reload
>> +       run_check _mount -o device=$dev_seed $dev_replace_tgt $SCRATCH_MNT
>>          echo -- sprout --
>>          od -x $SCRATCH_MNT/foobar
>> +       od -x $SCRATCH_MNT/foobar2
>>          _scratch_unmount
>>
>>   }
>>
>>   seed_is_mountable()
>>   {
>> +       _btrfs_forget_or_module_reload
>>          run_check _mount $dev_seed $SCRATCH_MNT
>>          _run_btrfs_util_prog filesystem show -m $SCRATCH_MNT
>>          _scratch_unmount
>> diff --git a/tests/btrfs/163.out b/tests/btrfs/163.out
>> index 91f6f5b6f48a..351ef7b040b2 100644
>> --- a/tests/btrfs/163.out
>> +++ b/tests/btrfs/163.out
>> @@ -1,5 +1,5 @@
>>   QA output created by 163
>> --- gloden --
>> +-- golden --
>>   0000000 abab abab abab abab abab abab abab abab
>>   *
>>   20000000
>> @@ -7,3 +7,6 @@ QA output created by 163
>>   0000000 abab abab abab abab abab abab abab abab
>>   *
>>   20000000
>> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
>> +*
>> +20000000
>> --
>> 2.25.1
>>
> 
>