[PATCH 0/8] Misc device cleanup patches: part1

[PATCH 0/8] Misc device cleanup patches: part1

[Anand Jain]

A miscellaneous device management related cleanup patches. The core
objective of this and upcoming patches is to able to have a clear
clarity on mutex usages and remove redundant codes. And few good
to have fixes.

Anand Jain (7):
  btrfs: optimize use of volume_mutex in btrfs_ioctl_resize()
  btrfs: move user provided string checks outside of volume_mutex
  btrfs: use i_size_read() instead of open code
  btrfs: rename btrfs_add_device to btrfs_add_dev_item
  btrfs: btrfs_rm_device() does not need uuid_mutex
  btrfs: move volume_mutex into btrfs_init_new_device()
  btrfs: rename btrfs_init_new_device() to btrfs_add_device()

 fs/btrfs/ioctl.c   | 64 +++++++++++++++++++++++++++---------------------------
 fs/btrfs/volumes.c | 20 ++++++++++-------
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 45 insertions(+), 41 deletions(-)

-- 
2.13.1

[PATCH 1/7] btrfs: optimize use of volume_mutex in btrfs_ioctl_resize()

[Anand Jain]

We can push volume_mutex lock further down after the memory operation.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 09c95f1b07dc..2eb220213d63 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1470,7 +1470,6 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 	}
 
-	mutex_lock(&fs_info->volume_mutex);
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
 		ret = PTR_ERR(vol_args);
@@ -1495,12 +1494,13 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
+	mutex_lock(&fs_info->volume_mutex);
 	device = btrfs_find_device(fs_info, devid, NULL, NULL);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
 		ret = -ENODEV;
-		goto out_free;
+		goto out_mutex;
 	}
 
 	if (!device->writeable) {
@@ -1508,7 +1508,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 			   "resizer unable to apply on readonly device %llu",
 		       devid);
 		ret = -EPERM;
-		goto out_free;
+		goto out_mutex;
 	}
 
 	if (!strcmp(sizestr, "max"))
@@ -1524,13 +1524,13 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		new_size = memparse(sizestr, &retptr);
 		if (*retptr != '\0' || new_size == 0) {
 			ret = -EINVAL;
-			goto out_free;
+			goto out_mutex;
 		}
 	}
 
 	if (device->is_tgtdev_for_dev_replace) {
 		ret = -EPERM;
-		goto out_free;
+		goto out_mutex;
 	}
 
 	old_size = btrfs_device_get_total_bytes(device);
@@ -1538,24 +1538,24 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 	if (mod < 0) {
 		if (new_size > old_size) {
 			ret = -EINVAL;
-			goto out_free;
+			goto out_mutex;
 		}
 		new_size = old_size - new_size;
 	} else if (mod > 0) {
 		if (new_size > ULLONG_MAX - old_size) {
 			ret = -ERANGE;
-			goto out_free;
+			goto out_mutex;
 		}
 		new_size = old_size + new_size;
 	}
 
 	if (new_size < SZ_256M) {
 		ret = -EINVAL;
-		goto out_free;
+		goto out_mutex;
 	}
 	if (new_size > device->bdev->bd_inode->i_size) {
 		ret = -EFBIG;
-		goto out_free;
+		goto out_mutex;
 	}
 
 	new_size = round_down(new_size, fs_info->sectorsize);
@@ -1567,7 +1567,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		trans = btrfs_start_transaction(root, 0);
 		if (IS_ERR(trans)) {
 			ret = PTR_ERR(trans);
-			goto out_free;
+			goto out_mutex;
 		}
 		ret = btrfs_grow_device(trans, device, new_size);
 		btrfs_commit_transaction(trans);
@@ -1575,10 +1575,11 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		ret = btrfs_shrink_device(device, new_size);
 	} /* equal, nothing need to do */
 
+out_mutex:
+	mutex_unlock(&fs_info->volume_mutex);
 out_free:
 	kfree(vol_args);
 out:
-	mutex_unlock(&fs_info->volume_mutex);
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
 	mnt_drop_write_file(file);
 	return ret;
-- 
2.13.1

Re: [PATCH 1/7] btrfs: optimize use of volume_mutex in btrfs_ioctl_resize()

[David Sterba]

On Mon, Nov 06, 2017 at 04:36:12PM +0800, Anand Jain wrote:
> We can push volume_mutex lock further down after the memory operation.

I'm a bit reluctant to apply any patches that cleanup volume_mutex as
it's going to be removed.

[PATCH 2/7] btrfs: move user provided string checks outside of volume_mutex

[Anand Jain]

Do string check for the device's new size before volume_mutex is held.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2eb220213d63..b0465020972a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1445,7 +1445,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 {
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	u64 new_size;
+	u64 new_size = 0;
 	u64 old_size;
 	u64 devid = 1;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -1494,6 +1494,21 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
+	if (strcmp(sizestr, "max")) {
+		if (sizestr[0] == '-') {
+			mod = -1;
+			sizestr++;
+		} else if (sizestr[0] == '+') {
+			mod = 1;
+			sizestr++;
+		}
+		new_size = memparse(sizestr, &retptr);
+		if (*retptr != '\0' || new_size == 0) {
+			ret = -EINVAL;
+			goto out_free;
+		}
+	}
+
 	mutex_lock(&fs_info->volume_mutex);
 	device = btrfs_find_device(fs_info, devid, NULL, NULL);
 	if (!device) {
@@ -1511,28 +1526,14 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		goto out_mutex;
 	}
 
-	if (!strcmp(sizestr, "max"))
-		new_size = device->bdev->bd_inode->i_size;
-	else {
-		if (sizestr[0] == '-') {
-			mod = -1;
-			sizestr++;
-		} else if (sizestr[0] == '+') {
-			mod = 1;
-			sizestr++;
-		}
-		new_size = memparse(sizestr, &retptr);
-		if (*retptr != '\0' || new_size == 0) {
-			ret = -EINVAL;
-			goto out_mutex;
-		}
-	}
-
 	if (device->is_tgtdev_for_dev_replace) {
 		ret = -EPERM;
 		goto out_mutex;
 	}
 
+	if (!new_size)
+		new_size = device->bdev->bd_inode->i_size;
+
 	old_size = btrfs_device_get_total_bytes(device);
 
 	if (mod < 0) {
-- 
2.13.1

[PATCH 3/7] btrfs: use i_size_read() instead of open code

[Anand Jain]

As i_size_read() takes care of 32bit smp or preempt cases as well.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b0465020972a..86e7f5abd740 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1532,7 +1532,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 	}
 
 	if (!new_size)
-		new_size = device->bdev->bd_inode->i_size;
+		new_size = i_size_read(device->bdev->bd_inode);
 
 	old_size = btrfs_device_get_total_bytes(device);
 
@@ -1554,7 +1554,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		ret = -EINVAL;
 		goto out_mutex;
 	}
-	if (new_size > device->bdev->bd_inode->i_size) {
+	if (new_size > i_size_read(device->bdev->bd_inode)) {
 		ret = -EFBIG;
 		goto out_mutex;
 	}
-- 
2.13.1

Re: [PATCH 3/7] btrfs: use i_size_read() instead of open code

[David Sterba]

On Mon, Nov 06, 2017 at 04:36:14PM +0800, Anand Jain wrote:
> As i_size_read() takes care of 32bit smp or preempt cases as well.

Can bdev->bd_inode->i_size change so that we need to use the
i_size_read()? My answer is 'no'. You haven't provided any reasoning
why it should be otherwise.

Re: [PATCH 3/7] btrfs: use i_size_read() instead of open code

[Anand Jain]

On 11/07/2017 12:52 AM, David Sterba wrote:
> On Mon, Nov 06, 2017 at 04:36:14PM +0800, Anand Jain wrote:
>> As i_size_read() takes care of 32bit smp or preempt cases as well.
> 
> Can bdev->bd_inode->i_size change so that we need to use the
> i_size_read()? My answer is 'no'.

  Hm. Right I was looking at it only from the theoretical point of view.

  And I presume you mean to say disk resize at the block layer is not
  really a practically achievable concern.

Thanks, Anand

Re: [PATCH 3/7] btrfs: use i_size_read() instead of open code

[David Sterba]

On Tue, Nov 07, 2017 at 07:44:59AM +0800, Anand Jain wrote:
> 
> 
> On 11/07/2017 12:52 AM, David Sterba wrote:
> > On Mon, Nov 06, 2017 at 04:36:14PM +0800, Anand Jain wrote:
> >> As i_size_read() takes care of 32bit smp or preempt cases as well.
> > 
> > Can bdev->bd_inode->i_size change so that we need to use the
> > i_size_read()? My answer is 'no'.
> 
>   Hm. Right I was looking at it only from the theoretical point of view.
> 
>   And I presume you mean to say disk resize at the block layer is not
>   really a practically achievable concern.

There are some safety checks, at least if there are bios in flight on
the partition when it's mounted.

[PATCH 4/7] btrfs: rename btrfs_add_device to btrfs_add_dev_item

[Anand Jain]

Function btrfs_add_device() is about adding the device item
so rename to reflect that in the function. Similarly we have
btrfs_rm_dev_item().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5b03c2b1b851..2329f429ce40 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1716,7 +1716,7 @@ static noinline int find_next_devid(struct btrfs_fs_info *fs_info,
  * the device information is stored in the chunk root
  * the btrfs_device struct should be fully filled in
  */
-static int btrfs_add_device(struct btrfs_trans_handle *trans,
+static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 			    struct btrfs_fs_info *fs_info,
 			    struct btrfs_device *device)
 {
@@ -2501,7 +2501,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		}
 	}
 
-	ret = btrfs_add_device(trans, fs_info, device);
+	ret = btrfs_add_dev_item(trans, fs_info, device);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
 		goto error_sysfs;
-- 
2.13.1

Re: [PATCH 4/7] btrfs: rename btrfs_add_device to btrfs_add_dev_item

[David Sterba]

On Mon, Nov 06, 2017 at 04:36:15PM +0800, Anand Jain wrote:
> Function btrfs_add_device() is about adding the device item
> so rename to reflect that in the function. Similarly we have
> btrfs_rm_dev_item().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH 5/7] btrfs: btrfs_rm_device() does not need uuid_mutex

[Anand Jain]

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2329f429ce40..3d5ed4518f20 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1914,7 +1914,6 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	int ret = 0;
 
 	mutex_lock(&fs_info->volume_mutex);
-	mutex_lock(&uuid_mutex);
 
 	num_devices = fs_info->fs_devices->num_devices;
 	btrfs_dev_replace_lock(&fs_info->dev_replace, 0);
@@ -1950,9 +1949,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		mutex_unlock(&fs_info->chunk_mutex);
 	}
 
-	mutex_unlock(&uuid_mutex);
 	ret = btrfs_shrink_device(device, 0);
-	mutex_lock(&uuid_mutex);
 	if (ret)
 		goto error_undo;
 
@@ -2027,7 +2024,6 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	}
 
 out:
-	mutex_unlock(&uuid_mutex);
 	mutex_unlock(&fs_info->volume_mutex);
 	return ret;
 
-- 
2.13.1

Re: [PATCH 5/7] btrfs: btrfs_rm_device() does not need uuid_mutex

[David Sterba]

On Mon, Nov 06, 2017 at 04:36:16PM +0800, Anand Jain wrote:
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

No changelog at all? Sorry but I'm tired of asking you for explanations
each time. Removing a mutex is far from a trivial change.

Re: [PATCH 5/7] btrfs: btrfs_rm_device() does not need uuid_mutex

[Anand Jain]

On 11/07/2017 01:02 AM, David Sterba wrote:
> On Mon, Nov 06, 2017 at 04:36:16PM +0800, Anand Jain wrote:
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> No changelog at all?

  Oh. This patch is wrong. I thought I deleted this from my workspace.
  Now I notice this ended up in the ML.

  We need to uuid_mutex for the case of removing a seed device where
  the fsids are different.

Thanks, Anand

[PATCH 6/7] btrfs: move volume_mutex into btrfs_init_new_device()

[Anand Jain]

By moving the volume_mutex into btrfs_init_new_device() it will be
much closer to the items to be protected.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c   |  2 --
 fs/btrfs/volumes.c | 10 +++++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 86e7f5abd740..52289cc661f0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2648,7 +2648,6 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags))
 		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 
-	mutex_lock(&fs_info->volume_mutex);
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
 		ret = PTR_ERR(vol_args);
@@ -2663,7 +2662,6 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 
 	kfree(vol_args);
 out:
-	mutex_unlock(&fs_info->volume_mutex);
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3d5ed4518f20..727c815e9040 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2380,6 +2380,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
+	mutex_lock(&fs_info->volume_mutex);
+
 	if (fs_info->fs_devices->seeding) {
 		seeding_dev = 1;
 		down_write(&sb->s_umount);
@@ -2529,8 +2531,11 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		up_write(&sb->s_umount);
 		unlocked = true;
 
-		if (ret) /* transaction commit */
+		if (ret) {
+			mutex_unlock(&fs_info->volume_mutex);
+			/* transaction commit */
 			return ret;
+		}
 
 		ret = btrfs_relocate_sys_chunks(fs_info);
 		if (ret < 0)
@@ -2547,6 +2552,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		ret = btrfs_commit_transaction(trans);
 	}
 
+	mutex_unlock(&fs_info->volume_mutex);
+
 	/* Update ctime/mtime for libblkid */
 	update_dev_time(device_path);
 	return ret;
@@ -2566,6 +2573,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		mutex_unlock(&uuid_mutex);
 		up_write(&sb->s_umount);
 	}
+	mutex_unlock(&fs_info->volume_mutex);
 	return ret;
 }
 
-- 
2.13.1

[PATCH 7/7] btrfs: rename btrfs_init_new_device() to btrfs_add_device()

[Anand Jain]

No functional changes this patch renames btrfs_init_new_device()
to btrfs_add_device(), so that it matches to the btrfs_rm_device()
naming. And further it will help to further cleanup device init part
from btrfs_add_device() and btrfs_init_dev_replace().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c   | 2 +-
 fs/btrfs/volumes.c | 2 +-
 fs/btrfs/volumes.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 52289cc661f0..5df8538fc47c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2655,7 +2655,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 	}
 
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
-	ret = btrfs_init_new_device(fs_info, vol_args->name);
+	ret = btrfs_add_device(fs_info, vol_args->name);
 
 	if (!ret)
 		btrfs_info(fs_info, "disk added %s", vol_args->name);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 727c815e9040..14f36d4844f0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2357,7 +2357,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path)
+int btrfs_add_device(struct btrfs_fs_info *fs_info, const char *device_path)
 {
 	struct btrfs_root *root = fs_info->dev_root;
 	struct request_queue *q;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index b219f24f1c07..9d19812f5ba7 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -444,7 +444,7 @@ int btrfs_grow_device(struct btrfs_trans_handle *trans,
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_info *fs_info, u64 devid,
 				       u8 *uuid, u8 *fsid);
 int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
-int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
+int btrfs_add_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 				  const char *device_path,
 				  struct btrfs_device *srcdev,
-- 
2.13.1