Re: [PATCH] block: don't acquire .sysfs_lock before removing mq & iosched kobjects
From: Ming Lei <hidden>
Date: 2019-08-21 03:01:09
On Tue, Aug 20, 2019 at 02:21:10PM -0700, Bart Van Assche wrote:
On 8/16/19 6:55 AM, Ming Lei wrote:quoted
@@ -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.
I guess we have to break the serialization, because we can't hold q->sysfs_lock when deleting iosched kobjects. And we may re-check QUEUE_FLAG_REGISTERED after grabbing .sysfs_lock again, and stop to switch to new scheduler if queue is un-registered.
Can you have a look at the alternative patch below?
Sure.
quoted hunk ↗ jump to hunk
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);
.sysfs_lock has to be held for clearing 'REGISTERED', we have to wait for concurrent store to 'queue/scheduler' before un-registering queue.
- /* - * 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);
Could you explain why you move the above line here?
quoted hunk ↗ jump to hunk
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;
elv_unregister_queue() is still called in elevator_switch_mq() with .sysfs_lock held, then potential AB-BA lock still exists. The same issue exists on blk_mq_sysfs_unregister() too. Thanks, Ming