loop cleanups

loop cleanups

[Christoph Hellwig]

Hi Jens,

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

[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>
---
 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

Re: [PATCH 1/9] loop: reorder loop_exit

[Chaitanya Kulkarni]

On 6/21/21 03:21, Christoph Hellwig wrote:
> 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>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[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>
---
 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

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

[Chaitanya Kulkarni]

On 6/21/21 03:24, Christoph Hellwig wrote:
> 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>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[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>
---
 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

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

[Chaitanya Kulkarni]

On 6/21/21 03:26, Christoph Hellwig wrote:
> None of the callers cares about the allocated struct loop_device.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[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>
---
 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

Re: [PATCH 5/9] loop: split loop_control_ioctl

[Chaitanya Kulkarni]

On 6/21/21 03:31, Christoph Hellwig wrote:
> 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>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[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>
---
 drivers/block/loop.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d55526c355e1..3f1e934a7f9e 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);
+	mutex_unlock(&loop_ctl_mutex);
 	return lo->lo_number;
 
 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

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

[Chaitanya Kulkarni]

On 6/21/21 03:34, Christoph Hellwig wrote:
> Move acquiring and releasing loop_ctl_mutex from the callers into
> loop_add.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[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 3f1e934a7f9e..54e2551e339c 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 54e2551e339c..2ff5162bd28b 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 lo->lo_number;
 }
 
 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>
---
 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 2ff5162bd28b..a9fd1f0d0ded 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: [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry

[Chaitanya Kulkarni]

On 6/21/21 03:42, Christoph Hellwig wrote:
> Use idr_for_each_entry to simplify removing all devices.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I actually thought about this while reviewing the first
patch. Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Re: loop cleanups

[Tetsuo Handa]

On 2021/06/21 19:15, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series contains a bunch of cleanups for the loop driver,
> mostly related to probing and the control device.
> 

Please fold

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index a4a5466b998f..6c10400d4d38 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2163,8 +2163,9 @@ static int loop_add(int i)
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
 	add_disk(disk);
+	err = lo->lo_number;
 	mutex_unlock(&loop_ctl_mutex);
-	return lo->lo_number;
+	return err;
 
 out_free_queue:
 	blk_cleanup_queue(lo->lo_queue);
@@ -2253,8 +2254,9 @@ static int loop_control_get_free(int idx)
 	mutex_unlock(&loop_ctl_mutex);
 	return loop_add(-1);
 found:
+	ret = lo->lo_number;
 	mutex_unlock(&loop_ctl_mutex);
-	return lo->lo_number;
+	return ret;
 }
 
 static long loop_control_ioctl(struct file *file, unsigned int cmd,

into this series, for "mutex_unlock(&loop_ctl_mutex)" allows loop_control_remove()
to call "kfree(lo)" before "return lo->lo_number" is evaluated.



By the way, how can we fix a regression introduced by commit 6cc8e7430801fa23
("loop: scale loop device by introducing per device lock") ?
Conditionally holding global lock like below untested diff?

 drivers/block/loop.c |   67 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index a4a5466b998f..f755ef82ee51 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -86,6 +86,7 @@
 
 static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
+static DEFINE_MUTEX(loop_configure_mutex);
 
 static int max_part;
 static int part_shift;
@@ -680,9 +681,12 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
 			return -EBADF;
 
 		l = I_BDEV(f->f_mapping->host)->bd_disk->private_data;
-		if (l->lo_state != Lo_bound) {
+		/*
+		 * loop_configure_mutex protects us from observing
+		 * l->lo_state == Lo_bound but l->lo_backing_file == NULL.
+		 */
+		if (l->lo_state != Lo_bound)
 			return -EINVAL;
-		}
 		f = l->lo_backing_file;
 	}
 	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
@@ -704,10 +708,26 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	struct file	*file = NULL, *old_file;
 	int		error;
 	bool		partscan;
+	bool need_global_lock;
 
+	file = fget(arg);
+	if (!file)
+		return -EBADF;
+	need_global_lock = is_loop_device(file);
+	if (need_global_lock) {
+		error = mutex_lock_killable(&loop_configure_mutex);
+		if (error) {
+			fput(file);
+			return error;
+		}
+	}
 	error = mutex_lock_killable(&lo->lo_mutex);
-	if (error)
+	if (error) {
+		if (need_global_lock)
+			mutex_unlock(&loop_configure_mutex);
+		fput(file);
 		return error;
+	}
 	error = -ENXIO;
 	if (lo->lo_state != Lo_bound)
 		goto out_err;
@@ -717,11 +737,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
 		goto out_err;
 
-	error = -EBADF;
-	file = fget(arg);
-	if (!file)
-		goto out_err;
-
 	error = loop_validate_file(file, bdev);
 	if (error)
 		goto out_err;
@@ -745,6 +760,8 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 	blk_mq_unfreeze_queue(lo->lo_queue);
 	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
 	mutex_unlock(&lo->lo_mutex);
+	if (need_global_lock)
+		mutex_unlock(&loop_configure_mutex);
 	/*
 	 * We must drop file reference outside of lo_mutex as dropping
 	 * the file ref can take bd_mutex which creates circular locking
@@ -757,8 +774,9 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 
 out_err:
 	mutex_unlock(&lo->lo_mutex);
-	if (file)
-		fput(file);
+	if (need_global_lock)
+		mutex_unlock(&loop_configure_mutex);
+	fput(file);
 	return error;
 }
 
@@ -1074,6 +1092,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	loff_t		size;
 	bool		partscan;
 	unsigned short  bsize;
+	bool need_global_lock;
 
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
@@ -1093,9 +1112,18 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 			goto out_putf;
 	}
 
+	need_global_lock = is_loop_device(file);
+	if (need_global_lock) {
+		error = mutex_lock_killable(&loop_configure_mutex);
+		if (error)
+			goto out_bdev;
+	}
 	error = mutex_lock_killable(&lo->lo_mutex);
-	if (error)
+	if (error) {
+		if (need_global_lock)
+			mutex_unlock(&loop_configure_mutex);
 		goto out_bdev;
+	}
 
 	error = -EBUSY;
 	if (lo->lo_state != Lo_unbound)
@@ -1173,6 +1201,8 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	 */
 	bdgrab(bdev);
 	mutex_unlock(&lo->lo_mutex);
+	if (need_global_lock)
+		mutex_unlock(&loop_configure_mutex);
 	if (partscan)
 		loop_reread_partitions(lo, bdev);
 	if (!(mode & FMODE_EXCL))
@@ -1181,6 +1211,8 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 
 out_unlock:
 	mutex_unlock(&lo->lo_mutex);
+	if (need_global_lock)
+		mutex_unlock(&loop_configure_mutex);
 out_bdev:
 	if (!(mode & FMODE_EXCL))
 		bd_abort_claiming(bdev, loop_configure);
@@ -1309,11 +1341,17 @@ static int loop_clr_fd(struct loop_device *lo)
 {
 	int err;
 
-	err = mutex_lock_killable(&lo->lo_mutex);
+	err = mutex_lock_killable(&loop_configure_mutex);
 	if (err)
 		return err;
+	err = mutex_lock_killable(&lo->lo_mutex);
+	if (err) {
+		mutex_unlock(&loop_configure_mutex);
+		return err;
+	}
 	if (lo->lo_state != Lo_bound) {
 		mutex_unlock(&lo->lo_mutex);
+		mutex_unlock(&loop_configure_mutex);
 		return -ENXIO;
 	}
 	/*
@@ -1329,10 +1367,12 @@ static int loop_clr_fd(struct loop_device *lo)
 	if (atomic_read(&lo->lo_refcnt) > 1) {
 		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
 		mutex_unlock(&lo->lo_mutex);
+		mutex_unlock(&loop_configure_mutex);
 		return 0;
 	}
 	lo->lo_state = Lo_rundown;
 	mutex_unlock(&lo->lo_mutex);
+	mutex_unlock(&loop_configure_mutex);
 
 	return __loop_clr_fd(lo, false);
 }
@@ -1897,6 +1937,7 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 {
 	struct loop_device *lo = disk->private_data;
 
+	mutex_lock(&loop_configure_mutex);
 	mutex_lock(&lo->lo_mutex);
 	if (atomic_dec_return(&lo->lo_refcnt))
 		goto out_unlock;
@@ -1906,6 +1947,7 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 			goto out_unlock;
 		lo->lo_state = Lo_rundown;
 		mutex_unlock(&lo->lo_mutex);
+		mutex_unlock(&loop_configure_mutex);
 		/*
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
@@ -1923,6 +1965,7 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 
 out_unlock:
 	mutex_unlock(&lo->lo_mutex);
+	mutex_unlock(&loop_configure_mutex);
 }
 
 static const struct block_device_operations lo_fops = {

Re: loop cleanups

[Christoph Hellwig]

On Tue, Jun 22, 2021 at 08:15:27PM +0900, Tetsuo Handa wrote:
> On 2021/06/21 19:15, Christoph Hellwig wrote:
> > Hi Jens,
> > 
> > this series contains a bunch of cleanups for the loop driver,
> > mostly related to probing and the control device.
> > 
> 
> Please fold
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index a4a5466b998f..6c10400d4d38 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2163,8 +2163,9 @@ static int loop_add(int i)
>  	disk->queue		= lo->lo_queue;
>  	sprintf(disk->disk_name, "loop%d", i);
>  	add_disk(disk);
> +	err = lo->lo_number;
>  	mutex_unlock(&loop_ctl_mutex);
> -	return lo->lo_number;
> +	return err;
>  
>  out_free_queue:
>  	blk_cleanup_queue(lo->lo_queue);
> @@ -2253,8 +2254,9 @@ static int loop_control_get_free(int idx)
>  	mutex_unlock(&loop_ctl_mutex);
>  	return loop_add(-1);
>  found:
> +	ret = lo->lo_number;
>  	mutex_unlock(&loop_ctl_mutex);
> -	return lo->lo_number;
> +	return ret;
>  }

Good find.  But we already have local variables holding the index
in both functions, so we can just use those.

> By the way, how can we fix a regression introduced by commit 6cc8e7430801fa23
> ("loop: scale loop device by introducing per device lock") ?
> Conditionally holding global lock like below untested diff?

It would be nice to factor the global locking into helpers, but otherwise
this looks ok.  Maybe also rename loop_configure_mutex into
loop_validate_mutex