loop cleanups v2

loop cleanups v2

[Christoph Hellwig]

Hi Jens,

this series contains a bunch of cleanups for the loop driver,
mostly related to probing and the control device.

Changes since v1:
 - use local variables instead of dereferencing struct loop_device
   outside loop_ctl_mutex

[PATCH 1/9] loop: reorder loop_exit

[Christoph Hellwig]

Unregister the misc and blockdevice first to prevent further access,
and only then iterate to remove the devices.

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

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 081eb9aaeba8..44fa27c54ac2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2381,14 +2381,11 @@ static int loop_exit_cb(int id, void *ptr, void *data)
 static void __exit loop_exit(void)
 {
 	mutex_lock(&loop_ctl_mutex);
-
-	idr_for_each(&loop_index_idr, &loop_exit_cb, NULL);
-	idr_destroy(&loop_index_idr);
-
 	unregister_blkdev(LOOP_MAJOR, "loop");
-
 	misc_deregister(&loop_misc);
 
+	idr_for_each(&loop_index_idr, &loop_exit_cb, NULL);
+	idr_destroy(&loop_index_idr);
 	mutex_unlock(&loop_ctl_mutex);
 }
 
-- 
2.30.2

[PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit

[Christoph Hellwig]

loop_ctl_mutex is only needed to iterate the IDR for removing the loop
devices, so reduce the coverage.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 44fa27c54ac2..9df9fb490f87 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2380,13 +2380,14 @@ static int loop_exit_cb(int id, void *ptr, void *data)
 
 static void __exit loop_exit(void)
 {
-	mutex_lock(&loop_ctl_mutex);
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	misc_deregister(&loop_misc);
 
+	mutex_lock(&loop_ctl_mutex);
 	idr_for_each(&loop_index_idr, &loop_exit_cb, NULL);
-	idr_destroy(&loop_index_idr);
 	mutex_unlock(&loop_ctl_mutex);
+
+	idr_destroy(&loop_index_idr);
 }
 
 module_init(loop_init);
-- 
2.30.2

[PATCH 3/9] loop: remove the l argument to loop_add

[Christoph Hellwig]

None of the callers cares about the allocated struct loop_device.

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

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 9df9fb490f87..8392722d0e12 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2069,7 +2069,7 @@ static const struct blk_mq_ops loop_mq_ops = {
 	.complete	= lo_complete_rq,
 };
 
-static int loop_add(struct loop_device **l, int i)
+static int loop_add(int i)
 {
 	struct loop_device *lo;
 	struct gendisk *disk;
@@ -2157,7 +2157,6 @@ static int loop_add(struct loop_device **l, int i)
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
 	add_disk(disk);
-	*l = lo;
 	return lo->lo_number;
 
 out_cleanup_tags:
@@ -2227,7 +2226,7 @@ static void loop_probe(dev_t dev)
 
 	mutex_lock(&loop_ctl_mutex);
 	if (loop_lookup(&lo, idx) < 0)
-		loop_add(&lo, idx);
+		loop_add(idx);
 	mutex_unlock(&loop_ctl_mutex);
 }
 
@@ -2249,7 +2248,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 			ret = -EEXIST;
 			break;
 		}
-		ret = loop_add(&lo, parm);
+		ret = loop_add(parm);
 		break;
 	case LOOP_CTL_REMOVE:
 		ret = loop_lookup(&lo, parm);
@@ -2277,7 +2276,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 		ret = loop_lookup(&lo, -1);
 		if (ret >= 0)
 			break;
-		ret = loop_add(&lo, -1);
+		ret = loop_add(-1);
 	}
 	mutex_unlock(&loop_ctl_mutex);
 
@@ -2304,7 +2303,6 @@ MODULE_ALIAS("devname:loop-control");
 static int __init loop_init(void)
 {
 	int i, nr;
-	struct loop_device *lo;
 	int err;
 
 	part_shift = 0;
@@ -2358,7 +2356,7 @@ static int __init loop_init(void)
 	/* pre-create number of devices given by config or max_loop */
 	mutex_lock(&loop_ctl_mutex);
 	for (i = 0; i < nr; i++)
-		loop_add(&lo, i);
+		loop_add(i);
 	mutex_unlock(&loop_ctl_mutex);
 
 	printk(KERN_INFO "loop: module loaded\n");
-- 
2.30.2

[PATCH 4/9] loop: don't call loop_lookup before adding a loop device

[Christoph Hellwig]

loop_add returns the right error if the slot wasn't available.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8392722d0e12..b1e7e420563b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2219,14 +2219,12 @@ static int loop_lookup(struct loop_device **l, int i)
 static void loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
-	struct loop_device *lo;
 
 	if (max_loop && idx >= max_loop)
 		return;
 
 	mutex_lock(&loop_ctl_mutex);
-	if (loop_lookup(&lo, idx) < 0)
-		loop_add(idx);
+	loop_add(idx);
 	mutex_unlock(&loop_ctl_mutex);
 }
 
@@ -2243,11 +2241,6 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 	ret = -ENOSYS;
 	switch (cmd) {
 	case LOOP_CTL_ADD:
-		ret = loop_lookup(&lo, parm);
-		if (ret >= 0) {
-			ret = -EEXIST;
-			break;
-		}
 		ret = loop_add(parm);
 		break;
 	case LOOP_CTL_REMOVE:
-- 
2.30.2

[PATCH 5/9] loop: split loop_control_ioctl

[Christoph Hellwig]

Split loop_control_ioctl into a helper for each command.  This keeps the
code nicely separated for the upcoming locking changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 93 ++++++++++++++++++++++++++++----------------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b1e7e420563b..d55526c355e1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2228,8 +2228,51 @@ static void loop_probe(dev_t dev)
 	mutex_unlock(&loop_ctl_mutex);
 }
 
-static long loop_control_ioctl(struct file *file, unsigned int cmd,
-			       unsigned long parm)
+static int loop_control_add(int idx)
+{
+	int ret;
+		
+	ret = mutex_lock_killable(&loop_ctl_mutex);
+	if (ret)
+		return ret;
+	ret = loop_add(idx);
+	mutex_unlock(&loop_ctl_mutex);
+	return ret;
+}
+
+static int loop_control_remove(int idx)
+{
+	struct loop_device *lo;
+	int ret;
+		
+	ret = mutex_lock_killable(&loop_ctl_mutex);
+	if (ret)
+		return ret;
+
+	ret = loop_lookup(&lo, idx);
+	if (ret < 0)
+		goto out_unlock_ctrl;
+
+	ret = mutex_lock_killable(&lo->lo_mutex);
+	if (ret)
+		goto out_unlock_ctrl;
+	if (lo->lo_state != Lo_unbound ||
+	    atomic_read(&lo->lo_refcnt) > 0) {
+		mutex_unlock(&lo->lo_mutex);
+		ret = -EBUSY;
+		goto out_unlock_ctrl;
+	}
+	lo->lo_state = Lo_deleting;
+	mutex_unlock(&lo->lo_mutex);
+
+	idr_remove(&loop_index_idr, lo->lo_number);
+	loop_remove(lo);
+out_unlock_ctrl:
+	mutex_unlock(&loop_ctl_mutex);
+	return ret;
+}
+
+static int loop_control_get_free(int idx)
 {
 	struct loop_device *lo;
 	int ret;
@@ -2237,43 +2280,27 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 	ret = mutex_lock_killable(&loop_ctl_mutex);
 	if (ret)
 		return ret;
+	ret = loop_lookup(&lo, -1);
+	if (ret < 0)
+		ret = loop_add(-1);
+	mutex_unlock(&loop_ctl_mutex);
+
+	return ret;
+}
 
-	ret = -ENOSYS;
+static long loop_control_ioctl(struct file *file, unsigned int cmd,
+			       unsigned long parm)
+{
 	switch (cmd) {
 	case LOOP_CTL_ADD:
-		ret = loop_add(parm);
-		break;
+		return loop_control_add(parm);
 	case LOOP_CTL_REMOVE:
-		ret = loop_lookup(&lo, parm);
-		if (ret < 0)
-			break;
-		ret = mutex_lock_killable(&lo->lo_mutex);
-		if (ret)
-			break;
-		if (lo->lo_state != Lo_unbound) {
-			ret = -EBUSY;
-			mutex_unlock(&lo->lo_mutex);
-			break;
-		}
-		if (atomic_read(&lo->lo_refcnt) > 0) {
-			ret = -EBUSY;
-			mutex_unlock(&lo->lo_mutex);
-			break;
-		}
-		lo->lo_state = Lo_deleting;
-		mutex_unlock(&lo->lo_mutex);
-		idr_remove(&loop_index_idr, lo->lo_number);
-		loop_remove(lo);
-		break;
+		return loop_control_remove(parm);
 	case LOOP_CTL_GET_FREE:
-		ret = loop_lookup(&lo, -1);
-		if (ret >= 0)
-			break;
-		ret = loop_add(-1);
+		return loop_control_get_free(parm);
+	default:
+		return -ENOSYS;
 	}
-	mutex_unlock(&loop_ctl_mutex);
-
-	return ret;
 }
 
 static const struct file_operations loop_ctl_fops = {
-- 
2.30.2

[PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add

[Christoph Hellwig]

Move acquiring and releasing loop_ctl_mutex from the callers into
loop_add.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d55526c355e1..01d393a60b37 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2079,9 +2079,12 @@ static int loop_add(int i)
 	lo = kzalloc(sizeof(*lo), GFP_KERNEL);
 	if (!lo)
 		goto out;
-
 	lo->lo_state = Lo_unbound;
 
+	err = mutex_lock_killable(&loop_ctl_mutex);
+	if (err)
+		goto out_free_dev;
+
 	/* allocate id, if @id >= 0, we're requesting that specific id */
 	if (i >= 0) {
 		err = idr_alloc(&loop_index_idr, lo, i, i + 1, GFP_KERNEL);
@@ -2091,7 +2094,7 @@ static int loop_add(int i)
 		err = idr_alloc(&loop_index_idr, lo, 0, 0, GFP_KERNEL);
 	}
 	if (err < 0)
-		goto out_free_dev;
+		goto out_unlock;
 	i = err;
 
 	err = -ENOMEM;
@@ -2157,12 +2160,15 @@ static int loop_add(int i)
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
 	add_disk(disk);
-	return lo->lo_number;
+	mutex_unlock(&loop_ctl_mutex);
+	return i;
 
 out_cleanup_tags:
 	blk_mq_free_tag_set(&lo->tag_set);
 out_free_idr:
 	idr_remove(&loop_index_idr, i);
+out_unlock:
+	mutex_unlock(&loop_ctl_mutex);
 out_free_dev:
 	kfree(lo);
 out:
@@ -2222,22 +2228,7 @@ static void loop_probe(dev_t dev)
 
 	if (max_loop && idx >= max_loop)
 		return;
-
-	mutex_lock(&loop_ctl_mutex);
 	loop_add(idx);
-	mutex_unlock(&loop_ctl_mutex);
-}
-
-static int loop_control_add(int idx)
-{
-	int ret;
-		
-	ret = mutex_lock_killable(&loop_ctl_mutex);
-	if (ret)
-		return ret;
-	ret = loop_add(idx);
-	mutex_unlock(&loop_ctl_mutex);
-	return ret;
 }
 
 static int loop_control_remove(int idx)
@@ -2281,11 +2272,11 @@ static int loop_control_get_free(int idx)
 	if (ret)
 		return ret;
 	ret = loop_lookup(&lo, -1);
-	if (ret < 0)
-		ret = loop_add(-1);
 	mutex_unlock(&loop_ctl_mutex);
 
-	return ret;
+	if (ret >= 0)
+		return ret;
+	return loop_add(-1);
 }
 
 static long loop_control_ioctl(struct file *file, unsigned int cmd,
@@ -2293,7 +2284,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 {
 	switch (cmd) {
 	case LOOP_CTL_ADD:
-		return loop_control_add(parm);
+		return loop_add(parm);
 	case LOOP_CTL_REMOVE:
 		return loop_control_remove(parm);
 	case LOOP_CTL_GET_FREE:
@@ -2374,10 +2365,8 @@ static int __init loop_init(void)
 	}
 
 	/* pre-create number of devices given by config or max_loop */
-	mutex_lock(&loop_ctl_mutex);
 	for (i = 0; i < nr; i++)
 		loop_add(i);
-	mutex_unlock(&loop_ctl_mutex);
 
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
-- 
2.30.2

[PATCH 7/9] loop: don't allow deleting an unspecified loop device

[Christoph Hellwig]

Passing a negative index to loop_lookup while return any unbound device.
Doing that for a delete does not make much sense, so add check to
explicitly reject that case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 01d393a60b37..58fc29f1b1ae 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2235,6 +2235,11 @@ static int loop_control_remove(int idx)
 {
 	struct loop_device *lo;
 	int ret;
+
+	if (idx < 0) {
+		pr_warn("deleting an unspecified loop device is not supported.\n");
+		return -EINVAL;
+	}
 		
 	ret = mutex_lock_killable(&loop_ctl_mutex);
 	if (ret)
-- 
2.30.2

[PATCH 8/9] loop: split loop_lookup

[Christoph Hellwig]

loop_lookup has two callers - one wants to do the a find by index and the
other wants any unbound loop device.  Open code the respective
functionality in each caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 57 ++++++++++----------------------------------
 1 file changed, 12 insertions(+), 45 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 58fc29f1b1ae..93abf6d8b88c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2184,44 +2184,6 @@ static void loop_remove(struct loop_device *lo)
 	kfree(lo);
 }
 
-static int find_free_cb(int id, void *ptr, void *data)
-{
-	struct loop_device *lo = ptr;
-	struct loop_device **l = data;
-
-	if (lo->lo_state == Lo_unbound) {
-		*l = lo;
-		return 1;
-	}
-	return 0;
-}
-
-static int loop_lookup(struct loop_device **l, int i)
-{
-	struct loop_device *lo;
-	int ret = -ENODEV;
-
-	if (i < 0) {
-		int err;
-
-		err = idr_for_each(&loop_index_idr, &find_free_cb, &lo);
-		if (err == 1) {
-			*l = lo;
-			ret = lo->lo_number;
-		}
-		goto out;
-	}
-
-	/* lookup and return a specific i */
-	lo = idr_find(&loop_index_idr, i);
-	if (lo) {
-		*l = lo;
-		ret = lo->lo_number;
-	}
-out:
-	return ret;
-}
-
 static void loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
@@ -2245,9 +2207,11 @@ static int loop_control_remove(int idx)
 	if (ret)
 		return ret;
 
-	ret = loop_lookup(&lo, idx);
-	if (ret < 0)
+	lo = idr_find(&loop_index_idr, idx);
+	if (!lo) {
+		ret = -ENODEV;
 		goto out_unlock_ctrl;
+	}
 
 	ret = mutex_lock_killable(&lo->lo_mutex);
 	if (ret)
@@ -2271,17 +2235,20 @@ static int loop_control_remove(int idx)
 static int loop_control_get_free(int idx)
 {
 	struct loop_device *lo;
-	int ret;
+	int id, ret;
 
 	ret = mutex_lock_killable(&loop_ctl_mutex);
 	if (ret)
 		return ret;
-	ret = loop_lookup(&lo, -1);
+	idr_for_each_entry(&loop_index_idr, lo, id) {
+		if (lo->lo_state == Lo_unbound)
+			goto found;
+	}
 	mutex_unlock(&loop_ctl_mutex);
-
-	if (ret >= 0)
-		return ret;
 	return loop_add(-1);
+found:
+	mutex_unlock(&loop_ctl_mutex);
+	return id;
 }
 
 static long loop_control_ioctl(struct file *file, unsigned int cmd,
-- 
2.30.2

[PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry

[Christoph Hellwig]

Use idr_for_each_entry to simplify removing all devices.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 93abf6d8b88c..40b7c6c470f2 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2349,21 +2349,17 @@ static int __init loop_init(void)
 	return err;
 }
 
-static int loop_exit_cb(int id, void *ptr, void *data)
-{
-	struct loop_device *lo = ptr;
-
-	loop_remove(lo);
-	return 0;
-}
-
 static void __exit loop_exit(void)
 {
+	struct loop_device *lo;
+	int id;
+
 	unregister_blkdev(LOOP_MAJOR, "loop");
 	misc_deregister(&loop_misc);
 
 	mutex_lock(&loop_ctl_mutex);
-	idr_for_each(&loop_index_idr, &loop_exit_cb, NULL);
+	idr_for_each_entry(&loop_index_idr, lo, id)
+		loop_remove(lo);
 	mutex_unlock(&loop_ctl_mutex);
 
 	idr_destroy(&loop_index_idr);
-- 
2.30.2

Re: loop cleanups v2

[Jens Axboe]

On 6/23/21 8:58 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series contains a bunch of cleanups for the loop driver,
> mostly related to probing and the control device.
> 
> Changes since v1:
>  - use local variables instead of dereferencing struct loop_device
>    outside loop_ctl_mutex

Applied, thanks.

-- 
Jens Axboe

[PATCH 4/9] loop: don't call loop_lookup before adding a loop device

[Christoph Hellwig]

loop_add returns the right error if the slot wasn't available.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8392722d0e12..b1e7e420563b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2219,14 +2219,12 @@ static int loop_lookup(struct loop_device **l, int i)
 static void loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
-	struct loop_device *lo;
 
 	if (max_loop && idx >= max_loop)
 		return;
 
 	mutex_lock(&loop_ctl_mutex);
-	if (loop_lookup(&lo, idx) < 0)
-		loop_add(idx);
+	loop_add(idx);
 	mutex_unlock(&loop_ctl_mutex);
 }
 
@@ -2243,11 +2241,6 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 	ret = -ENOSYS;
 	switch (cmd) {
 	case LOOP_CTL_ADD:
-		ret = loop_lookup(&lo, parm);
-		if (ret >= 0) {
-			ret = -EEXIST;
-			break;
-		}
 		ret = loop_add(parm);
 		break;
 	case LOOP_CTL_REMOVE:
-- 
2.30.2