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

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

[Anand Jain]

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 and

Anand Jain (15):
  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
  btrfs: fix replace of seed device

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

-- 
2.25.1

[PATCH 01/15] 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>
---
 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 190e59152be5..3381a91d7deb 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1271,44 +1271,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_devices->fs_info->fs_devices->devices_kobj;
+	devinfo_kobj = device->fs_devices->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 02/15] 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>
---
 fs/btrfs/sysfs.c | 54 ++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 3381a91d7deb..241ec0ad0379 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1149,46 +1149,42 @@ 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;
+	/*
+	 * 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) {
-		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 (device->bdev) {
+		disk = device->bdev->bd_part;
+		disk_kobj = &part_to_dev(disk)->kobj;
+		sysfs_remove_link(devices_kobj, disk_kobj->name);
+	}
 
-		kobject_del(&one_device->devid_kobj);
-		kobject_put(&one_device->devid_kobj);
+	kobject_del(&device->devid_kobj);
+	kobject_put(&device->devid_kobj);
 
-		wait_for_completion(&one_device->kobj_unregister);
+	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);
-		}
-		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 03/15] 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>
---
 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 241ec0ad0379..7448caf12c53 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1175,18 +1175,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 04/15] 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>
---
 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 580e60fe07d0..979b40754cb4 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 7448caf12c53..69a58666f351 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1265,7 +1265,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;
@@ -1315,16 +1315,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;
 	}
@@ -1419,7 +1416,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 3f8bd1af29eb..8952f7031f4b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2599,7 +2599,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 				    orig_super_num_devices + 1);
 
 	/* add sysfs device entry */
-	btrfs_sysfs_add_devices_dir(fs_devices, device);
+	btrfs_sysfs_add_device(device);
 
 	/*
 	 * we've got more storage, clear any full flags on the space
-- 
2.25.1

[PATCH 05/15] 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>
---
 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 979b40754cb4..a7b1ad4e5706 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 69a58666f351..9853b9acd4bd 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;
@@ -1174,19 +1182,6 @@ static void btrfs_sysfs_remove_device(struct btrfs_device *device)
 	wait_for_completion(&device->kobj_unregister);
 }
 
-/* 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)
@@ -1422,7 +1417,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 8952f7031f4b..9921b43ef839 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2040,7 +2040,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;
@@ -2144,7 +2144,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;
@@ -2245,7 +2245,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 06/15] btrfs: initialize sysfs devid and device link for seed device

[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 this patch initializes
the seed device devid kobject and the device link in the sysfs. This takes
care of the Warning.

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 9853b9acd4bd..98ce955a0879 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)
@@ -1314,6 +1320,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);
@@ -1321,6 +1328,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 07/15] 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>
---
 fs/btrfs/sysfs.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 98ce955a0879..a5a73b21d4af 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1182,10 +1182,12 @@ void btrfs_sysfs_remove_device(struct btrfs_device *device)
 		sysfs_remove_link(devices_kobj, disk_kobj->name);
 	}
 
-	kobject_del(&device->devid_kobj);
-	kobject_put(&device->devid_kobj);
+	if (device->devid_kobj.state_initialized) {
+		kobject_del(&device->devid_kobj);
+		kobject_put(&device->devid_kobj);
+		wait_for_completion(&device->kobj_unregister);
+	}
 
-	wait_for_completion(&device->kobj_unregister);
 }
 
 static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
@@ -1324,19 +1326,21 @@ 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 08/15] 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 09/15] 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 9921b43ef839..7639a048c6cf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7184,16 +7184,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 10/15] 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 a7b1ad4e5706..aea1c782c009 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 11/15] 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 7639a048c6cf..3e7a7d94a211 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 12/15] 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 3e7a7d94a211..c8b0d9eb4468 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1961,16 +1961,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 13/15] 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 c8b0d9eb4468..dc81646b13c0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6741,19 +6741,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 14/15] 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 aea1c782c009..656d8ba642af 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 15/15] 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:
Make 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.
As in the below changes.

Another possible fix:
If we want to keep the ability to replace for seed-device, then we could
update the fsid of the replace-target blocks. And after replacement, you
have seed device but with sprout fsid. But then I don't know what is the
point and if there is any such use case.

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 656d8ba642af..02b7b3edf9a3 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -225,7 +225,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

Re: [PATCH 01/15] btrfs: add btrfs_sysfs_add_device helper

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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>
> ---
>  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 190e59152be5..3381a91d7deb 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1271,44 +1271,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;

Whitespace damage

>  
> -	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_devices->fs_info->fs_devices->devices_kobj;
> +	devinfo_kobj = device->fs_devices->fs_info->fs_devices->devinfo_kobj;

This function and its callers are called after the fs_info of devices is
initialized so can't you simply do 'device->fs_info->fs_devices->'...
reduces a level of pointer chasing.

> +	ASSERT(devices_kobj);
> +	ASSERT(devinfo_kobj);
<snip>


>  
> -	return error;
> +int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
> +				struct btrfs_device *one_device)
> +{
> +	int ret;

That variable can be defined inside the list_for_each-entry as it's
being used only in that context.
> +
> +	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)
> 

Re: [PATCH 03/15] btrfs: btrfs_sysfs_remove_devices_dir drop return value

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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>

Re: [PATCH 02/15] btrfs: add btrfs_sysfs_remove_device helper

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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>

LGTM, one nit below though:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/sysfs.c | 54 ++++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 3381a91d7deb..241ec0ad0379 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1149,46 +1149,42 @@ 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;
> +	/*
> +	 * 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;

nit: device->fs_info->fs_devices

> +	ASSERT(devices_kobj);
>  
> -	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);
> -		}
> +	if (device->bdev) {
> +		disk = device->bdev->bd_part;
> +		disk_kobj = &part_to_dev(disk)->kobj;
> +		sysfs_remove_link(devices_kobj, disk_kobj->name);
> +	}
>  
> -		kobject_del(&one_device->devid_kobj);
> -		kobject_put(&one_device->devid_kobj);
> +	kobject_del(&device->devid_kobj);
> +	kobject_put(&device->devid_kobj);
>  
> -		wait_for_completion(&one_device->kobj_unregister);
> +	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);
> -		}
> -		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;
>  }
> 

Re: [PATCH 04/15] btrfs: refactor btrfs_sysfs_add_devices_dir

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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>

Reviewd-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 05/15] btrfs: refactor btrfs_sysfs_remove_devices_dir

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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(-)
> 

<snip>

> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8952f7031f4b..9921b43ef839 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2040,7 +2040,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)

nit: Though I agree with this change it's unrelated.

>  {
>  	struct btrfs_device *device;
>  	struct btrfs_fs_devices *cur_devices;

<snip>

Re: [PATCH 06/15] btrfs: initialize sysfs devid and device link for seed device

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., 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

This patch warrants an fstests alongside it!

> 
>  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 this patch initializes
> the seed device devid kobject and the device link in the sysfs. This takes
> care of the Warning.
> 
> 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 9853b9acd4bd..98ce955a0879 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)
> @@ -1314,6 +1320,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);
> @@ -1321,6 +1328,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;
>  }
>  
> 

Re: [PATCH 07/15] btrfs: handle fail path for btrfs_sysfs_add_fs_devices

[Nikolay Borisov]

On 3.09.20 г. 3:57 ч., Anand Jain wrote:
> 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>

I though calling sysfs_remove_link for an uninitialized link could be
problematic but it's not - it will simply return -ENOENT. So

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/sysfs.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 98ce955a0879..a5a73b21d4af 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1182,10 +1182,12 @@ void btrfs_sysfs_remove_device(struct btrfs_device *device)
>  		sysfs_remove_link(devices_kobj, disk_kobj->name);
>  	}
>  
> -	kobject_del(&device->devid_kobj);
> -	kobject_put(&device->devid_kobj);
> +	if (device->devid_kobj.state_initialized) {
> +		kobject_del(&device->devid_kobj);
> +		kobject_put(&device->devid_kobj);
> +		wait_for_completion(&device->kobj_unregister);
> +	}
>  
> -	wait_for_completion(&device->kobj_unregister);
>  }
>  
>  static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
> @@ -1324,19 +1326,21 @@ 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)
> 

Re: [PATCH 01/15] btrfs: add btrfs_sysfs_add_device helper

[Anand Jain]

On 3/9/20 4:40 pm, Nikolay Borisov wrote:
> 
> 
> On 3.09.20 г. 3:57 ч., Anand Jain wrote:
>> 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>
>> ---
>>   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 190e59152be5..3381a91d7deb 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1271,44 +1271,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;
> 
> Whitespace damage

  oops.


> 
>>   
>> -	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_devices->fs_info->fs_devices->devices_kobj;
>> +	devinfo_kobj = device->fs_devices->fs_info->fs_devices->devinfo_kobj;
> 
> This function and its callers are called after the fs_info of devices is
> initialized so can't you simply do 'device->fs_info->fs_devices->'...
> reduces a level of pointer chasing.
> 

  Oh. Right. Will fix.

>> +	ASSERT(devices_kobj);
>> +	ASSERT(devinfo_kobj);
> <snip>
> 
> 
>>   
>> -	return error;
>> +int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
>> +				struct btrfs_device *one_device)
>> +{
>> +	int ret;
> 
> That variable can be defined inside the list_for_each-entry as it's
> being used only in that context.

ok.

Thanks!
Anand

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

Re: [PATCH 02/15] btrfs: add btrfs_sysfs_remove_device helper

[Anand Jain]

On 3/9/20 4:44 pm, Nikolay Borisov wrote:
> 
> 
> On 3.09.20 г. 3:57 ч., Anand Jain wrote:
>> 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>
> 
> LGTM, one nit below though:
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
>> ---
>>   fs/btrfs/sysfs.c | 54 ++++++++++++++++++++++--------------------------
>>   1 file changed, 25 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 3381a91d7deb..241ec0ad0379 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1149,46 +1149,42 @@ 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;
>> +	/*
>> +	 * 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;
> 
> nit: device->fs_info->fs_devices

  Sorry what are you suggesting here? I didn't understand.

Thanks, Anand



> 
>> +	ASSERT(devices_kobj);
>>   
>> -	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);
>> -		}
>> +	if (device->bdev) {
>> +		disk = device->bdev->bd_part;
>> +		disk_kobj = &part_to_dev(disk)->kobj;
>> +		sysfs_remove_link(devices_kobj, disk_kobj->name);
>> +	}
>>   
>> -		kobject_del(&one_device->devid_kobj);
>> -		kobject_put(&one_device->devid_kobj);
>> +	kobject_del(&device->devid_kobj);
>> +	kobject_put(&device->devid_kobj);
>>   
>> -		wait_for_completion(&one_device->kobj_unregister);
>> +	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);
>> -		}
>> -		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;
>>   }
>>

Re: [PATCH 02/15] btrfs: add btrfs_sysfs_remove_device helper

[Nikolay Borisov]

On 3.09.20 г. 13:03 ч., Anand Jain wrote:
> 
> 
> On 3/9/20 4:44 pm, Nikolay Borisov wrote:
>>
>>
>> On 3.09.20 г. 3:57 ч., Anand Jain wrote:
>>> 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>
>>
>> LGTM, one nit below though:
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>>> ---
>>>   fs/btrfs/sysfs.c | 54 ++++++++++++++++++++++--------------------------
>>>   1 file changed, 25 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>>> index 3381a91d7deb..241ec0ad0379 100644
>>> --- a/fs/btrfs/sysfs.c
>>> +++ b/fs/btrfs/sysfs.c
>>> @@ -1149,46 +1149,42 @@ 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;
>>> +    /*
>>> +     * 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;
>>
>> nit: device->fs_info->fs_devices
> 
>  Sorry what are you suggesting here? I didn't understand.

Ah, nothing, you already done it the way I suggested.

> 
> Thanks, Anand
> 
> 
> 
>>
>>> +    ASSERT(devices_kobj);
>>>   -    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);
>>> -        }
>>> +    if (device->bdev) {
>>> +        disk = device->bdev->bd_part;
>>> +        disk_kobj = &part_to_dev(disk)->kobj;
>>> +        sysfs_remove_link(devices_kobj, disk_kobj->name);
>>> +    }
>>>   -        kobject_del(&one_device->devid_kobj);
>>> -        kobject_put(&one_device->devid_kobj);
>>> +    kobject_del(&device->devid_kobj);
>>> +    kobject_put(&device->devid_kobj);
>>>   -        wait_for_completion(&one_device->kobj_unregister);
>>> +    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);
>>> -        }
>>> -        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;
>>>   }
>>>
> 

Re: [PATCH 01/15] btrfs: add btrfs_sysfs_add_device helper

[Anand Jain]

>> -	return error;
>> +int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
>> +				struct btrfs_device *one_device)
>> +{
>> +	int ret;
> 
> That variable can be defined inside the list_for_each-entry as it's
> being used only in that context.

  Patch 6 uses ret in seed list loop. So its ok.

Thanks, Anand

[PATCH v3 1/15] 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>
---
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 190e59152be5..5cfa13957e2a 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1271,44 +1271,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

Re: [PATCH 06/15] btrfs: initialize sysfs devid and device link for seed device

[Anand Jain]

On 3/9/20 5:06 pm, Nikolay Borisov wrote:
> 
> 
> On 3.09.20 г. 3:57 ч., 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
> 
> This patch warrants an fstests alongside it!

  I have been using one which I forgot to send. Now done.

Thanks, Anand

[PATCH] fstests: btrfs/161: extend the test case to delete seed

[Anand Jain]

This test case btrfs/161 does the sprout tests, so after sprout
this patch deletes the seed to verify.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/161     | 16 ++++++++++++++--
 tests/btrfs/161.out |  4 ++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/tests/btrfs/161 b/tests/btrfs/161
index dfff8b871fa3..feaa7a4de6c2 100755
--- a/tests/btrfs/161
+++ b/tests/btrfs/161
@@ -5,9 +5,13 @@
 # FS QA Test 161
 #
 # seed sprout functionality test
-#  Create a seed device, mount it and, add a new device to create a
-#  sprout filesystem.
+#  Create seed
+#  Create sprout
+#  Delete seed
 #
+# The null kobject free warning is address by the kernel patch
+#   btrfs: initialize sysfs devid and device link for seed device
+
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
 echo "QA output created by $seq"
@@ -64,11 +68,19 @@ create_sprout()
 	run_check _mount $dev_sprout $SCRATCH_MNT
 	echo -- sprout --
 	od -x $SCRATCH_MNT/foobar
+}
+
+delete_seed()
+{
+	$BTRFS_UTIL_PROG device delete $dev_seed $SCRATCH_MNT
+	echo -- seed removed --
+	od -x $SCRATCH_MNT/foobar
 	_scratch_unmount
 }
 
 create_seed
 create_sprout
+delete_seed
 
 _scratch_dev_pool_put
 
diff --git a/tests/btrfs/161.out b/tests/btrfs/161.out
index 363b8217f45c..0fe9c128dcde 100644
--- a/tests/btrfs/161.out
+++ b/tests/btrfs/161.out
@@ -7,3 +7,7 @@ QA output created by 161
 0000000 abab abab abab abab abab abab abab abab
 *
 1000000
+-- seed removed --
+0000000 abab abab abab abab abab abab abab abab
+*
+1000000
-- 
2.25.1

Re: [PATCH v3 1/15] btrfs: add btrfs_sysfs_add_device helper

[Josef Bacik]

On 9/3/20 6:46 AM, Anand Jain wrote:
> 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>

Thanks,

Josef

Re: [PATCH 02/15] btrfs: add btrfs_sysfs_remove_device helper

[Josef Bacik]

On 9/2/20 8:57 PM, Anand Jain wrote:
> 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: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 03/15] btrfs: btrfs_sysfs_remove_devices_dir drop return value

[Josef Bacik]

On 9/2/20 8:57 PM, Anand Jain wrote:
> btrfs_sysfs_remove_devices_dir() return value is unused declare it as
> void.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 04/15] btrfs: refactor btrfs_sysfs_add_devices_dir

[Josef Bacik]

On 9/2/20 8:57 PM, Anand Jain wrote:
> 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>

This failed to apply cleanly, so I couldn't make sure the rest of the series 
built or review the rest of them.  Thanks,

Josef

Re: [PATCH] fstests: btrfs/161: extend the test case to delete seed

[Josef Bacik]

On 9/3/20 7:35 AM, Anand Jain wrote:
> This test case btrfs/161 does the sprout tests, so after sprout
> this patch deletes the seed to verify.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

I'm opposed to changing tests that are currently passing to making them start 
failing if you don't have the fix.  This makes the tester go try to figure out 
wtf a test that was passing before is now suddenly failing.  Instead add a new 
test to test the new functionality, so it's clear to the tester that we're 
testing something new.  Thanks,

Josef

[PATCH] 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: initialize sysfs devid and device link for seed device

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

This patch also adds a filter to filter the scratch pool devices
without the device path.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/filter       | 13 +++++++
 tests/btrfs/219     | 94 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/219.out | 39 +++++++++++++++++++
 tests/btrfs/group   |  1 +
 4 files changed, 147 insertions(+)
 create mode 100755 tests/btrfs/219
 create mode 100644 tests/btrfs/219.out

diff --git a/common/filter b/common/filter
index 2477f3860151..7c5f288692e7 100644
--- a/common/filter
+++ b/common/filter
@@ -304,6 +304,19 @@ _filter_testdir_and_scratch()
 	fi
 }
 
+_filter_scratch_pool_short()
+{
+	SHORT=""
+
+	for DEV in $SCRATCH_DEV_POOL
+	do
+		SHORT="$SHORT $(echo $DEV| rev | awk -F / '{print $1}' | rev)"
+	done
+
+	FILTER_STRING=$(echo $SHORT | sed -e 's/\s\+/\\\|/g')
+	sed -e "s,$FILTER_STRING,SCRATCH_DEV,g"
+}
+
 # Turn any device in the scratch pool into SCRATCH_DEV
 _filter_scratch_pool()
 {
diff --git a/tests/btrfs/219 b/tests/btrfs/219
new file mode 100755
index 000000000000..deb2857af004
--- /dev/null
+++ b/tests/btrfs/219
@@ -0,0 +1,94 @@
+#! /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: initialize sysfs devid and device link for seed device
+#
+# 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
+. ./common/filter.btrfs
+
+# 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 ----
+UUID=$($BTRFS_UTIL_PROG filesystem show -m $SCRATCH_MNT | head -1 | \
+							awk '{print $4}')
+find /sys/fs/btrfs/$UUID/devinfo/ | rev | awk -F /  '{print $1}' | rev
+find /sys/fs/btrfs/$UUID/devices/ | rev | awk -F /  '{print $1}' | rev | \
+						_filter_scratch_pool_short
+echo
+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 ----
+find /sys/fs/btrfs/$UUID/devinfo/ | rev | awk -F /  '{print $1}' | rev
+find /sys/fs/btrfs/$UUID/devices/ | rev | awk -F /  '{print $1}' | rev | \
+						_filter_scratch_pool_short
+echo
+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..6b054ccce95d
--- /dev/null
+++ b/tests/btrfs/219.out
@@ -0,0 +1,39 @@
+QA output created by 219
+--- before delete ----
+
+1
+in_fs_metadata
+replace_target
+writeable
+missing
+2
+in_fs_metadata
+replace_target
+writeable
+missing
+
+SCRATCH_DEV
+SCRATCH_DEV
+
+0000000 abab abab abab abab abab abab abab abab
+*
+4000000
+0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
+*
+4000000
+--- after delete ----
+
+2
+in_fs_metadata
+replace_target
+writeable
+missing
+
+SCRATCH_DEV
+
+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 v3 07/15] 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>
---
v3: fix the goto fail
 fs/btrfs/sysfs.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 56eca63360ad..be5b773225e8 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1182,10 +1182,12 @@ void btrfs_sysfs_remove_device(struct btrfs_device *device)
 		sysfs_remove_link(devices_kobj, disk_kobj->name);
 	}
 
-	kobject_del(&device->devid_kobj);
-	kobject_put(&device->devid_kobj);
+	if (device->devid_kobj.state_initialized) {
+		kobject_del(&device->devid_kobj);
+		kobject_put(&device->devid_kobj);
+		wait_for_completion(&device->kobj_unregister);
+	}
 
-	wait_for_completion(&device->kobj_unregister);
 }
 
 static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
@@ -1325,18 +1327,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

Re: [PATCH 06/15] btrfs: initialize sysfs devid and device link for seed device

[David Sterba]

On Thu, Sep 03, 2020 at 08:57:42AM +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 this patch initializes
> the seed device devid kobject and the device link in the sysfs. This takes
> care of the Warning.

So this is the fix but needs 5 more cleanup patches and this makes it
too hard to backport. Is there an isolated patch that can be also
applied to current 5.9 queue.

The test leads to a crash so once the test lands in fstests all testing
machines without the fix will crash, so we need fix first, cleanup
later. There are also the seed list changes to this need to reflect that
somehow.