[RFC PATCH 00/20] loop: cleanup and small improvement

[RFC PATCH 00/20] loop: cleanup and small improvement

[Chaitanya Kulkarni]

Hi,

In the current loop driver, there are different coding style present
which creates confusion when adding new code e.g. variable declaration,
function parameter spacing, switch case break vs return, setting up
error code before the error condition when error code is not shared,
structure member alignment, inconsistent variable declaration style,
declaring the same variable in two switch cases, using extra variable
instead of direclty accessing structure member for one occurance in the
code etc. 

This patch-series tries to fix that and also adds some improvements such
as adding lockdep assert, reducing the memset count, using snprintf etc.

Marking it RFC since I'm sure if these little fixes are welcome or not
as code works fine without it. On confirmation I'll spend more time
building a patch-series with an an extensive test report.

Hopefully unlike my previous series this will not end up twice on the
mailing list.

-ck

Chaitanya Kulkarni (20):
  loop: use uniform alignment for struct members
  loop: add lockdep assert in loop_add()
  loop: add lockdep assert in loop_lookup()
  loop: allow user to set the queue depth
  loop: use snprintf for XXX_show()
  loop: add newline after variable declaration
  loop: use uniform variable declaration style
  loop: use uniform function header style
  loop: remove extra variable in lo_fallocate()
  loop: remove extra variable in lo_req_flush
  loop: remove local variable in lo_compat_ioctl
  loop: cleanup lo_ioctl()
  loop: remove memset in info64 to compat
  loop: remove memset in info64 from compat
  loop: remove memset in loop_info64_from_old()
  loop: remove memset in loop_info64_to_old()
  loop: change fd set err at actual error condition
  loop: configure set err at actual error condition
  loop: set error value in case of actual error
  loop: remove the extra line in declaration

 drivers/block/loop.c | 253 ++++++++++++++++++++-----------------------
 drivers/block/loop.h |  26 ++---
 2 files changed, 132 insertions(+), 147 deletions(-)

-- 
2.22.1

[RFC PATCH 01/20] loop: use uniform alignment for struct members

[Chaitanya Kulkarni]

Use uniform alignment and tab spaceing for the struct members. Also,
add bdev identifire name for function pointer to turnoff checkpatch
warning.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index a3c04f310672..638642bfc76d 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -41,16 +41,16 @@ struct loop_device {
 	char		lo_encrypt_key[LO_KEY_SIZE];
 	int		lo_encrypt_key_size;
 	struct loop_func_table *lo_encryption;
-	__u32           lo_init[2];
+	__u32		lo_init[2];
 	kuid_t		lo_key_owner;	/* Who set the key */
-	int		(*ioctl)(struct loop_device *, int cmd, 
+	int		(*ioctl)(struct loop_device *bdev, int cmd,
 				 unsigned long arg); 
 
-	struct file *	lo_backing_file;
-	struct block_device *lo_device;
-	void		*key_data; 
+	struct file		*lo_backing_file;
+	struct block_device	*lo_device;
+	void			*key_data;
 
-	gfp_t		old_gfp_mask;
+	gfp_t			old_gfp_mask;
 
 	spinlock_t		lo_lock;
 	int			lo_state;
@@ -66,13 +66,13 @@ struct loop_device {
 };
 
 struct loop_cmd {
-	struct kthread_work work;
-	bool use_aio; /* use AIO interface to handle I/O */
-	atomic_t ref; /* only for aio */
-	long ret;
-	struct kiocb iocb;
-	struct bio_vec *bvec;
-	struct cgroup_subsys_state *css;
+	struct kthread_work		work;
+	bool				use_aio; /* use asynchronous I/O */
+	atomic_t			ref; /* only for aio */
+	long				ret;
+	struct kiocb			iocb;
+	struct bio_vec			*bvec;
+	struct cgroup_subsys_state	*css;
 };
 
 /* Support for loadable transfer modules */
-- 
2.22.1

[RFC PATCH 02/20] loop: add lockdep assert in loop_add()

[Chaitanya Kulkarni]

In current code loop_add() is called from loop_probe(),
loop_control_ioctl(), and loop_init(). Each of these functions hold
global mutex lock loop_ctrl_mutex with two variants mutex_lock() and
mutex_lock_killable().

Add lockdep_asset_lock_held() in loop_add() to make sure it will
generate an appropriate error message in case of misuse.  

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 578fc034db3f..3bfdcabaf6b1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2081,6 +2081,8 @@ static int loop_add(struct loop_device **l, int i)
 	struct gendisk *disk;
 	int err;
 
+	lockdep_assert_held(&loop_ctl_mutex);
+
 	err = -ENOMEM;
 	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
 	if (!lo)
-- 
2.22.1

[RFC PATCH 03/20] loop: add lockdep assert in loop_lookup()

[Chaitanya Kulkarni]

In current code loop_lookup() is called from loop_probe(),
and loop_control_ioctl(). Each of these functions hold global
mutex lock loop_ctrl_mutex with two variants mutex_lock() and
mutex_lock_killable().

Add lockdep_asset_lock_held() in loop_lookup() to make sure it will
generate an appropriate error message in case of misuse.  

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3bfdcabaf6b1..0a8cee66c622 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2211,6 +2211,8 @@ static int loop_lookup(struct loop_device **l, int i)
 	struct loop_device *lo;
 	int ret = -ENODEV;
 
+	lockdep_assert_held(&loop_ctl_mutex);
+
 	if (i < 0) {
 		int err;
 
-- 
2.22.1

[RFC PATCH 04/20] loop: allow user to set the queue depth

[Chaitanya Kulkarni]

Instead of hardcoding queue depth allow user to set the hw queue depth
using module parameter. Set default value to 128 to retain the existing
behavior.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0a8cee66c622..cf2789f7e6ba 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1950,6 +1950,9 @@ module_param(max_loop, int, 0444);
 MODULE_PARM_DESC(max_loop, "Maximum number of loop devices");
 module_param(max_part, int, 0444);
 MODULE_PARM_DESC(max_part, "Maximum number of partitions per loop device");
+static int hw_queue_depth = 128;
+module_param_named(hw_queue_depth, hw_queue_depth, int, 0444);
+MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
 
@@ -2105,7 +2108,7 @@ static int loop_add(struct loop_device **l, int i)
 	err = -ENOMEM;
 	lo->tag_set.ops = &loop_mq_ops;
 	lo->tag_set.nr_hw_queues = 1;
-	lo->tag_set.queue_depth = 128;
+	lo->tag_set.queue_depth = hw_queue_depth;
 	lo->tag_set.numa_node = NUMA_NO_NODE;
 	lo->tag_set.cmd_size = sizeof(struct loop_cmd);
 	lo->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_STACKING;
-- 
2.22.1

[RFC PATCH 05/20] loop: use snprintf for XXX_show()

[Chaitanya Kulkarni]

Use snprintf() over sprintf() which allows us to limit the target
buffer size.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cf2789f7e6ba..3f8d9bdfc16e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -806,33 +806,35 @@ static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
 
 static ssize_t loop_attr_offset_show(struct loop_device *lo, char *buf)
 {
-	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_offset);
+	return snprintf(buf, PAGE_SIZE,
+			"%llu\n", (unsigned long long)lo->lo_offset);
 }
 
 static ssize_t loop_attr_sizelimit_show(struct loop_device *lo, char *buf)
 {
-	return sprintf(buf, "%llu\n", (unsigned long long)lo->lo_sizelimit);
+	return snprintf(buf, PAGE_SIZE,
+			"%llu\n", (unsigned long long)lo->lo_sizelimit);
 }
 
 static ssize_t loop_attr_autoclear_show(struct loop_device *lo, char *buf)
 {
 	int autoclear = (lo->lo_flags & LO_FLAGS_AUTOCLEAR);
 
-	return sprintf(buf, "%s\n", autoclear ? "1" : "0");
+	return snprintf(buf, PAGE_SIZE, "%s\n", autoclear ? "1" : "0");
 }
 
 static ssize_t loop_attr_partscan_show(struct loop_device *lo, char *buf)
 {
 	int partscan = (lo->lo_flags & LO_FLAGS_PARTSCAN);
 
-	return sprintf(buf, "%s\n", partscan ? "1" : "0");
+	return snprintf(buf, PAGE_SIZE, "%s\n", partscan ? "1" : "0");
 }
 
 static ssize_t loop_attr_dio_show(struct loop_device *lo, char *buf)
 {
 	int dio = (lo->lo_flags & LO_FLAGS_DIRECT_IO);
 
-	return sprintf(buf, "%s\n", dio ? "1" : "0");
+	return snprintf(buf, PAGE_SIZE, "%s\n", dio ? "1" : "0");
 }
 
 LOOP_ATTR_RO(backing_file);
-- 
2.22.1

Re: [RFC PATCH 05/20] loop: use snprintf for XXX_show()

[Johannes Thumshirn]

On 02/02/2021 06:39, Chaitanya Kulkarni wrote:
> Use snprintf() over sprintf() which allows us to limit the target
> buffer size.

Why not sysfs_emit()?

Re: [RFC PATCH 05/20] loop: use snprintf for XXX_show()

[Chaitanya Kulkarni]

On 2/2/21 03:05, Johannes Thumshirn wrote:
> On 02/02/2021 06:39, Chaitanya Kulkarni wrote:
>> Use snprintf() over sprintf() which allows us to limit the target
>> buffer size.
> Why not sysfs_emit()?
>
Sure, will add to V1.

[RFC PATCH 06/20] loop: add newline after variable declaration

[Chaitanya Kulkarni]

Add a new line after variable declaration at the start of the function
just like we have in the block layer code.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3f8d9bdfc16e..9e59adfd11ff 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -457,6 +457,7 @@ static int lo_req_flush(struct loop_device *lo, struct request *rq)
 {
 	struct file *file = lo->lo_backing_file;
 	int ret = vfs_fsync(file, 0);
+
 	if (unlikely(ret && ret != -EINVAL))
 		ret = -EIO;
 
@@ -1584,6 +1585,7 @@ static int loop_set_capacity(struct loop_device *lo)
 static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 {
 	int error = -ENXIO;
+
 	if (lo->lo_state != Lo_bound)
 		goto out;
 
-- 
2.22.1

[RFC PATCH 07/20] loop: use uniform variable declaration style

[Chaitanya Kulkarni]

The mixing of two different styles of variable declaration cretes
confusion for the new developer when adding the code. since this is
a block driver use standard variable declaration present in the block
layer.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 9e59adfd11ff..c4458b3f1dab 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -669,8 +669,8 @@ static inline int is_loop_device(struct file *file)
 
 static int loop_validate_file(struct file *file, struct block_device *bdev)
 {
-	struct inode	*inode = file->f_mapping->host;
-	struct file	*f = file;
+	struct inode *inode = file->f_mapping->host;
+	struct file *f = file;
 
 	/* Avoid recursion */
 	while (is_loop_device(f)) {
@@ -701,9 +701,9 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
 static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 			  unsigned int arg)
 {
-	struct file	*file = NULL, *old_file;
-	int		error;
-	bool		partscan;
+	struct file *file = NULL, *old_file;
+	bool partscan;
+	int error;
 
 	error = mutex_lock_killable(&lo->lo_mutex);
 	if (error)
@@ -1069,13 +1069,13 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 			  struct block_device *bdev,
 			  const struct loop_config *config)
 {
-	struct file	*file;
-	struct inode	*inode;
+	struct file *file;
+	struct inode *inode;
 	struct address_space *mapping;
-	int		error;
-	loff_t		size;
-	bool		partscan;
-	unsigned short  bsize;
+	int error;
+	loff_t size;
+	bool partscan;
+	unsigned short bsize;
 
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
-- 
2.22.1

[RFC PATCH 08/20] loop: use uniform function header style

[Chaitanya Kulkarni]

There are two different styles present in the code right now, that
creates a confusion for a new developer on which style to follow. Since
this is a block driver follow common coding style for the function
header than having return type on the separate line and align the
function parameters to the end of the function name.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 85 +++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c4458b3f1dab..b0a8d5b0641f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -232,8 +232,7 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
  * loop_validate_block_size() - validates the passed in block size
  * @bsize: size to validate
  */
-static int
-loop_validate_block_size(unsigned short bsize)
+static int loop_validate_block_size(unsigned short bsize)
 {
 	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
 		return -EINVAL;
@@ -255,11 +254,10 @@ static void loop_set_size(struct loop_device *lo, loff_t size)
 		kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 }
 
-static inline int
-lo_do_transfer(struct loop_device *lo, int cmd,
-	       struct page *rpage, unsigned roffs,
-	       struct page *lpage, unsigned loffs,
-	       int size, sector_t rblock)
+static inline int lo_do_transfer(struct loop_device *lo, int cmd,
+				 struct page *rpage, unsigned roffs,
+				 struct page *lpage, unsigned loffs,
+				 int size, sector_t rblock)
 {
 	int ret;
 
@@ -296,7 +294,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
 }
 
 static int lo_write_simple(struct loop_device *lo, struct request *rq,
-		loff_t pos)
+			   loff_t pos)
 {
 	struct bio_vec bvec;
 	struct req_iterator iter;
@@ -318,7 +316,7 @@ static int lo_write_simple(struct loop_device *lo, struct request *rq,
  * access to the destination pages of the backing file.
  */
 static int lo_write_transfer(struct loop_device *lo, struct request *rq,
-		loff_t pos)
+			     loff_t pos)
 {
 	struct bio_vec bvec, b;
 	struct req_iterator iter;
@@ -348,7 +346,7 @@ static int lo_write_transfer(struct loop_device *lo, struct request *rq,
 }
 
 static int lo_read_simple(struct loop_device *lo, struct request *rq,
-		loff_t pos)
+			  loff_t pos)
 {
 	struct bio_vec bvec;
 	struct req_iterator iter;
@@ -377,7 +375,7 @@ static int lo_read_simple(struct loop_device *lo, struct request *rq,
 }
 
 static int lo_read_transfer(struct loop_device *lo, struct request *rq,
-		loff_t pos)
+			    loff_t pos)
 {
 	struct bio_vec bvec, b;
 	struct req_iterator iter;
@@ -964,8 +962,7 @@ static void loop_update_rotational(struct loop_device *lo)
 		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
 }
 
-static int
-loop_release_xfer(struct loop_device *lo)
+static int loop_release_xfer(struct loop_device *lo)
 {
 	int err = 0;
 	struct loop_func_table *xfer = lo->lo_encryption;
@@ -980,9 +977,8 @@ loop_release_xfer(struct loop_device *lo)
 	return err;
 }
 
-static int
-loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
-	       const struct loop_info64 *i)
+static int loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
+			  const struct loop_info64 *i)
 {
 	int err = 0;
 
@@ -1009,9 +1005,8 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
  * Configures the loop device parameters according to the passed
  * in loop_info64 configuration.
  */
-static int
-loop_set_status_from_info(struct loop_device *lo,
-			  const struct loop_info64 *info)
+static int loop_set_status_from_info(struct loop_device *lo,
+				     const struct loop_info64 *info)
 {
 	int err;
 	struct loop_func_table *xfer;
@@ -1336,8 +1331,8 @@ static int loop_clr_fd(struct loop_device *lo)
 	return __loop_clr_fd(lo, false);
 }
 
-static int
-loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+static int loop_set_status(struct loop_device *lo,
+			   const struct loop_info64 *info)
 {
 	int err;
 	struct block_device *bdev;
@@ -1420,8 +1415,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	return err;
 }
 
-static int
-loop_get_status(struct loop_device *lo, struct loop_info64 *info)
+static int loop_get_status(struct loop_device *lo,
+			   struct loop_info64 *info)
 {
 	struct path path;
 	struct kstat stat;
@@ -1464,8 +1459,8 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info)
 	return ret;
 }
 
-static void
-loop_info64_from_old(const struct loop_info *info, struct loop_info64 *info64)
+static void loop_info64_from_old(const struct loop_info *info,
+				 struct loop_info64 *info64)
 {
 	memset(info64, 0, sizeof(*info64));
 	info64->lo_number = info->lo_number;
@@ -1486,8 +1481,8 @@ loop_info64_from_old(const struct loop_info *info, struct loop_info64 *info64)
 	memcpy(info64->lo_encrypt_key, info->lo_encrypt_key, LO_KEY_SIZE);
 }
 
-static int
-loop_info64_to_old(const struct loop_info64 *info64, struct loop_info *info)
+static int loop_info64_to_old(const struct loop_info64 *info64,
+			      struct loop_info *info)
 {
 	memset(info, 0, sizeof(*info));
 	info->lo_number = info64->lo_number;
@@ -1516,8 +1511,8 @@ loop_info64_to_old(const struct loop_info64 *info64, struct loop_info *info)
 	return 0;
 }
 
-static int
-loop_set_status_old(struct loop_device *lo, const struct loop_info __user *arg)
+static int loop_set_status_old(struct loop_device *lo,
+			       const struct loop_info __user *arg)
 {
 	struct loop_info info;
 	struct loop_info64 info64;
@@ -1528,8 +1523,8 @@ loop_set_status_old(struct loop_device *lo, const struct loop_info __user *arg)
 	return loop_set_status(lo, &info64);
 }
 
-static int
-loop_set_status64(struct loop_device *lo, const struct loop_info64 __user *arg)
+static int loop_set_status64(struct loop_device *lo,
+			     const struct loop_info64 __user *arg)
 {
 	struct loop_info64 info64;
 
@@ -1538,8 +1533,9 @@ loop_set_status64(struct loop_device *lo, const struct loop_info64 __user *arg)
 	return loop_set_status(lo, &info64);
 }
 
-static int
-loop_get_status_old(struct loop_device *lo, struct loop_info __user *arg) {
+static int loop_get_status_old(struct loop_device *lo,
+			       struct loop_info __user *arg)
+{
 	struct loop_info info;
 	struct loop_info64 info64;
 	int err;
@@ -1555,8 +1551,9 @@ loop_get_status_old(struct loop_device *lo, struct loop_info __user *arg) {
 	return err;
 }
 
-static int
-loop_get_status64(struct loop_device *lo, struct loop_info64 __user *arg) {
+static int loop_get_status64(struct loop_device *lo,
+			     struct loop_info64 __user *arg)
+{
 	struct loop_info64 info64;
 	int err;
 
@@ -1660,8 +1657,8 @@ static int lo_simple_ioctl(struct loop_device *lo, unsigned int cmd,
 	return err;
 }
 
-static int lo_ioctl(struct block_device *bdev, fmode_t mode,
-	unsigned int cmd, unsigned long arg)
+static int lo_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
+		    unsigned long arg)
 {
 	struct loop_device *lo = bdev->bd_disk->private_data;
 	void __user *argp = (void __user *) arg;
@@ -1813,9 +1810,8 @@ loop_info64_to_compat(const struct loop_info64 *info64,
 	return 0;
 }
 
-static int
-loop_set_status_compat(struct loop_device *lo,
-		       const struct compat_loop_info __user *arg)
+static int loop_set_status_compat(struct loop_device *lo,
+				 const struct compat_loop_info __user *arg)
 {
 	struct loop_info64 info64;
 	int ret;
@@ -1826,9 +1822,8 @@ loop_set_status_compat(struct loop_device *lo,
 	return loop_set_status(lo, &info64);
 }
 
-static int
-loop_get_status_compat(struct loop_device *lo,
-		       struct compat_loop_info __user *arg)
+static int loop_get_status_compat(struct loop_device *lo,
+				  struct compat_loop_info __user *arg)
 {
 	struct loop_info64 info64;
 	int err;
@@ -1999,7 +1994,7 @@ EXPORT_SYMBOL(loop_register_transfer);
 EXPORT_SYMBOL(loop_unregister_transfer);
 
 static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
-		const struct blk_mq_queue_data *bd)
+				  const struct blk_mq_queue_data *bd)
 {
 	struct request *rq = bd->rq;
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
@@ -2068,7 +2063,7 @@ static void loop_queue_work(struct kthread_work *work)
 }
 
 static int loop_init_request(struct blk_mq_tag_set *set, struct request *rq,
-		unsigned int hctx_idx, unsigned int numa_node)
+			     unsigned int hctx_idx, unsigned int numa_node)
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
 
-- 
2.22.1

[RFC PATCH 09/20] loop: remove extra variable in lo_fallocate()

[Chaitanya Kulkarni]

The local variable q is used to pass it to the blk_queue_discard(). We
can get away with using lo->lo_queue instead of storing in a local
variable which is not used anywhere else.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b0a8d5b0641f..f9f352c7a56f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -434,12 +434,11 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
 	 * information.
 	 */
 	struct file *file = lo->lo_backing_file;
-	struct request_queue *q = lo->lo_queue;
 	int ret;
 
 	mode |= FALLOC_FL_KEEP_SIZE;
 
-	if (!blk_queue_discard(q)) {
+	if (!blk_queue_discard(lo->lo_queue)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.22.1

Re: [RFC PATCH 09/20] loop: remove extra variable in lo_fallocate()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 10/20] loop: remove extra variable in lo_req_flush

[Chaitanya Kulkarni]

The local variable file is used to pass it to the vfs_fsync(). We can
get away with using lo->lo_backing_file instead of storing in a local
variable which is not used anywhere else.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f9f352c7a56f..b8028c6d5ecc 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -452,8 +452,7 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
 
 static int lo_req_flush(struct loop_device *lo, struct request *rq)
 {
-	struct file *file = lo->lo_backing_file;
-	int ret = vfs_fsync(file, 0);
+	int ret = vfs_fsync(lo->lo_backing_file, 0);
 
 	if (unlikely(ret && ret != -EINVAL))
 		ret = -EIO;
-- 
2.22.1

Re: [RFC PATCH 10/20] loop: remove extra variable in lo_req_flush

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 11/20] loop: remove local variable in lo_compat_ioctl

[Chaitanya Kulkarni]

Instead of storing the return values into the err variable just return
the err from switch cases, since we don't do anything after switch with
that error but return. This also removes the need for the local
variable err in lo_compat_ioctl().

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b8028c6d5ecc..d9cd0ac3d947 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1838,17 +1838,14 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 			   unsigned int cmd, unsigned long arg)
 {
 	struct loop_device *lo = bdev->bd_disk->private_data;
-	int err;
 
 	switch(cmd) {
 	case LOOP_SET_STATUS:
-		err = loop_set_status_compat(lo,
-			     (const struct compat_loop_info __user *)arg);
-		break;
+		return loop_set_status_compat(lo,
+				(const struct compat_loop_info __user *)arg);
 	case LOOP_GET_STATUS:
-		err = loop_get_status_compat(lo,
-				     (struct compat_loop_info __user *)arg);
-		break;
+		return loop_get_status_compat(lo,
+				(struct compat_loop_info __user *)arg);
 	case LOOP_SET_CAPACITY:
 	case LOOP_CLR_FD:
 	case LOOP_GET_STATUS64:
@@ -1860,13 +1857,10 @@ static int lo_compat_ioctl(struct block_device *bdev, fmode_t mode,
 	case LOOP_CHANGE_FD:
 	case LOOP_SET_BLOCK_SIZE:
 	case LOOP_SET_DIRECT_IO:
-		err = lo_ioctl(bdev, mode, cmd, arg);
-		break;
+		return lo_ioctl(bdev, mode, cmd, arg);
 	default:
-		err = -ENOIOCTLCMD;
-		break;
+		return  -ENOIOCTLCMD;
 	}
-	return err;
 }
 #endif
 
-- 
2.22.1

Re: [RFC PATCH 11/20] loop: remove local variable in lo_compat_ioctl

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 12/20] loop: cleanup lo_ioctl()

[Chaitanya Kulkarni]

Instead of storing the return values into the err variable just return
the err from switch cases, since we don't do anything after switch with
that error but return. This also removes the need for the local
variable err in lo_ioctl(). Instead of declaring config variable twice
in the set_fd and configire switch cases declare and initialize the loop
config variabel that removes the need for memset. Also, move status64
switch case near to set status case.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d9cd0ac3d947..bc074ad00eaf 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1660,48 +1660,35 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
 {
 	struct loop_device *lo = bdev->bd_disk->private_data;
 	void __user *argp = (void __user *) arg;
-	int err;
+	struct loop_config config = { };
 
 	switch (cmd) {
-	case LOOP_SET_FD: {
+	case LOOP_SET_FD:
 		/*
 		 * Legacy case - pass in a zeroed out struct loop_config with
 		 * only the file descriptor set , which corresponds with the
 		 * default parameters we'd have used otherwise.
 		 */
-		struct loop_config config;
-
-		memset(&config, 0, sizeof(config));
 		config.fd = arg;
-
 		return loop_configure(lo, mode, bdev, &config);
-	}
-	case LOOP_CONFIGURE: {
-		struct loop_config config;
-
+	case LOOP_CONFIGURE:
 		if (copy_from_user(&config, argp, sizeof(config)))
 			return -EFAULT;
-
 		return loop_configure(lo, mode, bdev, &config);
-	}
 	case LOOP_CHANGE_FD:
 		return loop_change_fd(lo, bdev, arg);
 	case LOOP_CLR_FD:
 		return loop_clr_fd(lo);
 	case LOOP_SET_STATUS:
-		err = -EPERM;
-		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-			err = loop_set_status_old(lo, argp);
-		}
-		break;
+		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
+			return loop_set_status_old(lo, argp);
+		return -EPERM;
+	case LOOP_SET_STATUS64:
+		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
+			return loop_set_status64(lo, argp);
+		return -EPERM;
 	case LOOP_GET_STATUS:
 		return loop_get_status_old(lo, argp);
-	case LOOP_SET_STATUS64:
-		err = -EPERM;
-		if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
-			err = loop_set_status64(lo, argp);
-		}
-		break;
 	case LOOP_GET_STATUS64:
 		return loop_get_status64(lo, argp);
 	case LOOP_SET_CAPACITY:
@@ -1711,11 +1698,8 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
 			return -EPERM;
 		fallthrough;
 	default:
-		err = lo_simple_ioctl(lo, cmd, arg);
-		break;
+		return lo_simple_ioctl(lo, cmd, arg);
 	}
-
-	return err;
 }
 
 #ifdef CONFIG_COMPAT
-- 
2.22.1

[RFC PATCH 13/20] loop: remove memset in info64 to compat

[Chaitanya Kulkarni]

Initialize the local variable info to zero at the time of declataion.
this also remove the need for the memset().

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index bc074ad00eaf..667a3945bf39 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1759,9 +1759,8 @@ static noinline int
 loop_info64_to_compat(const struct loop_info64 *info64,
 		      struct compat_loop_info __user *arg)
 {
-	struct compat_loop_info info;
+	struct compat_loop_info info = { };
 
-	memset(&info, 0, sizeof(info));
 	info.lo_number = info64->lo_number;
 	info.lo_device = info64->lo_device;
 	info.lo_inode = info64->lo_inode;
-- 
2.22.1

Re: [RFC PATCH 13/20] loop: remove memset in info64 to compat

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 14/20] loop: remove memset in info64 from compat

[Chaitanya Kulkarni]

In current code loop_info64_from_compat() is only called from the one
caller loop_set_status_compat(). Initialize the info64 local variable
to zero before we pass it to the loop_info64_compat() so that we can get
rid of the memset in the loop_info64_from_compat().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 667a3945bf39..2029ca399ea3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1731,7 +1731,6 @@ loop_info64_from_compat(const struct compat_loop_info __user *arg,
 	if (copy_from_user(&info, arg, sizeof(info)))
 		return -EFAULT;
 
-	memset(info64, 0, sizeof(*info64));
 	info64->lo_number = info.lo_number;
 	info64->lo_device = info.lo_device;
 	info64->lo_inode = info.lo_inode;
@@ -1794,7 +1793,7 @@ loop_info64_to_compat(const struct loop_info64 *info64,
 static int loop_set_status_compat(struct loop_device *lo,
 				 const struct compat_loop_info __user *arg)
 {
-	struct loop_info64 info64;
+	struct loop_info64 info64 = { };
 	int ret;
 
 	ret = loop_info64_from_compat(arg, &info64);
-- 
2.22.1

Re: [RFC PATCH 14/20] loop: remove memset in info64 from compat

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 15/20] loop: remove memset in loop_info64_from_old()

[Chaitanya Kulkarni]

In current code loop_info64_from_old() is only called from the one
caller loop_set_status_old(). Initialize the info64 local variable to
zero before we pass it to the loop_info64_from_old() so that we can get
rid of the memset in the loop_info64_from_old().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 2029ca399ea3..62f622791fb5 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1460,7 +1460,6 @@ static int loop_get_status(struct loop_device *lo,
 static void loop_info64_from_old(const struct loop_info *info,
 				 struct loop_info64 *info64)
 {
-	memset(info64, 0, sizeof(*info64));
 	info64->lo_number = info->lo_number;
 	info64->lo_device = info->lo_device;
 	info64->lo_inode = info->lo_inode;
@@ -1513,7 +1512,7 @@ static int loop_set_status_old(struct loop_device *lo,
 			       const struct loop_info __user *arg)
 {
 	struct loop_info info;
-	struct loop_info64 info64;
+	struct loop_info64 info64 = { };
 
 	if (copy_from_user(&info, arg, sizeof (struct loop_info)))
 		return -EFAULT;
-- 
2.22.1

Re: [RFC PATCH 15/20] loop: remove memset in loop_info64_from_old()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 16/20] loop: remove memset in loop_info64_to_old()

[Chaitanya Kulkarni]

In current code loop_info64_to_old() is only called from the one caller
loop_get_status_old(). Initialize the info local variable to zero
before we pass it to the loop_info64_from_old() so that we can get rid
of the memset in the loop_info64_to_old().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 62f622791fb5..af3e3bcd564d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1481,7 +1481,6 @@ static void loop_info64_from_old(const struct loop_info *info,
 static int loop_info64_to_old(const struct loop_info64 *info64,
 			      struct loop_info *info)
 {
-	memset(info, 0, sizeof(*info));
 	info->lo_number = info64->lo_number;
 	info->lo_device = info64->lo_device;
 	info->lo_inode = info64->lo_inode;
@@ -1533,7 +1532,7 @@ static int loop_set_status64(struct loop_device *lo,
 static int loop_get_status_old(struct loop_device *lo,
 			       struct loop_info __user *arg)
 {
-	struct loop_info info;
+	struct loop_info info = { };
 	struct loop_info64 info64;
 	int err;
 
-- 
2.22.1

Re: [RFC PATCH 16/20] loop: remove memset in loop_info64_to_old()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[RFC PATCH 17/20] loop: change fd set err at actual error condition

[Chaitanya Kulkarni]

The function loop_change_fd() set error = -ENXIO before checking loop
state, error = -EINVAL before checking loop flags, error = -EBADF
before calling fget and error = -EINVAL before comparing the old file
size to the new file file. None of these error values are reused for
the above conditions.

Conditionally set the error after we actually know that error condition
is true instead of setting it before the if check.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index af3e3bcd564d..89f9c73bb2af 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -704,19 +704,22 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	error = mutex_lock_killable(&lo->lo_mutex);
 	if (error)
 		return error;
-	error = -ENXIO;
-	if (lo->lo_state != Lo_bound)
+	if (lo->lo_state != Lo_bound) {
+		error = -ENXIO;
 		goto out_err;
+	}
 
 	/* the loop device has to be read-only */
-	error = -EINVAL;
-	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
+	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY)) {
+		error = -EINVAL;
 		goto out_err;
+	}
 
-	error = -EBADF;
 	file = fget(arg);
-	if (!file)
+	if (!file) {
+		error = -EBADF;
 		goto out_err;
+	}
 
 	error = loop_validate_file(file, bdev);
 	if (error)
@@ -724,11 +727,11 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 
 	old_file = lo->lo_backing_file;
 
-	error = -EINVAL;
-
 	/* size of the new backing store needs to be the same */
-	if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
+	if (get_loop_size(lo, file) != get_loop_size(lo, old_file)) {
+		error = -EINVAL;
 		goto out_err;
+	}
 
 	/* and ... switch */
 	blk_mq_freeze_queue(lo->lo_queue);
-- 
2.22.1

[RFC PATCH 18/20] loop: configure set err at actual error condition

[Chaitanya Kulkarni]

The function loop_configure() set error = -EBADF before calling fget(),
error = -EBUSY before checking the state. None of these error values are
reused for the above conditions.

Conditionally set the error after we actually know that error condition
is true instead of setting it before the if check.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 89f9c73bb2af..d99ae348e4e2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1076,10 +1076,11 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
 
-	error = -EBADF;
 	file = fget(config->fd);
-	if (!file)
+	if (!file) {
+		error = -EBADF;
 		goto out;
+	}
 
 	/*
 	 * If we don't hold exclusive handle for the device, upgrade to it
@@ -1095,9 +1096,10 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	if (error)
 		goto out_bdev;
 
-	error = -EBUSY;
-	if (lo->lo_state != Lo_unbound)
+	if (lo->lo_state != Lo_unbound) {
+		error = -EBUSY;
 		goto out_unlock;
+	}
 
 	error = loop_validate_file(file, bdev);
 	if (error)
-- 
2.22.1

[RFC PATCH 19/20] loop: set error value in case of actual error

[Chaitanya Kulkarni]

The function add loop_add() set err = -ENOMEM before the call to
kzalloc(), err = -ENOMEM before the call to alloc_disk(). None of these
error number values are shared. That requires err to be set explicitly
before actual error happens.

Conditionally set the error after we actually know that error condition
is true insted of setting it before the if check.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d99ae348e4e2..ef70795e36ab 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2062,10 +2062,11 @@ static int loop_add(struct loop_device **l, int i)
 
 	lockdep_assert_held(&loop_ctl_mutex);
 
-	err = -ENOMEM;
 	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
-	if (!lo)
+	if (!lo) {
+		err = -ENOMEM;
 		goto out;
+	}
 
 	lo->lo_state = Lo_unbound;
 
@@ -2081,7 +2082,6 @@ static int loop_add(struct loop_device **l, int i)
 		goto out_free_dev;
 	i = err;
 
-	err = -ENOMEM;
 	lo->tag_set.ops = &loop_mq_ops;
 	lo->tag_set.nr_hw_queues = 1;
 	lo->tag_set.queue_depth = hw_queue_depth;
@@ -2091,8 +2091,10 @@ static int loop_add(struct loop_device **l, int i)
 	lo->tag_set.driver_data = lo;
 
 	err = blk_mq_alloc_tag_set(&lo->tag_set);
-	if (err)
+	if (err) {
+		err = -ENOMEM;
 		goto out_free_idr;
+	}
 
 	lo->lo_queue = blk_mq_init_queue(&lo->tag_set);
 	if (IS_ERR(lo->lo_queue)) {
@@ -2111,10 +2113,11 @@ static int loop_add(struct loop_device **l, int i)
 	 */
 	blk_queue_flag_set(QUEUE_FLAG_NOMERGES, lo->lo_queue);
 
-	err = -ENOMEM;
 	disk = lo->lo_disk = alloc_disk(1 << part_shift);
-	if (!disk)
+	if (!disk) {
+		err = -ENOMEM;
 		goto out_free_queue;
+	}
 
 	/*
 	 * Disable partition scanning by default. The in-kernel partition
-- 
2.22.1

[RFC PATCH 20/20] loop: remove the extra line in declaration

[Chaitanya Kulkarni]

The initialization line fits into the one line nicely no need to break
it to the next line.

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ef70795e36ab..e691a8c6b04a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2033,8 +2033,7 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
 
 static void loop_queue_work(struct kthread_work *work)
 {
-	struct loop_cmd *cmd =
-		container_of(work, struct loop_cmd, work);
+	struct loop_cmd *cmd = container_of(work, struct loop_cmd, work);
 
 	loop_handle_cmd(cmd);
 }
-- 
2.22.1

Re: [RFC PATCH 20/20] loop: remove the extra line in declaration

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>