Re: [PATCH] block: don't acquire .sysfs_lock before removing mq & iosched kobjects

[Bart Van Assche]

On 8/16/19 6:55 AM, Ming Lei wrote:

@@ -567,8 +568,17 @@ int elevator_switch_mq(struct request_queue *q,
        lockdep_assert_held(&q->sysfs_lock);

if (q->elevator) {

-               if (q->elevator->registered)
+               if (q->elevator->registered) {
+                       /*
+                        * sysfs write is exclusively, release
+                        * sysfs_lock for avoiding deadlock with
+                        * sysfs built-in lock which is required
+                        * in either .show or .store path.
+                        */
+                       mutex_unlock(&q->sysfs_lock);
                        elv_unregister_queue(q);
+                       mutex_lock(&q->sysfs_lock);
+               }
                ioc_clear_queue(q);
                elevator_exit(q, q->elevator);
        }

Hi Ming,

I don't like this part of the patch. Consider the following call chain:

queue_attr_store() -> elv_iosched_store() -> __elevator_change() ->
elevator_switch() -> elevator_switch_mq().

queue_attr_store() locks sysfs_lock to serialize sysfs attribute show
and store callbacks. So the above changes unlocks sysfs_lock from inside
such a callback function and hence breaks that serialization. Can you
have a look at the alternative patch below?

Thanks,

Bart.


Subject: [PATCH] block: Fix lock inversion triggered during request queue 
removal

Call blk_mq_unregister_dev() after having deleted q->kobj. Move
the kobject_uevent(q->mq_kobj, KOBJ_REMOVE) call from inside
blk_mq_unregister_dev() to its caller.

---
 block/blk-mq-sysfs.c |  5 ++---
 block/blk-sysfs.c    | 19 +++----------------
 block/elevator.c     |  2 --
 3 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index d6e1a9bd7131..0ec968009791 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -270,16 +270,15 @@ void blk_mq_unregister_dev(struct device *dev, struct 
request_queue *q)
        struct blk_mq_hw_ctx *hctx;
        int i;

-       lockdep_assert_held(&q->sysfs_lock);
-
        queue_for_each_hw_ctx(q, hctx, i)
                blk_mq_unregister_hctx(hctx);

-       kobject_uevent(q->mq_kobj, KOBJ_REMOVE);
        kobject_del(q->mq_kobj);
        kobject_put(&dev->kobj);

+       mutex_lock(&q->sysfs_lock);
        q->mq_sysfs_init_done = false;
+       mutex_unlock(&q->sysfs_lock);
 }

 void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 977c659dcd18..e6f8cd99aded 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -1029,31 +1029,18 @@ void blk_unregister_queue(struct gendisk *disk)
        if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
                return;

-       /*
-        * Since sysfs_remove_dir() prevents adding new directory entries
-        * before removal of existing entries starts, protect against
-        * concurrent elv_iosched_store() calls.
-        */
-       mutex_lock(&q->sysfs_lock);
-
        blk_queue_flag_clear(QUEUE_FLAG_REGISTERED, q);

-       /*
-        * Remove the sysfs attributes before unregistering the queue data
-        * structures that can be modified through sysfs.
-        */
        if (queue_is_mq(q))
-               blk_mq_unregister_dev(disk_to_dev(disk), q);
-       mutex_unlock(&q->sysfs_lock);
-
+               kobject_uevent(q->mq_kobj, KOBJ_REMOVE);
        kobject_uevent(&q->kobj, KOBJ_REMOVE);
        kobject_del(&q->kobj);
        blk_trace_remove_sysfs(disk_to_dev(disk));

-       mutex_lock(&q->sysfs_lock);
+       if (queue_is_mq(q))
+               blk_mq_unregister_dev(disk_to_dev(disk), q);
        if (q->elevator)
                elv_unregister_queue(q);
-       mutex_unlock(&q->sysfs_lock);

        kobject_put(&disk_to_dev(disk)->kobj);
 }
diff --git a/block/elevator.c b/block/elevator.c
index 2f17d66d0e61..128e7cf032e1 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -495,8 +495,6 @@ int elv_register_queue(struct request_queue *q)

 void elv_unregister_queue(struct request_queue *q)
 {
-       lockdep_assert_held(&q->sysfs_lock);
-
        if (q) {
                struct elevator_queue *e = q->elevator;