[PATCH v5 0/2] btrfs: do some parameters optimization

[PATCH v5 0/2] btrfs: do some parameters optimization

[Gu Jinxiang]

Do parameters optimization for function btrfs_parse_early_options
and btrfs_scan_one_device.

Gu Jinxiang (2):
  btrfs: make fs_devices to be a local variable
  btrfs: get device pointer from btrfs_scan_one_device

 fs/btrfs/super.c   | 38 ++++++++++++++++++++++++--------------
 fs/btrfs/volumes.c | 22 ++++++++--------------
 fs/btrfs/volumes.h |  4 ++--
 3 files changed, 34 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] btrfs: make fs_devices to be a local variable

[Gu Jinxiang]

fs_devices is always passed to btrfs_scan_one_device which
overrides it. And in the call stack below fs_devices is passed to
btrfs_scan_one_device from btrfs_mount_root.
And in btrfs_mount_root the output fs_devices of this call stack
is not used.
btrfs_mount_root
        -> btrfs_parse_early_options
                ->btrfs_scan_one_device
So, there is no necessary to pass fs_devices from btrfs_mount_root,
use a local variable in btrfs_parse_early_options is enough.

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
Reviewed-by: Anand Jain <Anand.Jain@oracle.com>
---

Changelog:
v5: change a line wrap, and rebase to misc-next.
v4: changed a line warp, and adjusted the order of two rows.
v3: rebase to misc-next.
v2: deal with Nikolay's comment, make changelog more clair.

 fs/btrfs/super.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 28ab75ebb983..dcab4a0244e5 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -884,10 +884,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
  * only when we need to allocate a new super block.
  */
 static int btrfs_parse_early_options(const char *options, fmode_t flags,
-		void *holder, struct btrfs_fs_devices **fs_devices)
+					void *holder)
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *device_name, *opts, *orig, *p;
+	struct btrfs_fs_devices *fs_devices = NULL;
 	int error = 0;
 
 	lockdep_assert_held(&uuid_mutex);
@@ -917,8 +918,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 				error = -ENOMEM;
 				goto out;
 			}
-			error = btrfs_scan_one_device(device_name,
-					flags, holder, fs_devices);
+			error = btrfs_scan_one_device(device_name, flags,
+							holder, &fs_devices);
 			kfree(device_name);
 			if (error)
 				goto out;
@@ -1554,7 +1555,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	}
 
 	mutex_lock(&uuid_mutex);
-	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
+	error = btrfs_parse_early_options(data, mode, fs_type);
 	if (error) {
 		mutex_unlock(&uuid_mutex);
 		goto error_fs_info;
-- 
2.17.1

Re: [PATCH v5 1/2] btrfs: make fs_devices to be a local variable

[Nikolay Borisov]

On 16.07.2018 12:11, Gu Jinxiang wrote:
> fs_devices is always passed to btrfs_scan_one_device which
> overrides it. And in the call stack below fs_devices is passed to
> btrfs_scan_one_device from btrfs_mount_root.
> And in btrfs_mount_root the output fs_devices of this call stack
> is not used.
> btrfs_mount_root
>         -> btrfs_parse_early_options
>                 ->btrfs_scan_one_device
> So, there is no necessary to pass fs_devices from btrfs_mount_root,
> use a local variable in btrfs_parse_early_options is enough.
> 
> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> Reviewed-by: Anand Jain <Anand.Jain@oracle.com>
> ---
> 
> Changelog:
> v5: change a line wrap, and rebase to misc-next.
> v4: changed a line warp, and adjusted the order of two rows.
> v3: rebase to misc-next.
> v2: deal with Nikolay's comment, make changelog more clair.
> 
>  fs/btrfs/super.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 28ab75ebb983..dcab4a0244e5 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -884,10 +884,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>   * only when we need to allocate a new super block.
>   */
>  static int btrfs_parse_early_options(const char *options, fmode_t flags,
> -		void *holder, struct btrfs_fs_devices **fs_devices)
> +					void *holder)
>  {
>  	substring_t args[MAX_OPT_ARGS];
>  	char *device_name, *opts, *orig, *p;
> +	struct btrfs_fs_devices *fs_devices = NULL;
>  	int error = 0;
>  
>  	lockdep_assert_held(&uuid_mutex);
> @@ -917,8 +918,8 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>  				error = -ENOMEM;
>  				goto out;
>  			}
> -			error = btrfs_scan_one_device(device_name,
> -					flags, holder, fs_devices);
> +			error = btrfs_scan_one_device(device_name, flags,
> +							holder, &fs_devices);

You are line wraps are in fact worse than before, the first letter of
the "holder.." line has to be aligned exactly under the first letter of
"device_name". I assume David will fix this while merging so no need to
resend but fix your editor for future submissions.

>  			kfree(device_name);
>  			if (error)
>  				goto out;
> @@ -1554,7 +1555,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>  	}
>  
>  	mutex_lock(&uuid_mutex);
> -	error = btrfs_parse_early_options(data, mode, fs_type, &fs_devices);
> +	error = btrfs_parse_early_options(data, mode, fs_type);
>  	if (error) {
>  		mutex_unlock(&uuid_mutex);
>  		goto error_fs_info;
> 

[PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[Gu Jinxiang]

Instead of pointer to btrfs_fs_devices as an arg in
btrfs_scan_one_device, better to make it as a return value.

And since btrfs_fs_devices can be get by btrfs_device,
better to return btrfs_device than fs_btrfs_devices.

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---

Changelog:
v5: rebase to misc-next.
v4: as suggested by Anand, change return value of
    btrfs_scan_one_device from btrfs_fs_devices to btrfs_device.
v3: as comment by robot, use PTR_ERR_OR_ZERO, and rebase to misc-next.
v2: as comment by Nikolay, use ERR_CAST instead of cast type manually.

 fs/btrfs/super.c   | 35 ++++++++++++++++++++++-------------
 fs/btrfs/volumes.c | 22 ++++++++--------------
 fs/btrfs/volumes.h |  4 ++--
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index dcab4a0244e5..413bbe2d6db2 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -888,7 +888,7 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *device_name, *opts, *orig, *p;
-	struct btrfs_fs_devices *fs_devices = NULL;
+	struct btrfs_device *device = NULL;
 	int error = 0;
 
 	lockdep_assert_held(&uuid_mutex);
@@ -918,11 +918,13 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
 				error = -ENOMEM;
 				goto out;
 			}
-			error = btrfs_scan_one_device(device_name, flags,
-							holder, &fs_devices);
+			device = btrfs_scan_one_device(device_name, flags,
+							holder);
 			kfree(device_name);
-			if (error)
+			if (IS_ERR(device)) {
+				error = PTR_ERR(device);
 				goto out;
+			}
 		}
 	}
 
@@ -1518,6 +1520,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 {
 	struct block_device *bdev = NULL;
 	struct super_block *s;
+	struct btrfs_device *device = NULL;
 	struct btrfs_fs_devices *fs_devices = NULL;
 	struct btrfs_fs_info *fs_info = NULL;
 	struct security_mnt_opts new_sec_opts;
@@ -1561,12 +1564,15 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		goto error_fs_info;
 	}
 
-	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
-	if (error) {
+	device = btrfs_scan_one_device(device_name, mode, fs_type);
+	if (IS_ERR(device)) {
+		error = PTR_ERR(device);
 		mutex_unlock(&uuid_mutex);
 		goto error_fs_info;
 	}
 
+	fs_devices = device->fs_devices;
+
 	fs_info->fs_devices = fs_devices;
 
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
@@ -2227,7 +2233,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 				unsigned long arg)
 {
 	struct btrfs_ioctl_vol_args *vol;
-	struct btrfs_fs_devices *fs_devices;
+	struct btrfs_device *device = NULL;
 	int ret = -ENOTTY;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -2240,19 +2246,22 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 	switch (cmd) {
 	case BTRFS_IOC_SCAN_DEV:
 		mutex_lock(&uuid_mutex);
-		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
-					    &btrfs_root_fs_type, &fs_devices);
+		device = btrfs_scan_one_device(vol->name, FMODE_READ,
+					    &btrfs_root_fs_type);
+		ret = PTR_ERR_OR_ZERO(device);
 		mutex_unlock(&uuid_mutex);
 		break;
 	case BTRFS_IOC_DEVICES_READY:
 		mutex_lock(&uuid_mutex);
-		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
-					    &btrfs_root_fs_type, &fs_devices);
-		if (ret) {
+		device = btrfs_scan_one_device(vol->name, FMODE_READ,
+					    &btrfs_root_fs_type);
+		if (IS_ERR(device)) {
+			ret = PTR_ERR(device);
 			mutex_unlock(&uuid_mutex);
 			break;
 		}
-		ret = !(fs_devices->num_devices == fs_devices->total_devices);
+		ret = !(device->fs_devices->num_devices ==
+				device->fs_devices->total_devices);
 		mutex_unlock(&uuid_mutex);
 		break;
 	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 88d37bfa99c8..9f485696461b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1213,15 +1213,14 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
  * and we are not allowed to call set_blocksize during the scan. The superblock
  * is read via pagecache
  */
-int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
-			  struct btrfs_fs_devices **fs_devices_ret)
+struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
+					   void *holder)
 {
 	struct btrfs_super_block *disk_super;
 	bool new_device_added = false;
-	struct btrfs_device *device;
+	struct btrfs_device *ret = NULL;
 	struct block_device *bdev;
 	struct page *page;
-	int ret = 0;
 	u64 bytenr;
 
 	lockdep_assert_held(&uuid_mutex);
@@ -1237,21 +1236,16 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
 
 	bdev = blkdev_get_by_path(path, flags, holder);
 	if (IS_ERR(bdev))
-		return PTR_ERR(bdev);
+		return ERR_CAST(bdev);
 
 	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
-		ret = -EINVAL;
+		ret = ERR_PTR(-EINVAL);
 		goto error_bdev_put;
 	}
 
-	device = device_list_add(path, disk_super, &new_device_added);
-	if (IS_ERR(device)) {
-		ret = PTR_ERR(device);
-	} else {
-		*fs_devices_ret = device->fs_devices;
-		if (new_device_added)
-			btrfs_free_stale_devices(path, device);
-	}
+	ret = device_list_add(path, disk_super, &new_device_added);
+	if (new_device_added)
+		btrfs_free_stale_devices(path, ret);
 
 	btrfs_release_disk_super(page);
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 9665b84b1026..06d8bb7dd557 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -403,8 +403,8 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 			   int mirror_num, int async_submit);
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       fmode_t flags, void *holder);
-int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
-			  struct btrfs_fs_devices **fs_devices_ret);
+struct btrfs_device *btrfs_scan_one_device(const char *path,
+					   fmode_t flags, void *holder);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
-- 
2.17.1

Re: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[David Sterba]

On Mon, Jul 16, 2018 at 05:11:17PM +0800, Gu Jinxiang wrote:
> Instead of pointer to btrfs_fs_devices as an arg in
> btrfs_scan_one_device, better to make it as a return value.
> 
> And since btrfs_fs_devices can be get by btrfs_device,
> better to return btrfs_device than fs_btrfs_devices.
> 
> Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> ---
> 
> Changelog:
> v5: rebase to misc-next.
> v4: as suggested by Anand, change return value of
>     btrfs_scan_one_device from btrfs_fs_devices to btrfs_device.
> v3: as comment by robot, use PTR_ERR_OR_ZERO, and rebase to misc-next.
> v2: as comment by Nikolay, use ERR_CAST instead of cast type manually.
> 
>  fs/btrfs/super.c   | 35 ++++++++++++++++++++++-------------
>  fs/btrfs/volumes.c | 22 ++++++++--------------
>  fs/btrfs/volumes.h |  4 ++--
>  3 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index dcab4a0244e5..413bbe2d6db2 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -888,7 +888,7 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>  {
>  	substring_t args[MAX_OPT_ARGS];
>  	char *device_name, *opts, *orig, *p;
> -	struct btrfs_fs_devices *fs_devices = NULL;
> +	struct btrfs_device *device = NULL;
>  	int error = 0;
>  
>  	lockdep_assert_held(&uuid_mutex);
> @@ -918,11 +918,13 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
>  				error = -ENOMEM;
>  				goto out;
>  			}
> -			error = btrfs_scan_one_device(device_name, flags,
> -							holder, &fs_devices);
> +			device = btrfs_scan_one_device(device_name, flags,
> +							holder);
>  			kfree(device_name);
> -			if (error)
> +			if (IS_ERR(device)) {
> +				error = PTR_ERR(device);
>  				goto out;
> +			}
>  		}
>  	}
>  
> @@ -1518,6 +1520,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>  {
>  	struct block_device *bdev = NULL;
>  	struct super_block *s;
> +	struct btrfs_device *device = NULL;
>  	struct btrfs_fs_devices *fs_devices = NULL;
>  	struct btrfs_fs_info *fs_info = NULL;
>  	struct security_mnt_opts new_sec_opts;
> @@ -1561,12 +1564,15 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>  		goto error_fs_info;
>  	}
>  
> -	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> -	if (error) {
> +	device = btrfs_scan_one_device(device_name, mode, fs_type);
> +	if (IS_ERR(device)) {
> +		error = PTR_ERR(device);
>  		mutex_unlock(&uuid_mutex);
>  		goto error_fs_info;
>  	}
>  
> +	fs_devices = device->fs_devices;
> +
>  	fs_info->fs_devices = fs_devices;
>  
>  	error = btrfs_open_devices(fs_devices, mode, fs_type);
> @@ -2227,7 +2233,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>  				unsigned long arg)
>  {
>  	struct btrfs_ioctl_vol_args *vol;
> -	struct btrfs_fs_devices *fs_devices;
> +	struct btrfs_device *device = NULL;
>  	int ret = -ENOTTY;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -2240,19 +2246,22 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
>  	switch (cmd) {
>  	case BTRFS_IOC_SCAN_DEV:
>  		mutex_lock(&uuid_mutex);
> -		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
> -					    &btrfs_root_fs_type, &fs_devices);
> +		device = btrfs_scan_one_device(vol->name, FMODE_READ,
> +					    &btrfs_root_fs_type);
> +		ret = PTR_ERR_OR_ZERO(device);
>  		mutex_unlock(&uuid_mutex);
>  		break;
>  	case BTRFS_IOC_DEVICES_READY:
>  		mutex_lock(&uuid_mutex);
> -		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
> -					    &btrfs_root_fs_type, &fs_devices);
> -		if (ret) {
> +		device = btrfs_scan_one_device(vol->name, FMODE_READ,
> +					    &btrfs_root_fs_type);
> +		if (IS_ERR(device)) {
> +			ret = PTR_ERR(device);
>  			mutex_unlock(&uuid_mutex);
>  			break;
>  		}
> -		ret = !(fs_devices->num_devices == fs_devices->total_devices);
> +		ret = !(device->fs_devices->num_devices ==
> +				device->fs_devices->total_devices);
>  		mutex_unlock(&uuid_mutex);
>  		break;
>  	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 88d37bfa99c8..9f485696461b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1213,15 +1213,14 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
>   * and we are not allowed to call set_blocksize during the scan. The superblock
>   * is read via pagecache
>   */
> -int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> -			  struct btrfs_fs_devices **fs_devices_ret)
> +struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
> +					   void *holder)
>  {
>  	struct btrfs_super_block *disk_super;
>  	bool new_device_added = false;
> -	struct btrfs_device *device;
> +	struct btrfs_device *ret = NULL;
>  	struct block_device *bdev;
>  	struct page *page;
> -	int ret = 0;
>  	u64 bytenr;
>  
>  	lockdep_assert_held(&uuid_mutex);
> @@ -1237,21 +1236,16 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>  
>  	bdev = blkdev_get_by_path(path, flags, holder);
>  	if (IS_ERR(bdev))
> -		return PTR_ERR(bdev);
> +		return ERR_CAST(bdev);
>  
>  	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
> -		ret = -EINVAL;
> +		ret = ERR_PTR(-EINVAL);
>  		goto error_bdev_put;
>  	}
>  
> -	device = device_list_add(path, disk_super, &new_device_added);
> -	if (IS_ERR(device)) {
> -		ret = PTR_ERR(device);
> -	} else {
> -		*fs_devices_ret = device->fs_devices;
> -		if (new_device_added)
> -			btrfs_free_stale_devices(path, device);
> -	}
> +	ret = device_list_add(path, disk_super, &new_device_added);
> +	if (new_device_added)
> +		btrfs_free_stale_devices(path, ret);

Why is this skipping the error handling? I've resolved the conflict in a
different way in misc-next, have you had a look there?

Also the 'ret' got dropped and 'device' is used instead, the non-int
type for ret would be confusing.

>  
>  	btrfs_release_disk_super(page);
>  
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 9665b84b1026..06d8bb7dd557 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -403,8 +403,8 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>  			   int mirror_num, int async_submit);
>  int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  		       fmode_t flags, void *holder);
> -int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> -			  struct btrfs_fs_devices **fs_devices_ret);
> +struct btrfs_device *btrfs_scan_one_device(const char *path,
> +					   fmode_t flags, void *holder);
>  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
>  void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
> -- 
> 2.17.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 8038 bytes --]



> -----Original Message-----
> From: David Sterba [mailto:dsterba@suse.cz]
> Sent: Monday, July 16, 2018 8:34 PM
> To: Gu, Jinxiang/¹Ë ½ðÏã <gujx@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org; anand.jain@oracle.com
> Subject: Re: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device
> 
> On Mon, Jul 16, 2018 at 05:11:17PM +0800, Gu Jinxiang wrote:
> > Instead of pointer to btrfs_fs_devices as an arg in
> > btrfs_scan_one_device, better to make it as a return value.
> >
> > And since btrfs_fs_devices can be get by btrfs_device,
> > better to return btrfs_device than fs_btrfs_devices.
> >
> > Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> > ---
> >
> > Changelog:
> > v5: rebase to misc-next.
> > v4: as suggested by Anand, change return value of
> >     btrfs_scan_one_device from btrfs_fs_devices to btrfs_device.
> > v3: as comment by robot, use PTR_ERR_OR_ZERO, and rebase to misc-next.
> > v2: as comment by Nikolay, use ERR_CAST instead of cast type manually.
> >
> >  fs/btrfs/super.c   | 35 ++++++++++++++++++++++-------------
> >  fs/btrfs/volumes.c | 22 ++++++++--------------
> >  fs/btrfs/volumes.h |  4 ++--
> >  3 files changed, 32 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index dcab4a0244e5..413bbe2d6db2 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -888,7 +888,7 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
> >  {
> >  	substring_t args[MAX_OPT_ARGS];
> >  	char *device_name, *opts, *orig, *p;
> > -	struct btrfs_fs_devices *fs_devices = NULL;
> > +	struct btrfs_device *device = NULL;
> >  	int error = 0;
> >
> >  	lockdep_assert_held(&uuid_mutex);
> > @@ -918,11 +918,13 @@ static int btrfs_parse_early_options(const char *options, fmode_t flags,
> >  				error = -ENOMEM;
> >  				goto out;
> >  			}
> > -			error = btrfs_scan_one_device(device_name, flags,
> > -							holder, &fs_devices);
> > +			device = btrfs_scan_one_device(device_name, flags,
> > +							holder);
> >  			kfree(device_name);
> > -			if (error)
> > +			if (IS_ERR(device)) {
> > +				error = PTR_ERR(device);
> >  				goto out;
> > +			}
> >  		}
> >  	}
> >
> > @@ -1518,6 +1520,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
> >  {
> >  	struct block_device *bdev = NULL;
> >  	struct super_block *s;
> > +	struct btrfs_device *device = NULL;
> >  	struct btrfs_fs_devices *fs_devices = NULL;
> >  	struct btrfs_fs_info *fs_info = NULL;
> >  	struct security_mnt_opts new_sec_opts;
> > @@ -1561,12 +1564,15 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
> >  		goto error_fs_info;
> >  	}
> >
> > -	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> > -	if (error) {
> > +	device = btrfs_scan_one_device(device_name, mode, fs_type);
> > +	if (IS_ERR(device)) {
> > +		error = PTR_ERR(device);
> >  		mutex_unlock(&uuid_mutex);
> >  		goto error_fs_info;
> >  	}
> >
> > +	fs_devices = device->fs_devices;
> > +
> >  	fs_info->fs_devices = fs_devices;
> >
> >  	error = btrfs_open_devices(fs_devices, mode, fs_type);
> > @@ -2227,7 +2233,7 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
> >  				unsigned long arg)
> >  {
> >  	struct btrfs_ioctl_vol_args *vol;
> > -	struct btrfs_fs_devices *fs_devices;
> > +	struct btrfs_device *device = NULL;
> >  	int ret = -ENOTTY;
> >
> >  	if (!capable(CAP_SYS_ADMIN))
> > @@ -2240,19 +2246,22 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
> >  	switch (cmd) {
> >  	case BTRFS_IOC_SCAN_DEV:
> >  		mutex_lock(&uuid_mutex);
> > -		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
> > -					    &btrfs_root_fs_type, &fs_devices);
> > +		device = btrfs_scan_one_device(vol->name, FMODE_READ,
> > +					    &btrfs_root_fs_type);
> > +		ret = PTR_ERR_OR_ZERO(device);
> >  		mutex_unlock(&uuid_mutex);
> >  		break;
> >  	case BTRFS_IOC_DEVICES_READY:
> >  		mutex_lock(&uuid_mutex);
> > -		ret = btrfs_scan_one_device(vol->name, FMODE_READ,
> > -					    &btrfs_root_fs_type, &fs_devices);
> > -		if (ret) {
> > +		device = btrfs_scan_one_device(vol->name, FMODE_READ,
> > +					    &btrfs_root_fs_type);
> > +		if (IS_ERR(device)) {
> > +			ret = PTR_ERR(device);
> >  			mutex_unlock(&uuid_mutex);
> >  			break;
> >  		}
> > -		ret = !(fs_devices->num_devices == fs_devices->total_devices);
> > +		ret = !(device->fs_devices->num_devices ==
> > +				device->fs_devices->total_devices);
> >  		mutex_unlock(&uuid_mutex);
> >  		break;
> >  	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 88d37bfa99c8..9f485696461b 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1213,15 +1213,14 @@ static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
> >   * and we are not allowed to call set_blocksize during the scan. The superblock
> >   * is read via pagecache
> >   */
> > -int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> > -			  struct btrfs_fs_devices **fs_devices_ret)
> > +struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
> > +					   void *holder)
> >  {
> >  	struct btrfs_super_block *disk_super;
> >  	bool new_device_added = false;
> > -	struct btrfs_device *device;
> > +	struct btrfs_device *ret = NULL;
> >  	struct block_device *bdev;
> >  	struct page *page;
> > -	int ret = 0;
> >  	u64 bytenr;
> >
> >  	lockdep_assert_held(&uuid_mutex);
> > @@ -1237,21 +1236,16 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> >
> >  	bdev = blkdev_get_by_path(path, flags, holder);
> >  	if (IS_ERR(bdev))
> > -		return PTR_ERR(bdev);
> > +		return ERR_CAST(bdev);
> >
> >  	if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
> > -		ret = -EINVAL;
> > +		ret = ERR_PTR(-EINVAL);
> >  		goto error_bdev_put;
> >  	}
> >
> > -	device = device_list_add(path, disk_super, &new_device_added);
> > -	if (IS_ERR(device)) {
> > -		ret = PTR_ERR(device);
> > -	} else {
> > -		*fs_devices_ret = device->fs_devices;
> > -		if (new_device_added)
> > -			btrfs_free_stale_devices(path, device);
> > -	}
> > +	ret = device_list_add(path, disk_super, &new_device_added);
> > +	if (new_device_added)
> > +		btrfs_free_stale_devices(path, ret);
> 
> Why is this skipping the error handling? I've resolved the conflict in a
> different way in misc-next, have you had a look there?

Since when new_device_added set to be true, the return value of device_list_add
must not a error value.
So, I think no necessary to add the judge.

> 
> Also the 'ret' got dropped and 'device' is used instead, the non-int
> type for ret would be confusing.
OK, Thank you.


> 
> >
> >  	btrfs_release_disk_super(page);
> >
> > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> > index 9665b84b1026..06d8bb7dd557 100644
> > --- a/fs/btrfs/volumes.h
> > +++ b/fs/btrfs/volumes.h
> > @@ -403,8 +403,8 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> >  			   int mirror_num, int async_submit);
> >  int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
> >  		       fmode_t flags, void *holder);
> > -int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> > -			  struct btrfs_fs_devices **fs_devices_ret);
> > +struct btrfs_device *btrfs_scan_one_device(const char *path,
> > +					   fmode_t flags, void *holder);
> >  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
> >  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
> >  void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
> > --
> > 2.17.1
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[Anand Jain]

::

>>> @@ -1561,12 +1564,15 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
>>>   		goto error_fs_info;
>>>   	}
>>>
>>> -	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
>>> -	if (error) {
>>> +	device = btrfs_scan_one_device(device_name, mode, fs_type);
>>> +	if (IS_ERR(device)) {
>>> +		error = PTR_ERR(device);
>>>   		mutex_unlock(&uuid_mutex);
>>>   		goto error_fs_info;
>>>   	}
>>>
>>> +	fs_devices = device->fs_devices;
>>> +

  The version at misc-next is missing this assignment and it panics.

::

Thanks, Anand

Re: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[David Sterba]

On Tue, Jul 17, 2018 at 12:54:00PM +0800, Anand Jain wrote:
> 
> ::
> 
> >>> @@ -1561,12 +1564,15 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
> >>>   		goto error_fs_info;
> >>>   	}
> >>>
> >>> -	error = btrfs_scan_one_device(device_name, mode, fs_type, &fs_devices);
> >>> -	if (error) {
> >>> +	device = btrfs_scan_one_device(device_name, mode, fs_type);
> >>> +	if (IS_ERR(device)) {
> >>> +		error = PTR_ERR(device);
> >>>   		mutex_unlock(&uuid_mutex);
> >>>   		goto error_fs_info;
> >>>   	}
> >>>
> >>> +	fs_devices = device->fs_devices;
> >>> +
> 
>   The version at misc-next is missing this assignment and it panics.

Thanks, that was my mismerge. Now added back and pushed.

Re: [PATCH v5 2/2] btrfs: get device pointer from btrfs_scan_one_device

[David Sterba]

On Tue, Jul 17, 2018 at 01:08:34AM +0000, Gu, Jinxiang wrote:
> > > -	device = device_list_add(path, disk_super, &new_device_added);
> > > -	if (IS_ERR(device)) {
> > > -		ret = PTR_ERR(device);
> > > -	} else {
> > > -		*fs_devices_ret = device->fs_devices;
> > > -		if (new_device_added)
> > > -			btrfs_free_stale_devices(path, device);
> > > -	}
> > > +	ret = device_list_add(path, disk_super, &new_device_added);
> > > +	if (new_device_added)
> > > +		btrfs_free_stale_devices(path, ret);
> > 
> > Why is this skipping the error handling? I've resolved the conflict in a
> > different way in misc-next, have you had a look there?
> 
> Since when new_device_added set to be true, the return value of device_list_add
> must not a error value.

Yeah, but this is not obvious from the context and looks like "
device_list_add fails but we'll do something anyway", so I'm for keeping
the check.

> So, I think no necessary to add the judge.