On Tue, Aug 20, 2019 at 02:21:10PM -0700, Bart Van Assche wrote:
> 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.
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.
>
> 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?
> 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
