* loop cleanups
@ 2021-06-21 10:15 Christoph Hellwig
2021-06-21 10:15 ` [PATCH 1/9] loop: reorder loop_exit Christoph Hellwig
` (9 more replies)
0 siblings, 10 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
Hi Jens,
this series contains a bunch of cleanups for the loop driver,
mostly related to probing and the control device.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/9] loop: reorder loop_exit
2021-06-21 10:15 loop cleanups Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:04 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit Christoph Hellwig
` (8 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit
2021-06-21 10:15 loop cleanups Christoph Hellwig
2021-06-21 10:15 ` [PATCH 1/9] loop: reorder loop_exit Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:07 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 3/9] loop: remove the l argument to loop_add Christoph Hellwig
` (7 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/9] loop: remove the l argument to loop_add
2021-06-21 10:15 loop cleanups Christoph Hellwig
2021-06-21 10:15 ` [PATCH 1/9] loop: reorder loop_exit Christoph Hellwig
2021-06-21 10:15 ` [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:09 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 4/9] loop: don't call loop_lookup before adding a loop device Christoph Hellwig
` (6 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/9] loop: don't call loop_lookup before adding a loop device
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (2 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 3/9] loop: remove the l argument to loop_add Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 10:15 ` [PATCH 5/9] loop: split loop_control_ioctl Christoph Hellwig
` (5 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/9] loop: split loop_control_ioctl
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (3 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 4/9] loop: don't call loop_lookup before adding a loop device Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:14 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add Christoph Hellwig
` (4 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (4 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 5/9] loop: split loop_control_ioctl Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:16 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 7/9] loop: don't allow deleting an unspecified loop device Christoph Hellwig
` (3 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/9] loop: don't allow deleting an unspecified loop device
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (5 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 10:15 ` [PATCH 8/9] loop: split loop_lookup Christoph Hellwig
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 8/9] loop: split loop_lookup
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (6 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 7/9] loop: don't allow deleting an unspecified loop device Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 10:15 ` [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry Christoph Hellwig
2021-06-22 11:15 ` loop cleanups Tetsuo Handa
9 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (7 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 8/9] loop: split loop_lookup Christoph Hellwig
@ 2021-06-21 10:15 ` Christoph Hellwig
2021-06-21 20:20 ` Chaitanya Kulkarni
2021-06-22 11:15 ` loop cleanups Tetsuo Handa
9 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-21 10:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/9] loop: reorder loop_exit
2021-06-21 10:15 ` [PATCH 1/9] loop: reorder loop_exit Christoph Hellwig
@ 2021-06-21 20:04 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:04 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit
2021-06-21 10:15 ` [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit Christoph Hellwig
@ 2021-06-21 20:07 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:07 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/9] loop: remove the l argument to loop_add
2021-06-21 10:15 ` [PATCH 3/9] loop: remove the l argument to loop_add Christoph Hellwig
@ 2021-06-21 20:09 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:09 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/9] loop: split loop_control_ioctl
2021-06-21 10:15 ` [PATCH 5/9] loop: split loop_control_ioctl Christoph Hellwig
@ 2021-06-21 20:14 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:14 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add
2021-06-21 10:15 ` [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add Christoph Hellwig
@ 2021-06-21 20:16 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:16 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry
2021-06-21 10:15 ` [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry Christoph Hellwig
@ 2021-06-21 20:20 ` Chaitanya Kulkarni
0 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-06-21 20:20 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: Tetsuo Handa, linux-block
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: loop cleanups
2021-06-21 10:15 loop cleanups Christoph Hellwig
` (8 preceding siblings ...)
2021-06-21 10:15 ` [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry Christoph Hellwig
@ 2021-06-22 11:15 ` Tetsuo Handa
2021-06-23 14:41 ` Christoph Hellwig
9 siblings, 1 reply; 19+ messages in thread
From: Tetsuo Handa @ 2021-06-22 11:15 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: linux-block
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 = {
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: loop cleanups
2021-06-22 11:15 ` loop cleanups Tetsuo Handa
@ 2021-06-23 14:41 ` Christoph Hellwig
0 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-23 14:41 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Christoph Hellwig, Jens Axboe, linux-block
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry
2021-06-23 14:58 loop cleanups v2 Christoph Hellwig
@ 2021-06-23 14:59 ` Christoph Hellwig
0 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-06-23 14:59 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tetsuo Handa, linux-block, Chaitanya Kulkarni
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-06-23 15:04 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 10:15 loop cleanups Christoph Hellwig
2021-06-21 10:15 ` [PATCH 1/9] loop: reorder loop_exit Christoph Hellwig
2021-06-21 20:04 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 2/9] loop: reduce loop_ctl_mutex coverage in loop_exit Christoph Hellwig
2021-06-21 20:07 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 3/9] loop: remove the l argument to loop_add Christoph Hellwig
2021-06-21 20:09 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 4/9] loop: don't call loop_lookup before adding a loop device Christoph Hellwig
2021-06-21 10:15 ` [PATCH 5/9] loop: split loop_control_ioctl Christoph Hellwig
2021-06-21 20:14 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 6/9] loop: move loop_ctl_mutex locking into loop_add Christoph Hellwig
2021-06-21 20:16 ` Chaitanya Kulkarni
2021-06-21 10:15 ` [PATCH 7/9] loop: don't allow deleting an unspecified loop device Christoph Hellwig
2021-06-21 10:15 ` [PATCH 8/9] loop: split loop_lookup Christoph Hellwig
2021-06-21 10:15 ` [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry Christoph Hellwig
2021-06-21 20:20 ` Chaitanya Kulkarni
2021-06-22 11:15 ` loop cleanups Tetsuo Handa
2021-06-23 14:41 ` Christoph Hellwig
2021-06-23 14:58 loop cleanups v2 Christoph Hellwig
2021-06-23 14:59 ` [PATCH 9/9] loop: rewrite loop_exit using idr_for_each_entry Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.