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

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

[Anand Jain]

v4: Return 1 for device matched in device_matched()
    Use scnprintf() instead of sprintf() in device_matched()
    Change log updated drop commit id
    Use str[0] instead of strlen()
    Change logic from !lookup_bdev() to lookup_bdev == 0

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     | 68 ++++++++++++++++++------------------------
 fs/btrfs/volumes.h     |  4 ++-
 4 files changed, 42 insertions(+), 41 deletions(-)

-- 
2.33.1

[PATCH v4 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>
---
v4: Return 1 for device matched in device_matched()
    Use scnprintf() instead of sprintf() in device_matched()
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 | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4b244acd0cfa..05333133e877 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -535,15 +535,43 @@ 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:
+ *	1	If it is the same device.
+ *	0	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);
+	scnprintf(device_name, BTRFS_PATH_NAME_MAX, "%s",
+		  rcu_str_deref(device->name));
 	rcu_read_unlock();
 
-	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 1;
+
+	return 0;
 }
 
 /*
@@ -578,7 +606,8 @@ static int btrfs_free_stale_devices(const char *path,
 				continue;
 			if (path && !device->name)
 				continue;
-			if (path && !device_path_matched(path, device))
+			/* Errors are considered as match failed */
+			if (path && device_matched(device, path) != 1)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-- 
2.33.1

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

[Nikolay Borisov]

On 10.01.22 г. 15:23, 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>
> ---
> v4: Return 1 for device matched in device_matched()
>     Use scnprintf() instead of sprintf() in device_matched()
> 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 | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4b244acd0cfa..05333133e877 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -535,15 +535,43 @@ 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:
> + *	1	If it is the same device.
> + *	0	If it is not the same device.
> + *	-errno	For error.
> + */
> +static int device_matched(struct btrfs_device *device, const char *path)
>  {

Again, this is a predicate function, the error return values is treated
the same as if the match failed. So why not simply make the function
bool and in case of error return false and be done with it? This is not
a big deal for the latest kernel as this function is removed in patch 4
but AFAIR it's going to be backported to 5.4. My question is do we have
 a real need to communicate the error? Given that errors can be safely
ignored here making a simpler interface is a better option.


<snip>

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

[Anand Jain]

>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 4b244acd0cfa..05333133e877 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -535,15 +535,43 @@ 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:
>> + *	1	If it is the same device.
>> + *	0	If it is not the same device.
>> + *	-errno	For error.
>> + */
>> +static int device_matched(struct btrfs_device *device, const char *path)
>>   {
> 
> Again, this is a predicate function, the error return values is treated
> the same as if the match failed. So why not simply make the function
> bool and in case of error return false and be done with it? This is not
> a big deal for the latest kernel as this function is removed in patch 4
> but AFAIR it's going to be backported to 5.4. My question is do we have
>   a real need to communicate the error? Given that errors can be safely
> ignored here making a simpler interface is a better option.

Ok. I agree. It can be simpler to treat errors as false and return bool. 
I will update.

Thanks.

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

[Anand Jain]

After the commit (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>
---
v4: Change log updated drop commit id
    Use name[0] instead of strlen()
    Change logic from !lookup_bdev() to lookup_bdev == 0
v3: -

 fs/btrfs/super.c   |  8 +++++++-
 fs/btrfs/volumes.c | 51 +++++++++++++++++++++++++---------------------
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1ff03fb6c64a..6eca93096ca5 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 (vol->name[0] != '\0') {
+			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 05333133e877..c3f387cb549d 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,
  *	0	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);
@@ -564,10 +563,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 1;
 
@@ -577,16 +572,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;
@@ -594,7 +589,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) {
@@ -604,14 +599,14 @@ 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;
 			/* Errors are considered as match failed */
-			if (path && device_matched(device, path) != 1)
+			if (devt && device_matched(device, devt) != 1)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-				if (path && ret != 0)
+				if (devt && ret != 0)
 					ret = -EBUSY;
 				break;
 			}
@@ -1370,12 +1365,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;
@@ -1424,9 +1419,15 @@ 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 (!IS_ERR(device) && new_device_added) {
+		dev_t devt;
+
+		/*
+		 * It is ok to ignore if we fail to free the stale device (if
+		 * any). As there is nothing much that can be done about it.
+		 */
+		if (lookup_bdev(path, &devt) == 0)
+			btrfs_free_stale_devices(devt, device);
 	}
 
 	btrfs_release_disk_super(disk_super);
@@ -2657,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;
@@ -2848,10 +2850,13 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	 * Now that we have written a new super block to this device, check all
 	 * other fs_devices list if device_path alienates any other scanned
 	 * device.
+	 * Skip forget_deivces if lookup_bdev() fails as there is nothing much
+	 * that can do about it.
 	 * 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) == 0)
+		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 v4 2/4] btrfs: redeclare btrfs_stale_devices arg1 to dev_t

[Nikolay Borisov]

On 10.01.22 г. 15:23, Anand Jain wrote:
> After the commit (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>
> ---
> v4: Change log updated drop commit id
>     Use name[0] instead of strlen()
>     Change logic from !lookup_bdev() to lookup_bdev == 0
> v3: -
> 
>  fs/btrfs/super.c   |  8 +++++++-
>  fs/btrfs/volumes.c | 51 +++++++++++++++++++++++++---------------------
>  fs/btrfs/volumes.h |  2 +-
>  3 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1ff03fb6c64a..6eca93096ca5 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 (vol->name[0] != '\0') {
> +			ret = lookup_bdev(vol->name, &devt);
> +			if (ret)
> +				break;
> +		}
> +		ret = btrfs_forget_devices(devt);
>  		break;
>  	case BTRFS_IOC_DEVICES_READY:
>  		mutex_lock(&uuid_mutex);

<snip>

> @@ -577,16 +572,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;
> @@ -594,7 +589,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) {
> @@ -604,14 +599,14 @@ 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)

This check is now rendered obsolete since ->name is used iff we have
passed a patch to match against it, but since your series removes the
path altogether having device->name becomes obsolete, hence it can be
removed.

<snip>

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

[Anand Jain]

>> @@ -604,14 +599,14 @@ 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)
> 
> This check is now rendered obsolete since ->name is used iff we have
> passed a patch to match against it, but since your series removes the
> path altogether having device->name becomes obsolete, hence it can be
> removed.

We have it to check for the missing device. Device->name == '\0' is one 
of the ways coded to identify a missing device. It helps to fail early 
instead of failing inside device_matched() at lookup_bdev().

Thanks, Anand

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

[Nikolay Borisov]

On 11.01.22 г. 6:51, Anand Jain wrote:
> 
>>> @@ -604,14 +599,14 @@ 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)
>>
>> This check is now rendered obsolete since ->name is used iff we have
>> passed a patch to match against it, but since your series removes the
>> path altogether having device->name becomes obsolete, hence it can be
>> removed.
> 
> We have it to check for the missing device. Device->name == '\0' is one
> of the ways coded to identify a missing device. It helps to fail early
> instead of failing inside device_matched() at lookup_bdev().

In this case shouldn't the check be just for if (!device->name) rather
than also checking for the presence of devt? Also a comment is warranted
that we are skipping missing devices.

> 
> Thanks, Anand
> 

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

[Anand Jain]

On 11/01/2022 16:30, Nikolay Borisov wrote:
> 
> 
> On 11.01.22 г. 6:51, Anand Jain wrote:
>>
>>>> @@ -604,14 +599,14 @@ 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)
>>>
>>> This check is now rendered obsolete since ->name is used iff we have
>>> passed a patch to match against it, but since your series removes the
>>> path altogether having device->name becomes obsolete, hence it can be
>>> removed.
>>
>> We have it to check for the missing device. Device->name == '\0' is one
>> of the ways coded to identify a missing device. It helps to fail early
>> instead of failing inside device_matched() at lookup_bdev().
> 
> In this case shouldn't the check be just for if (!device->name) rather
> than also checking for the presence of devt? Also a comment is warranted
> that we are skipping missing devices.

I think you missed the point that %devt is an argument there? It implies 
and frees the device with that matching %devt.
IMO it is straightforward that if %devt present then skips the devices 
without a name.
I will add the comment.

Thanks, Anand

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

[Nikolay Borisov]

On 11.01.22 г. 14:36, Anand Jain wrote:
> 
> 
> On 11/01/2022 16:30, Nikolay Borisov wrote:
>>
>>
>> On 11.01.22 г. 6:51, Anand Jain wrote:
>>>
>>>>> @@ -604,14 +599,14 @@ 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)
>>>>
>>>> This check is now rendered obsolete since ->name is used iff we have
>>>> passed a patch to match against it, but since your series removes the
>>>> path altogether having device->name becomes obsolete, hence it can be
>>>> removed.
>>>
>>> We have it to check for the missing device. Device->name == '\0' is one
>>> of the ways coded to identify a missing device. It helps to fail early
>>> instead of failing inside device_matched() at lookup_bdev().
>>
>> In this case shouldn't the check be just for if (!device->name) rather
>> than also checking for the presence of devt? Also a comment is warranted
>> that we are skipping missing devices.
> 
> I think you missed the point that %devt is an argument there? It implies
> and frees the device with that matching %devt.
> IMO it is straightforward that if %devt present then skips the devices
> without a name.
> I will add the comment.

Precisely, how is devt related to the name of the device? Before your
patch the path argument was directly related but now ?

> 
> Thanks, Anand
> 

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

[Nikolay Borisov]

On 11.01.22 г. 14:49, Nikolay Borisov wrote:
> 
> 
> On 11.01.22 г. 14:36, Anand Jain wrote:
>>
>>
>> On 11/01/2022 16:30, Nikolay Borisov wrote:
>>>
>>>
>>> On 11.01.22 г. 6:51, Anand Jain wrote:
>>>>
>>>>>> @@ -604,14 +599,14 @@ 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)
>>>>>
>>>>> This check is now rendered obsolete since ->name is used iff we have
>>>>> passed a patch to match against it, but since your series removes the
>>>>> path altogether having device->name becomes obsolete, hence it can be
>>>>> removed.
>>>>
>>>> We have it to check for the missing device. Device->name == '\0' is one
>>>> of the ways coded to identify a missing device. It helps to fail early
>>>> instead of failing inside device_matched() at lookup_bdev().
>>>
>>> In this case shouldn't the check be just for if (!device->name) rather
>>> than also checking for the presence of devt? Also a comment is warranted
>>> that we are skipping missing devices.
>>
>> I think you missed the point that %devt is an argument there? It implies
>> and frees the device with that matching %devt.
>> IMO it is straightforward that if %devt present then skips the devices
>> without a name.
>> I will add the comment.
> 
> Precisely, how is devt related to the name of the device? Before your
> patch the path argument was directly related but now ?

Ok, so ->name is used in the device_match function. But when it's gone
in patch 4 I think this check could be removed altogether as ->name is
no longer used.

> 
>>
>> Thanks, Anand
>>
> 

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

[Anand Jain]

>>>>>>> @@ -604,14 +599,14 @@ 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)
>>>>>>
>>>>>> This check is now rendered obsolete since ->name is used iff we have
>>>>>> passed a patch to match against it, but since your series removes the
>>>>>> path altogether having device->name becomes obsolete, hence it can be
>>>>>> removed.
>>>>>
>>>>> We have it to check for the missing device. Device->name == '\0' is one
>>>>> of the ways coded to identify a missing device. It helps to fail early
>>>>> instead of failing inside device_matched() at lookup_bdev().
>>>>
>>>> In this case shouldn't the check be just for if (!device->name) rather
>>>> than also checking for the presence of devt? Also a comment is warranted
>>>> that we are skipping missing devices.
>>>
>>> I think you missed the point that %devt is an argument there? It implies
>>> and frees the device with that matching %devt.
>>> IMO it is straightforward that if %devt present then skips the devices
>>> without a name.
>>> I will add the comment.
>>
>> Precisely, how is devt related to the name of the device? Before your
>> patch the path argument was directly related but now ?
> 
> Ok, so ->name is used in the device_match function. But when it's gone
> in patch 4 I think this check could be removed altogether as ->name is
> no longer used.

I have moved the whole check into device_matched(). So it goes away in 
patch 4.

[PATCH v4 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>
---
v4:  Add comment; And, use the logic lookup_bdev() == 0;
v3: -

 fs/btrfs/dev-replace.c |  3 +++
 fs/btrfs/volumes.c     | 40 +++++++++++++++-------------------------
 fs/btrfs/volumes.h     |  2 ++
 3 files changed, 20 insertions(+), 25 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 c3f387cb549d..9eedf8dfac02 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -815,11 +815,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);
@@ -902,6 +908,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++;
@@ -964,16 +971,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
@@ -1006,6 +1004,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;
 	}
 
 	/*
@@ -1419,16 +1418,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) && new_device_added) {
-		dev_t devt;
-
-		/*
-		 * It is ok to ignore if we fail to free the stale device (if
-		 * any). As there is nothing much that can be done about it.
-		 */
-		if (lookup_bdev(path, &devt) == 0)
-			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 +2649,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 +2698,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)
@@ -2850,13 +2843,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	 * Now that we have written a new super block to this device, check all
 	 * other fs_devices list if device_path alienates any other scanned
 	 * device.
-	 * Skip forget_deivces if lookup_bdev() fails as there is nothing much
-	 * that can do about it.
 	 * 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) == 0)
-		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..ebfe5dc73e26 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -73,6 +73,8 @@ struct btrfs_device {
 	/* the mode sent to blkdev_get */
 	fmode_t mode;
 
+	/* Device's major-minor number */
+	dev_t devt;
 	unsigned long dev_state;
 	blk_status_t last_flush_error;
 
-- 
2.33.1

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

[Goffredo Baroncelli]

On 10/01/2022 14.23, 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.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---

[...]

> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 76215de8ce34..ebfe5dc73e26 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -73,6 +73,8 @@ struct btrfs_device {
>   	/* the mode sent to blkdev_get */
>   	fmode_t mode;
>   
> +	/* Device's major-minor number */
> +	dev_t devt;

What is the relation between

	device->devt

and

	device->bdev->bd_dev

?

I assumed that there are situation where there is no a device connected to a btrfs_device
structure (e.g. a degraded mounted filesystem where a device is missing); in this case
does devt == 0 ?

But are there cases where devt != 0 (a device is associated to a block device structure) and bdev == NULL ?




>   	unsigned long dev_state;
>   	blk_status_t last_flush_error;
>   


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH v4 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..ebfe5dc73e26 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -73,6 +73,8 @@ struct btrfs_device {
>>       /* the mode sent to blkdev_get */
>>       fmode_t mode;
>> +    /* Device's major-minor number */
>> +    dev_t devt;
> 
> What is the relation between
> 
>      device->devt
> 
> and
> 
>      device->bdev->bd_dev
> 
> ?

  Both are same. device->bdev gets an update at the time device open. 
Otherwise, it is null.

> I assumed that there are situation where there is no a device connected 
> to a btrfs_device
> structure (e.g. a degraded mounted filesystem where a device is 
> missing); in this case
> does devt == 0 ?

  Even for the missing device we do call add_missing_dev() -> 
btrfs_alloc_device() that will ensure devt == 0.

> But are there cases where devt != 0 (a device is associated to a block 
> device structure) and bdev == NULL ?

  It is possible- When we unmount, the btrfs_device continues to exist 
and, both device->name and device->devt shall not be NULL/0; However, 
device->bdev will be NULL; If the device is scanned again with a 
different uuid, then the free_stale_device() is called to check and free 
the old/stale struct btrfs_device.

Thanks, Anand

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

[Goffredo Baroncelli]

On 11/01/2022 06.27, Anand Jain wrote:
> 
>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>>> index 76215de8ce34..ebfe5dc73e26 100644
>>> --- a/fs/btrfs/volumes.h
>>> +++ b/fs/btrfs/volumes.h
>>> @@ -73,6 +73,8 @@ struct btrfs_device {
>>>       /* the mode sent to blkdev_get */
>>>       fmode_t mode;
>>> +    /* Device's major-minor number */
>>> +    dev_t devt;
>>
>> What is the relation between
>>
>>      device->devt
>>
>> and
>>
>>      device->bdev->bd_dev
>>
>> ?
> 
>   Both are same. device->bdev gets an update at the time device open. Otherwise, it is null.
> 
>> I assumed that there are situation where there is no a device connected to a btrfs_device
>> structure (e.g. a degraded mounted filesystem where a device is missing); in this case
>> does devt == 0 ?
> 
>   Even for the missing device we do call add_missing_dev() -> btrfs_alloc_device() that will ensure devt == 0.
> 
>> But are there cases where devt != 0 (a device is associated to a block device structure) and bdev == NULL ?
> 
>   It is possible- When we unmount, the btrfs_device continues to exist and, both device->name and device->devt shall not be NULL/0; However, device->bdev will be NULL; If the device is scanned again with a different uuid, then the free_stale_device() is called to check and free the old/stale struct btrfs_device.

Ok, I think that this case (where devt!=0 and bdev==NULL) should be inserted as comment in the structure before the devt field.

> 
> Thanks, Anand


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

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

[Anand Jain]

On 12/01/2022 01:18, Goffredo Baroncelli wrote:
> On 11/01/2022 06.27, Anand Jain wrote:
>>
>>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>>>> index 76215de8ce34..ebfe5dc73e26 100644
>>>> --- a/fs/btrfs/volumes.h
>>>> +++ b/fs/btrfs/volumes.h
>>>> @@ -73,6 +73,8 @@ struct btrfs_device {
>>>>       /* the mode sent to blkdev_get */
>>>>       fmode_t mode;
>>>> +    /* Device's major-minor number */
>>>> +    dev_t devt;
>>>
>>> What is the relation between
>>>
>>>      device->devt
>>>
>>> and
>>>
>>>      device->bdev->bd_dev
>>>
>>> ?
>>
>>   Both are same. device->bdev gets an update at the time device open. 
>> Otherwise, it is null.
>>
>>> I assumed that there are situation where there is no a device 
>>> connected to a btrfs_device
>>> structure (e.g. a degraded mounted filesystem where a device is 
>>> missing); in this case
>>> does devt == 0 ?
>>
>>   Even for the missing device we do call add_missing_dev() -> 
>> btrfs_alloc_device() that will ensure devt == 0.
>>
>>> But are there cases where devt != 0 (a device is associated to a 
>>> block device structure) and bdev == NULL ?
>>
>>   It is possible- When we unmount, the btrfs_device continues to exist 
>> and, both device->name and device->devt shall not be NULL/0; However, 
>> device->bdev will be NULL; If the device is scanned again with a 
>> different uuid, then the free_stale_device() is called to check and 
>> free the old/stale struct btrfs_device.
> 
> Ok, I think that this case (where devt!=0 and bdev==NULL) should be 
> inserted as comment in the structure before the devt field.

I will add but, did you find any pitfall for breaking such a case?
Or are you submitting any patch based on this rule?

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

[Goffredo Baroncelli]

On 12/01/2022 04.17, Anand Jain wrote:
> 
> 
[...]
>>
>> Ok, I think that this case (where devt!=0 and bdev==NULL) should be inserted as comment in the structure before the devt field.
> 
> I will add but, did you find any pitfall for breaking such a case?
> Or are you submitting any patch based on this rule?

In a patch that I sent few days ago, I exported the major/minor
in <UUID>/devinfo/<devid>/major-minor sysfs property.
In order to do that, I taken the values from btrfs_device->bdev->bd_dev,
checking that bdev is not NULL.
So I didn't understood why we need to add copy of devt also
is the btrfs_device structure, because it is easy to taken the bdev one.

You answer was "we need that, because sometime bdev is NULL even
if a reasonable devt value exist", which is a valid reason.
So I suggest to add this info in the header so even the "next" developer
with a no so deep knowledge of the btrfs device life-cycle will be
aware of this case.

BR
G.Baroncelli
-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH v4 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>
---
v4: -
v3: -

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9eedf8dfac02..7834f7d18293 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -535,40 +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:
- *	1	If it is the same device.
- *	0	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();
-	scnprintf(device_name, BTRFS_PATH_NAME_MAX, "%s",
-		  rcu_str_deref(device->name));
-	rcu_read_unlock();
-
-	ret = lookup_bdev(device_name, &dev_old);
-	kfree(device_name);
-	if (ret)
-		return ret;
-
-	if (dev_old == dev_new)
-		return 1;
-
-	return 0;
-}
-
 /*
  *  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,7 +568,7 @@ static int btrfs_free_stale_devices(dev_t devt,
 			if (devt && !device->name)
 				continue;
 			/* Errors are considered as match failed */
-			if (devt && device_matched(device, devt) != 1)
+			if (devt && device->devt != devt)
 				continue;
 			if (fs_devices->opened) {
 				/* for an already deleted device return 0 */
-- 
2.33.1