[PATCH v3 0/4] btrfs: match device by dev_t

[PATCH v3 0/4] btrfs: match device by dev_t

[Anand Jain]

v3: Added patch 3/4 saves and uses dev_t in btrfs_device and
    thus patch 4/4 removes the function device_matched().
    fstests btrfs passed with no new regressions.

v2: Fix 
     sparse: warning: incorrect type in argument 1 (different address spaces)
     For using device->name->str

    Fix Josef suggestion to pass dev_t instead of device-path in the
     patch 2/2.

--- original cover letter -----
Patch 1 is the actual bug fix and should go to stable 5.4 as well.
On 5.4 patch1 conflicts (outside of the changes in the patch),
so not yet marked for the stable.

Patch 2 simplifies calling lookup_bdev() in the device_matched()
by moving the same to the parent function two levels up.

Patch 2 is not merged with 1 because to keep the patch 1 changes local
to a function so that it can be easily backported to 5.4 and 5.10.

We should save the dev_t in struct btrfs_device with that may be
we could clean up a few more things, including fixing the below sparse
warning.

  sparse: sparse: incorrect type in argument 1 (different address spaces)

For using without rcu:

  error = lookup_bdev(device->name->str, &dev_old); 


Anand Jain (4):
  btrfs: harden identification of the stale device
  btrfs: redeclare btrfs_stale_devices arg1 to dev_t
  btrfs: add device major-minor info in the struct btrfs_device
  btrfs: use dev_t to match device in device_matched

 fs/btrfs/dev-replace.c |  3 ++
 fs/btrfs/super.c       |  8 ++++-
 fs/btrfs/volumes.c     | 67 +++++++++++++++++-------------------------
 fs/btrfs/volumes.h     |  3 +-
 4 files changed, 39 insertions(+), 42 deletions(-)

-- 
2.33.1

[PATCH v3 1/4] btrfs: harden identification of the stale device

[Anand Jain]

Identifying and removing the stale device from the fs_uuids list is done
by the function btrfs_free_stale_devices().
btrfs_free_stale_devices() in turn depends on the function
device_path_matched() to check if the device repeats in more than one
btrfs_device structure.

The matching of the device happens by its path, the device path. However,
when dm mapper is in use, the dm device paths are nothing but a link to
the actual block device, which leads to the device_path_matched() failing
to match.

Fix this by matching the dev_t as provided by lookup_bdev() instead of
plain strcmp() the device paths.

Reported-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: -
v2: Fix 
     sparse: warning: incorrect type in argument 1 (different address spaces)
     For using device->name->str

    Fix Josef suggestion to pass dev_t instead of device-path in the
     patch 2/2.

 fs/btrfs/volumes.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4b244acd0cfa..ad9e08c3199c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -535,15 +535,46 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
 	return ret;
 }
 
-static bool device_path_matched(const char *path, struct btrfs_device *device)
+/*
+ * Check if the device in the 'path' matches with the device in the given
+ * struct btrfs_device '*device'.
+ * Returns:
+ *	0	If it is the same device.
+ *	1	If it is not the same device.
+ *	-errno	For error.
+ */
+static int device_matched(struct btrfs_device *device, const char *path)
 {
-	int found;
+	char *device_name;
+	dev_t dev_old;
+	dev_t dev_new;
+	int ret;
+
+	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
+	if (!device_name)
+		return -ENOMEM;
 
 	rcu_read_lock();
-	found = strcmp(rcu_str_deref(device->name), path);
+	ret = sprintf(device_name, "%s", rcu_str_deref(device->name));
 	rcu_read_unlock();
+	if (!ret) {
+		kfree(device_name);
+		return -EINVAL;
+	}
 
-	return found == 0;
+	ret = lookup_bdev(device_name, &dev_old);
+	kfree(device_name);
+	if (ret)
+		return ret;
+
+	ret = lookup_bdev(path, &dev_new);
+	if (ret)
+		return ret;
+
+	if (dev_old == dev_new)
+		return 0;
+
+	return 1;
 }
 
 /*
@@ -578,7 +609,7 @@ static int btrfs_free_stale_devices(const char *path,
 				continue;
 			if (path && !device->name)
 				continue;
-			if (path && !device_path_matched(path, device))
+			if (path && device_matched(device, path) != 0)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-- 
2.33.1

[PATCH v3 2/4] btrfs: redeclare btrfs_stale_devices arg1 to dev_t

[Anand Jain]

After the commit cb57afa39796 ("btrfs: harden identification of the stale
device"), we don't have to match the device path anymore. Instead, we
match the dev_t. So pass in the dev_t instead of the device-path, in the call
chain btrfs_forget_devices()->btrfs_free_stale_devices().

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

 fs/btrfs/super.c   |  8 +++++++-
 fs/btrfs/volumes.c | 45 +++++++++++++++++++++++----------------------
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1ff03fb6c64a..bdf54b2673fe 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2386,6 +2386,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 {
 	struct btrfs_ioctl_vol_args *vol;
 	struct btrfs_device *device = NULL;
+	dev_t devt = 0;
 	int ret = -ENOTTY;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -2405,7 +2406,12 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 		mutex_unlock(&uuid_mutex);
 		break;
 	case BTRFS_IOC_FORGET_DEV:
-		ret = btrfs_forget_devices(vol->name);
+		if (strlen(vol->name)) {
+			ret = lookup_bdev(vol->name, &devt);
+			if (ret)
+				break;
+		}
+		ret = btrfs_forget_devices(devt);
 		break;
 	case BTRFS_IOC_DEVICES_READY:
 		mutex_lock(&uuid_mutex);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ad9e08c3199c..cb43ee5925ad 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -543,11 +543,10 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
  *	1	If it is not the same device.
  *	-errno	For error.
  */
-static int device_matched(struct btrfs_device *device, const char *path)
+static int device_matched(struct btrfs_device *device, dev_t dev_new)
 {
 	char *device_name;
 	dev_t dev_old;
-	dev_t dev_new;
 	int ret;
 
 	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
@@ -567,10 +566,6 @@ static int device_matched(struct btrfs_device *device, const char *path)
 	if (ret)
 		return ret;
 
-	ret = lookup_bdev(path, &dev_new);
-	if (ret)
-		return ret;
-
 	if (dev_old == dev_new)
 		return 0;
 
@@ -580,16 +575,16 @@ static int device_matched(struct btrfs_device *device, const char *path)
 /*
  *  Search and remove all stale (devices which are not mounted) devices.
  *  When both inputs are NULL, it will search and release all stale devices.
- *  path:	Optional. When provided will it release all unmounted devices
- *		matching this path only.
+ *  devt:	Optional. When provided will it release all unmounted devices
+ *		matching this devt only.
  *  skip_dev:	Optional. Will skip this device when searching for the stale
  *		devices.
- *  Return:	0 for success or if @path is NULL.
- * 		-EBUSY if @path is a mounted device.
- * 		-ENOENT if @path does not match any device in the list.
+ *  Return:	0 for success or if @devt is 0.
+ *		-EBUSY if @devt is a mounted device.
+ *		-ENOENT if @devt does not match any device in the list.
  */
-static int btrfs_free_stale_devices(const char *path,
-				     struct btrfs_device *skip_device)
+static int btrfs_free_stale_devices(dev_t devt,
+				    struct btrfs_device *skip_device)
 {
 	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
 	struct btrfs_device *device, *tmp_device;
@@ -597,7 +592,7 @@ static int btrfs_free_stale_devices(const char *path,
 
 	lockdep_assert_held(&uuid_mutex);
 
-	if (path)
+	if (devt)
 		ret = -ENOENT;
 
 	list_for_each_entry_safe(fs_devices, tmp_fs_devices, &fs_uuids, fs_list) {
@@ -607,13 +602,13 @@ static int btrfs_free_stale_devices(const char *path,
 					 &fs_devices->devices, dev_list) {
 			if (skip_device && skip_device == device)
 				continue;
-			if (path && !device->name)
+			if (devt && !device->name)
 				continue;
-			if (path && device_matched(device, path) != 0)
+			if (devt && device_matched(device, devt) != 0)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-				if (path && ret != 0)
+				if (devt && ret != 0)
 					ret = -EBUSY;
 				break;
 			}
@@ -1372,12 +1367,12 @@ static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev
 	return disk_super;
 }
 
-int btrfs_forget_devices(const char *path)
+int btrfs_forget_devices(dev_t devt)
 {
 	int ret;
 
 	mutex_lock(&uuid_mutex);
-	ret = btrfs_free_stale_devices(strlen(path) ? path : NULL, NULL);
+	ret = btrfs_free_stale_devices(devt, NULL);
 	mutex_unlock(&uuid_mutex);
 
 	return ret;
@@ -1427,8 +1422,12 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
 
 	device = device_list_add(path, disk_super, &new_device_added);
 	if (!IS_ERR(device)) {
-		if (new_device_added)
-			btrfs_free_stale_devices(path, device);
+		if (new_device_added) {
+			dev_t devt;
+
+			if (!lookup_bdev(path, &devt))
+				btrfs_free_stale_devices(devt, device);
+		}
 	}
 
 	btrfs_release_disk_super(disk_super);
@@ -2659,6 +2658,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	int ret = 0;
 	bool seeding_dev = false;
 	bool locked = false;
+	dev_t devt;
 
 	if (sb_rdonly(sb) && !fs_devices->seeding)
 		return -EROFS;
@@ -2853,7 +2853,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	 * We can ignore the return value as it typically returns -EINVAL and
 	 * only succeeds if the device was an alien.
 	 */
-	btrfs_forget_devices(device_path);
+	if (!lookup_bdev(device_path, &devt))
+		btrfs_forget_devices(devt);
 
 	/* Update ctime/mtime for blkid or udev */
 	update_dev_time(device_path);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 98bbb293a3f9..76215de8ce34 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -529,7 +529,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       fmode_t flags, void *holder);
 struct btrfs_device *btrfs_scan_one_device(const char *path,
 					   fmode_t flags, void *holder);
-int btrfs_forget_devices(const char *path);
+int btrfs_forget_devices(dev_t devt);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
 void btrfs_assign_next_active_device(struct btrfs_device *device,
-- 
2.33.1

[PATCH v3 3/4] btrfs: add device major-minor info in the struct btrfs_device

[Anand Jain]

Internally it is common to use the major-minor number to identify a device
and, at a few locations in btrfs, we use the major-minor number to match
the device.

So when we identify a new btrfs device through device add or device
replace or device-scan/ready save the device's major-minor (dev_t) in the
struct btrfs_device so that we don't have to call lookup_bdev() again.

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

 fs/btrfs/dev-replace.c |  3 +++
 fs/btrfs/volumes.c     | 36 +++++++++++++++---------------------
 fs/btrfs/volumes.h     |  1 +
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 62b9651ea662..289d6cc1f5db 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -302,6 +302,9 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 		goto error;
 	}
 	rcu_assign_pointer(device->name, name);
+	ret = lookup_bdev(device_path, &device->devt);
+	if (ret)
+		goto error;
 
 	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 	device->generation = 0;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index cb43ee5925ad..33b5f40d030a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -817,11 +817,17 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 	struct rcu_string *name;
 	u64 found_transid = btrfs_super_generation(disk_super);
 	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
+	dev_t path_devt;
+	int error;
 	bool has_metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
 		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
 	bool fsid_change_in_progress = (btrfs_super_flags(disk_super) &
 					BTRFS_SUPER_FLAG_CHANGING_FSID_V2);
 
+	error = lookup_bdev(path, &path_devt);
+	if (error)
+		return ERR_PTR(error);
+
 	if (fsid_change_in_progress) {
 		if (!has_metadata_uuid)
 			fs_devices = find_fsid_inprogress(disk_super);
@@ -904,6 +910,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			return ERR_PTR(-ENOMEM);
 		}
 		rcu_assign_pointer(device->name, name);
+		device->devt = path_devt;
 
 		list_add_rcu(&device->dev_list, &fs_devices->devices);
 		fs_devices->num_devices++;
@@ -966,16 +973,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 		 * make sure it's the same device if the device is mounted
 		 */
 		if (device->bdev) {
-			int error;
-			dev_t path_dev;
-
-			error = lookup_bdev(path, &path_dev);
-			if (error) {
-				mutex_unlock(&fs_devices->device_list_mutex);
-				return ERR_PTR(error);
-			}
-
-			if (device->bdev->bd_dev != path_dev) {
+			if (device->devt != path_devt) {
 				mutex_unlock(&fs_devices->device_list_mutex);
 				/*
 				 * device->fs_info may not be reliable here, so
@@ -1008,6 +1006,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			fs_devices->missing_devices--;
 			clear_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state);
 		}
+		device->devt = path_devt;
 	}
 
 	/*
@@ -1421,14 +1420,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
 	}
 
 	device = device_list_add(path, disk_super, &new_device_added);
-	if (!IS_ERR(device)) {
-		if (new_device_added) {
-			dev_t devt;
-
-			if (!lookup_bdev(path, &devt))
-				btrfs_free_stale_devices(devt, device);
-		}
-	}
+	if (!IS_ERR(device) && new_device_added)
+		btrfs_free_stale_devices(device->devt, device);
 
 	btrfs_release_disk_super(disk_super);
 
@@ -2658,7 +2651,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	int ret = 0;
 	bool seeding_dev = false;
 	bool locked = false;
-	dev_t devt;
 
 	if (sb_rdonly(sb) && !fs_devices->seeding)
 		return -EROFS;
@@ -2708,6 +2700,9 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	device->fs_info = fs_info;
 	device->bdev = bdev;
+	ret = lookup_bdev(device_path, &device->devt);
+	if (ret)
+		goto error_free_device;
 
 	ret = btrfs_get_dev_zone_info(device, false);
 	if (ret)
@@ -2853,8 +2848,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	 * We can ignore the return value as it typically returns -EINVAL and
 	 * only succeeds if the device was an alien.
 	 */
-	if (!lookup_bdev(device_path, &devt))
-		btrfs_forget_devices(devt);
+	btrfs_forget_devices(device->devt);
 
 	/* Update ctime/mtime for blkid or udev */
 	update_dev_time(device_path);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 76215de8ce34..d75450a11713 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -73,6 +73,7 @@ struct btrfs_device {
 	/* the mode sent to blkdev_get */
 	fmode_t mode;
 
+	dev_t devt;
 	unsigned long dev_state;
 	blk_status_t last_flush_error;
 
-- 
2.33.1

[PATCH v3 4/4] btrfs: use dev_t to match device in device_matched

[Anand Jain]

Commit 6531891b2bcb ("btrfs: add device major-minor info in the struct
btrfs_device") saved the device major-minor number in the struct
btrfs_device upon discovering it.

So no need to lookup_bdev() again just match, which means
device_matched() can go away.

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

 fs/btrfs/volumes.c | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 33b5f40d030a..4d424c058a27 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -535,43 +535,6 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
 	return ret;
 }
 
-/*
- * Check if the device in the 'path' matches with the device in the given
- * struct btrfs_device '*device'.
- * Returns:
- *	0	If it is the same device.
- *	1	If it is not the same device.
- *	-errno	For error.
- */
-static int device_matched(struct btrfs_device *device, dev_t dev_new)
-{
-	char *device_name;
-	dev_t dev_old;
-	int ret;
-
-	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
-	if (!device_name)
-		return -ENOMEM;
-
-	rcu_read_lock();
-	ret = sprintf(device_name, "%s", rcu_str_deref(device->name));
-	rcu_read_unlock();
-	if (!ret) {
-		kfree(device_name);
-		return -EINVAL;
-	}
-
-	ret = lookup_bdev(device_name, &dev_old);
-	kfree(device_name);
-	if (ret)
-		return ret;
-
-	if (dev_old == dev_new)
-		return 0;
-
-	return 1;
-}
-
 /*
  *  Search and remove all stale (devices which are not mounted) devices.
  *  When both inputs are NULL, it will search and release all stale devices.
@@ -602,9 +565,7 @@ static int btrfs_free_stale_devices(dev_t devt,
 					 &fs_devices->devices, dev_list) {
 			if (skip_device && skip_device == device)
 				continue;
-			if (devt && !device->name)
-				continue;
-			if (devt && device_matched(device, devt) != 0)
+			if (devt && device->devt != devt)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-- 
2.33.1

Re: [PATCH v3 1/4] btrfs: harden identification of the stale device

[David Sterba]

On Fri, Jan 07, 2022 at 09:04:13PM +0800, Anand Jain wrote:
> Identifying and removing the stale device from the fs_uuids list is done
> by the function btrfs_free_stale_devices().
> btrfs_free_stale_devices() in turn depends on the function
> device_path_matched() to check if the device repeats in more than one
> btrfs_device structure.
> 
> The matching of the device happens by its path, the device path. However,
> when dm mapper is in use, the dm device paths are nothing but a link to
> the actual block device, which leads to the device_path_matched() failing
> to match.
> 
> Fix this by matching the dev_t as provided by lookup_bdev() instead of
> plain strcmp() the device paths.
> 
> Reported-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: -
> v2: Fix 
>      sparse: warning: incorrect type in argument 1 (different address spaces)
>      For using device->name->str
> 
>     Fix Josef suggestion to pass dev_t instead of device-path in the
>      patch 2/2.
> 
>  fs/btrfs/volumes.c | 41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4b244acd0cfa..ad9e08c3199c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -535,15 +535,46 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
>  	return ret;
>  }
>  
> -static bool device_path_matched(const char *path, struct btrfs_device *device)
> +/*
> + * Check if the device in the 'path' matches with the device in the given
> + * struct btrfs_device '*device'.
> + * Returns:
> + *	0	If it is the same device.
> + *	1	If it is not the same device.

The logic is reversed, as the function looks like a predicate, so it
should return a positive value if the condition is true. It's apparent a
the end.

> + *	-errno	For error.
> + */
> +static int device_matched(struct btrfs_device *device, const char *path)
>  {
> -	int found;
> +	char *device_name;
> +	dev_t dev_old;
> +	dev_t dev_new;
> +	int ret;
> +
> +	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
> +	if (!device_name)
> +		return -ENOMEM;
>  
>  	rcu_read_lock();
> -	found = strcmp(rcu_str_deref(device->name), path);
> +	ret = sprintf(device_name, "%s", rcu_str_deref(device->name));

Should this use scnprintf instead? That could check the length
BTRFS_PATH_NAME_MAX too, so we don't have to do strncpy and verify the
length manually.

>  	rcu_read_unlock();
> +	if (!ret) {
> +		kfree(device_name);
> +		return -EINVAL;
> +	}
>  
> -	return found == 0;
> +	ret = lookup_bdev(device_name, &dev_old);
> +	kfree(device_name);
> +	if (ret)
> +		return ret;
> +
> +	ret = lookup_bdev(path, &dev_new);
> +	if (ret)
> +		return ret;
> +
> +	if (dev_old == dev_new)
> +		return 0;

Here it's confusing, it's returning 0 ("false") but checking for
equality. It's probably to be able to merge the false and error value
to the same check, but then the helper is a bit inconsistent with the
predicate-like ones. If the errors are handled internally and the
result is true/false then it's ok, but still a bit confusing in case
it's not used in context of the stale devices (where we know the errors
don't matter).

> +
> +	return 1;
>  }
>  
>  /*
> @@ -578,7 +609,7 @@ static int btrfs_free_stale_devices(const char *path,
>  				continue;
>  			if (path && !device->name)
>  				continue;
> -			if (path && !device_path_matched(path, device))
> +			if (path && device_matched(device, path) != 0)
>  				continue;
>  			if (fs_devices->opened) {
>  				/* for an already deleted device return 0 */
> -- 
> 2.33.1

Re: [PATCH v3 2/4] btrfs: redeclare btrfs_stale_devices arg1 to dev_t

[David Sterba]

On Fri, Jan 07, 2022 at 09:04:14PM +0800, Anand Jain wrote:
> After the commit cb57afa39796 ("btrfs: harden identification of the stale

Please drop the commit id when referencing patches that haven't been in
released branch, the subject should be sufficient.

> device"), we don't have to match the device path anymore. Instead, we
> match the dev_t. So pass in the dev_t instead of the device-path, in the call
> chain btrfs_forget_devices()->btrfs_free_stale_devices().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: -
> 
>  fs/btrfs/super.c   |  8 +++++++-
>  fs/btrfs/volumes.c | 45 +++++++++++++++++++++++----------------------
>  fs/btrfs/volumes.h |  2 +-
>  3 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1ff03fb6c64a..bdf54b2673fe 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2386,6 +2386,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>  {
>  	struct btrfs_ioctl_vol_args *vol;
>  	struct btrfs_device *device = NULL;
> +	dev_t devt = 0;
>  	int ret = -ENOTTY;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -2405,7 +2406,12 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>  		mutex_unlock(&uuid_mutex);
>  		break;
>  	case BTRFS_IOC_FORGET_DEV:
> -		ret = btrfs_forget_devices(vol->name);
> +		if (strlen(vol->name)) {

The full strlen() is not necessary, just check the first byte.

> +			ret = lookup_bdev(vol->name, &devt);
> +			if (ret)
> +				break;
> +		}
> +		ret = btrfs_forget_devices(devt);
>  		break;
>  	case BTRFS_IOC_DEVICES_READY:
>  		mutex_lock(&uuid_mutex);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ad9e08c3199c..cb43ee5925ad 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -543,11 +543,10 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
>   *	1	If it is not the same device.
>   *	-errno	For error.
>   */
> -static int device_matched(struct btrfs_device *device, const char *path)
> +static int device_matched(struct btrfs_device *device, dev_t dev_new)
>  {
>  	char *device_name;
>  	dev_t dev_old;
> -	dev_t dev_new;
>  	int ret;
>  
>  	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
> @@ -567,10 +566,6 @@ static int device_matched(struct btrfs_device *device, const char *path)
>  	if (ret)
>  		return ret;
>  
> -	ret = lookup_bdev(path, &dev_new);
> -	if (ret)
> -		return ret;
> -
>  	if (dev_old == dev_new)
>  		return 0;
>  
> @@ -580,16 +575,16 @@ static int device_matched(struct btrfs_device *device, const char *path)
>  /*
>   *  Search and remove all stale (devices which are not mounted) devices.
>   *  When both inputs are NULL, it will search and release all stale devices.
> - *  path:	Optional. When provided will it release all unmounted devices
> - *		matching this path only.
> + *  devt:	Optional. When provided will it release all unmounted devices
> + *		matching this devt only.
>   *  skip_dev:	Optional. Will skip this device when searching for the stale
>   *		devices.
> - *  Return:	0 for success or if @path is NULL.
> - * 		-EBUSY if @path is a mounted device.
> - * 		-ENOENT if @path does not match any device in the list.
> + *  Return:	0 for success or if @devt is 0.
> + *		-EBUSY if @devt is a mounted device.
> + *		-ENOENT if @devt does not match any device in the list.
>   */
> -static int btrfs_free_stale_devices(const char *path,
> -				     struct btrfs_device *skip_device)
> +static int btrfs_free_stale_devices(dev_t devt,
> +				    struct btrfs_device *skip_device)
>  {
>  	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
>  	struct btrfs_device *device, *tmp_device;
> @@ -597,7 +592,7 @@ static int btrfs_free_stale_devices(const char *path,
>  
>  	lockdep_assert_held(&uuid_mutex);
>  
> -	if (path)
> +	if (devt)
>  		ret = -ENOENT;
>  
>  	list_for_each_entry_safe(fs_devices, tmp_fs_devices, &fs_uuids, fs_list) {
> @@ -607,13 +602,13 @@ static int btrfs_free_stale_devices(const char *path,
>  					 &fs_devices->devices, dev_list) {
>  			if (skip_device && skip_device == device)
>  				continue;
> -			if (path && !device->name)
> +			if (devt && !device->name)
>  				continue;
> -			if (path && device_matched(device, path) != 0)
> +			if (devt && device_matched(device, devt) != 0)
>  				continue;
>  			if (fs_devices->opened) {
>  				/* for an already deleted device return 0 */
> -				if (path && ret != 0)
> +				if (devt && ret != 0)
>  					ret = -EBUSY;
>  				break;
>  			}
> @@ -1372,12 +1367,12 @@ static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev
>  	return disk_super;
>  }
> -int btrfs_forget_devices(const char *path)
> +int btrfs_forget_devices(dev_t devt)
>  {
>  	int ret;
>  
>  	mutex_lock(&uuid_mutex);
> -	ret = btrfs_free_stale_devices(strlen(path) ? path : NULL, NULL);
> +	ret = btrfs_free_stale_devices(devt, NULL);
>  	mutex_unlock(&uuid_mutex);
>  
>  	return ret;
> @@ -1427,8 +1422,12 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
>  
>  	device = device_list_add(path, disk_super, &new_device_added);
>  	if (!IS_ERR(device)) {
> -		if (new_device_added)
> -			btrfs_free_stale_devices(path, device);
> +		if (new_device_added) {
> +			dev_t devt;
> +
> +			if (!lookup_bdev(path, &devt))

This reads like some negative condition, while what we expect is "if the
device exists and can be looked up" and not "if we can't look up the
device". Please write it like (lookup_bdev(...) == 0), and maybe add a
comment why we can ignore errors.

> +				btrfs_free_stale_devices(devt, device);
> +		}
>  	}
>  
>  	btrfs_release_disk_super(disk_super);
> @@ -2659,6 +2658,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  	int ret = 0;
>  	bool seeding_dev = false;
>  	bool locked = false;
> +	dev_t devt;
>  
>  	if (sb_rdonly(sb) && !fs_devices->seeding)
>  		return -EROFS;
> @@ -2853,7 +2853,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  	 * We can ignore the return value as it typically returns -EINVAL and
>  	 * only succeeds if the device was an alien.
>  	 */
> -	btrfs_forget_devices(device_path);
> +	if (!lookup_bdev(device_path, &devt))

Same here.

> +		btrfs_forget_devices(devt);
>  
>  	/* Update ctime/mtime for blkid or udev */
>  	update_dev_time(device_path);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 98bbb293a3f9..76215de8ce34 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -529,7 +529,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  		       fmode_t flags, void *holder);
>  struct btrfs_device *btrfs_scan_one_device(const char *path,
>  					   fmode_t flags, void *holder);
> -int btrfs_forget_devices(const char *path);
> +int btrfs_forget_devices(dev_t devt);
>  void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
>  void btrfs_assign_next_active_device(struct btrfs_device *device,
> -- 
> 2.33.1

Re: [PATCH v3 3/4] btrfs: add device major-minor info in the struct btrfs_device

[David Sterba]

On Fri, Jan 07, 2022 at 09:04:15PM +0800, Anand Jain wrote:
> Internally it is common to use the major-minor number to identify a device
> and, at a few locations in btrfs, we use the major-minor number to match
> the device.
> 
> So when we identify a new btrfs device through device add or device
> replace or device-scan/ready save the device's major-minor (dev_t) in the
> struct btrfs_device so that we don't have to call lookup_bdev() again.

Sounds good.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3: -
> 
>  fs/btrfs/dev-replace.c |  3 +++
>  fs/btrfs/volumes.c     | 36 +++++++++++++++---------------------
>  fs/btrfs/volumes.h     |  1 +
>  3 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 62b9651ea662..289d6cc1f5db 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -302,6 +302,9 @@ static int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
>  		goto error;
>  	}
>  	rcu_assign_pointer(device->name, name);
> +	ret = lookup_bdev(device_path, &device->devt);
> +	if (ret)
> +		goto error;
>  
>  	set_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
>  	device->generation = 0;
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index cb43ee5925ad..33b5f40d030a 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -817,11 +817,17 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  	struct rcu_string *name;
>  	u64 found_transid = btrfs_super_generation(disk_super);
>  	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
> +	dev_t path_devt;
> +	int error;
>  	bool has_metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
>  		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
>  	bool fsid_change_in_progress = (btrfs_super_flags(disk_super) &
>  					BTRFS_SUPER_FLAG_CHANGING_FSID_V2);
>  
> +	error = lookup_bdev(path, &path_devt);
> +	if (error)
> +		return ERR_PTR(error);
> +
>  	if (fsid_change_in_progress) {
>  		if (!has_metadata_uuid)
>  			fs_devices = find_fsid_inprogress(disk_super);
> @@ -904,6 +910,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			return ERR_PTR(-ENOMEM);
>  		}
>  		rcu_assign_pointer(device->name, name);
> +		device->devt = path_devt;
>  
>  		list_add_rcu(&device->dev_list, &fs_devices->devices);
>  		fs_devices->num_devices++;
> @@ -966,16 +973,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  		 * make sure it's the same device if the device is mounted
>  		 */
>  		if (device->bdev) {
> -			int error;
> -			dev_t path_dev;
> -
> -			error = lookup_bdev(path, &path_dev);
> -			if (error) {
> -				mutex_unlock(&fs_devices->device_list_mutex);
> -				return ERR_PTR(error);
> -			}
> -
> -			if (device->bdev->bd_dev != path_dev) {
> +			if (device->devt != path_devt) {
>  				mutex_unlock(&fs_devices->device_list_mutex);
>  				/*
>  				 * device->fs_info may not be reliable here, so
> @@ -1008,6 +1006,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			fs_devices->missing_devices--;
>  			clear_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state);
>  		}
> +		device->devt = path_devt;
>  	}
>  
>  	/*
> @@ -1421,14 +1420,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
>  	}
>  
>  	device = device_list_add(path, disk_super, &new_device_added);
> -	if (!IS_ERR(device)) {
> -		if (new_device_added) {
> -			dev_t devt;
> -
> -			if (!lookup_bdev(path, &devt))
> -				btrfs_free_stale_devices(devt, device);
> -		}
> -	}
> +	if (!IS_ERR(device) && new_device_added)
> +		btrfs_free_stale_devices(device->devt, device);
>  
>  	btrfs_release_disk_super(disk_super);
>  
> @@ -2658,7 +2651,6 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  	int ret = 0;
>  	bool seeding_dev = false;
>  	bool locked = false;
> -	dev_t devt;
>  
>  	if (sb_rdonly(sb) && !fs_devices->seeding)
>  		return -EROFS;
> @@ -2708,6 +2700,9 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  
>  	device->fs_info = fs_info;
>  	device->bdev = bdev;
> +	ret = lookup_bdev(device_path, &device->devt);
> +	if (ret)
> +		goto error_free_device;
>  
>  	ret = btrfs_get_dev_zone_info(device, false);
>  	if (ret)
> @@ -2853,8 +2848,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  	 * We can ignore the return value as it typically returns -EINVAL and
>  	 * only succeeds if the device was an alien.
>  	 */
> -	if (!lookup_bdev(device_path, &devt))
> -		btrfs_forget_devices(devt);
> +	btrfs_forget_devices(device->devt);
>  
>  	/* Update ctime/mtime for blkid or udev */
>  	update_dev_time(device_path);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 76215de8ce34..d75450a11713 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -73,6 +73,7 @@ struct btrfs_device {
>  	/* the mode sent to blkdev_get */
>  	fmode_t mode;
>  
> +	dev_t devt;

Please add a comment.

>  	unsigned long dev_state;
>  	blk_status_t last_flush_error;
>  
> -- 
> 2.33.1

Re: [PATCH v3 1/4] btrfs: harden identification of the stale device

[David Sterba]

On Fri, Jan 07, 2022 at 05:06:04PM +0100, David Sterba wrote:
> On Fri, Jan 07, 2022 at 09:04:13PM +0800, Anand Jain wrote:
> > Identifying and removing the stale device from the fs_uuids list is done
> > by the function btrfs_free_stale_devices().
> > btrfs_free_stale_devices() in turn depends on the function
> > device_path_matched() to check if the device repeats in more than one
> > btrfs_device structure.
> > 
> > The matching of the device happens by its path, the device path. However,
> > when dm mapper is in use, the dm device paths are nothing but a link to
> > the actual block device, which leads to the device_path_matched() failing
> > to match.
> > 
> > Fix this by matching the dev_t as provided by lookup_bdev() instead of
> > plain strcmp() the device paths.
> > 
> > Reported-by: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > ---
> > v3: -
> > v2: Fix 
> >      sparse: warning: incorrect type in argument 1 (different address spaces)
> >      For using device->name->str
> > 
> >     Fix Josef suggestion to pass dev_t instead of device-path in the
> >      patch 2/2.
> > 
> >  fs/btrfs/volumes.c | 41 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 4b244acd0cfa..ad9e08c3199c 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -535,15 +535,46 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
> >  	return ret;
> >  }
> >  
> > -static bool device_path_matched(const char *path, struct btrfs_device *device)
> > +/*
> > + * Check if the device in the 'path' matches with the device in the given
> > + * struct btrfs_device '*device'.
> > + * Returns:
> > + *	0	If it is the same device.
> > + *	1	If it is not the same device.
> 
> The logic is reversed, as the function looks like a predicate, so it
> should return a positive value if the condition is true. It's apparent a
> the end.
> 
> > + *	-errno	For error.
> > + */
> > +static int device_matched(struct btrfs_device *device, const char *path)
> >  {
> > -	int found;
> > +	char *device_name;
> > +	dev_t dev_old;
> > +	dev_t dev_new;
> > +	int ret;
> > +
> > +	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
> > +	if (!device_name)
> > +		return -ENOMEM;
> >  
> >  	rcu_read_lock();
> > -	found = strcmp(rcu_str_deref(device->name), path);
> > +	ret = sprintf(device_name, "%s", rcu_str_deref(device->name));
> 
> Should this use scnprintf instead? That could check the length
> BTRFS_PATH_NAME_MAX too, so we don't have to do strncpy and verify the
> length manually.
> 
> >  	rcu_read_unlock();
> > +	if (!ret) {
> > +		kfree(device_name);
> > +		return -EINVAL;
> > +	}
> >  
> > -	return found == 0;
> > +	ret = lookup_bdev(device_name, &dev_old);
> > +	kfree(device_name);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = lookup_bdev(path, &dev_new);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (dev_old == dev_new)
> > +		return 0;
> 
> Here it's confusing, it's returning 0 ("false") but checking for
> equality. It's probably to be able to merge the false and error value
> to the same check, but then the helper is a bit inconsistent with the
> predicate-like ones. If the errors are handled internally and the
> result is true/false then it's ok, but still a bit confusing in case
> it's not used in context of the stale devices (where we know the errors
> don't matter).

The last patch removes this helper completely so it would be pointless
to optimize this function, so you can ignore the comments above.

> 
> > +
> > +	return 1;
> >  }
> >  
> >  /*
> > @@ -578,7 +609,7 @@ static int btrfs_free_stale_devices(const char *path,
> >  				continue;
> >  			if (path && !device->name)
> >  				continue;
> > -			if (path && !device_path_matched(path, device))
> > +			if (path && device_matched(device, path) != 0)
> >  				continue;
> >  			if (fs_devices->opened) {
> >  				/* for an already deleted device return 0 */
> > -- 
> > 2.33.1

Re: [PATCH v3 1/4] btrfs: harden identification of the stale device

[Anand Jain]

On 08/01/2022 00:21, David Sterba wrote:
> On Fri, Jan 07, 2022 at 05:06:04PM +0100, David Sterba wrote:
>> On Fri, Jan 07, 2022 at 09:04:13PM +0800, Anand Jain wrote:
>>> Identifying and removing the stale device from the fs_uuids list is done
>>> by the function btrfs_free_stale_devices().
>>> btrfs_free_stale_devices() in turn depends on the function
>>> device_path_matched() to check if the device repeats in more than one
>>> btrfs_device structure.
>>>
>>> The matching of the device happens by its path, the device path. However,
>>> when dm mapper is in use, the dm device paths are nothing but a link to
>>> the actual block device, which leads to the device_path_matched() failing
>>> to match.
>>>
>>> Fix this by matching the dev_t as provided by lookup_bdev() instead of
>>> plain strcmp() the device paths.
>>>
>>> Reported-by: Josef Bacik <josef@toxicpanda.com>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> v3: -
>>> v2: Fix
>>>       sparse: warning: incorrect type in argument 1 (different address spaces)
>>>       For using device->name->str
>>>
>>>      Fix Josef suggestion to pass dev_t instead of device-path in the
>>>       patch 2/2.
>>>
>>>   fs/btrfs/volumes.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 4b244acd0cfa..ad9e08c3199c 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -535,15 +535,46 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder,
>>>   	return ret;
>>>   }
>>>   
>>> -static bool device_path_matched(const char *path, struct btrfs_device *device)
>>> +/*
>>> + * Check if the device in the 'path' matches with the device in the given
>>> + * struct btrfs_device '*device'.
>>> + * Returns:
>>> + *	0	If it is the same device.
>>> + *	1	If it is not the same device.
>>
>> The logic is reversed, as the function looks like a predicate, so it
>> should return a positive value if the condition is true. It's apparent a
>> the end.

This patch will be back-ported to 5.4 as well.

I fixed this in v4.

>>
>>> + *	-errno	For error.
>>> + */
>>> +static int device_matched(struct btrfs_device *device, const char *path)
>>>   {
>>> -	int found;
>>> +	char *device_name;
>>> +	dev_t dev_old;
>>> +	dev_t dev_new;
>>> +	int ret;
>>> +
>>> +	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
>>> +	if (!device_name)
>>> +		return -ENOMEM;
>>>   
>>>   	rcu_read_lock();
>>> -	found = strcmp(rcu_str_deref(device->name), path);
>>> +	ret = sprintf(device_name, "%s", rcu_str_deref(device->name));
>>
>> Should this use scnprintf instead? That could check the length
>> BTRFS_PATH_NAME_MAX too, so we don't have to do strncpy and verify the
>> length manually.
>>
   Right scnprintf is better.

>>>   	rcu_read_unlock();
>>> +	if (!ret) {
>>> +		kfree(device_name);
>>> +		return -EINVAL;
>>> +	}
>>>   
>>> -	return found == 0;
>>> +	ret = lookup_bdev(device_name, &dev_old);
>>> +	kfree(device_name);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = lookup_bdev(path, &dev_new);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (dev_old == dev_new)
>>> +		return 0;
>>
>> Here it's confusing, it's returning 0 ("false") but checking for
>> equality. It's probably to be able to merge the false and error value
>> to the same check, but then the helper is a bit inconsistent with the
>> predicate-like ones. If the errors are handled internally and the
>> result is true/false then it's ok, but still a bit confusing in case
>> it's not used in context of the stale devices (where we know the errors
>> don't matter).

  Got it. I fixed this in v4.

> The last patch removes this helper completely so it would be pointless
> to optimize this function, so you can ignore the comments above.

  This patch will be back-ported to 5.4.y. It is better we still fix it.
  Thanks for the review.

Thanks, Anand

>>
>>> +
>>> +	return 1;
>>>   }
>>>   
>>>   /*
>>> @@ -578,7 +609,7 @@ static int btrfs_free_stale_devices(const char *path,
>>>   				continue;
>>>   			if (path && !device->name)
>>>   				continue;
>>> -			if (path && !device_path_matched(path, device))
>>> +			if (path && device_matched(device, path) != 0)
>>>   				continue;
>>>   			if (fs_devices->opened) {
>>>   				/* for an already deleted device return 0 */
>>> -- 
>>> 2.33.1

Re: [PATCH v3 2/4] btrfs: redeclare btrfs_stale_devices arg1 to dev_t

[Anand Jain]

On 08/01/2022 00:16, David Sterba wrote:
> On Fri, Jan 07, 2022 at 09:04:14PM +0800, Anand Jain wrote:
>> After the commit cb57afa39796 ("btrfs: harden identification of the stale
> 
> Please drop the commit id when referencing patches that haven't been in
> released branch, the subject should be sufficient.

Got it.

>> @@ -2405,7 +2406,12 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>>   		mutex_unlock(&uuid_mutex);
>>   		break;
>>   	case BTRFS_IOC_FORGET_DEV:
>> -		ret = btrfs_forget_devices(vol->name);
>> +		if (strlen(vol->name)) {
> 
> The full strlen() is not necessary, just check the first byte.

Yep. Will update.


>> @@ -1427,8 +1422,12 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
>>   
>>   	device = device_list_add(path, disk_super, &new_device_added);
>>   	if (!IS_ERR(device)) {
>> -		if (new_device_added)
>> -			btrfs_free_stale_devices(path, device);
>> +		if (new_device_added) {
>> +			dev_t devt;
>> +
>> +			if (!lookup_bdev(path, &devt))
> 
> This reads like some negative condition, while what we expect is "if the
> device exists and can be looked up" and not "if we can't look up the
> device". Please write it like (lookup_bdev(...) == 0), and maybe add a
> comment why we can ignore errors.
> 

Ah. That's much easier to read. Changed in v4.


>> +				btrfs_free_stale_devices(devt, device);
>> +		}
>>   	}
>>   
>>   	btrfs_release_disk_super(disk_super);
>> @@ -2659,6 +2658,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   	int ret = 0;
>>   	bool seeding_dev = false;
>>   	bool locked = false;
>> +	dev_t devt;
>>   
>>   	if (sb_rdonly(sb) && !fs_devices->seeding)
>>   		return -EROFS;
>> @@ -2853,7 +2853,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   	 * We can ignore the return value as it typically returns -EINVAL and
>>   	 * only succeeds if the device was an alien.
>>   	 */
>> -	btrfs_forget_devices(device_path);
>> +	if (!lookup_bdev(device_path, &devt))
> 
> Same here.
> 
Ok, I fixed it in v4. However, patch 4/4 removes it.

Thanks, Anand

Re: [PATCH v3 3/4] btrfs: add device major-minor info in the struct btrfs_device

[Anand Jain]

>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index 76215de8ce34..d75450a11713 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -73,6 +73,7 @@ struct btrfs_device {
>>   	/* the mode sent to blkdev_get */
>>   	fmode_t mode;
>>   
>> +	dev_t devt;
> 
> Please add a comment.

  Added in v4.

Thanks,

> 
>>   	unsigned long dev_state;
>>   	blk_status_t last_flush_error;
>>   
>> -- 
>> 2.33.1