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

[Bart Van Assche <bvanassche@acm.org>]

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;