From: Jens Axboe <axboe@fb.com>
To: Paolo Valente <paolo.valente@linaro.org>,
Omar Sandoval <osandov@osandov.com>
Cc: <linux-block@vger.kernel.org>, <kernel-team@fb.com>
Subject: Re: [PATCH] blk-mq-sched: don't hold queue_lock when calling exit_icq
Date: Wed, 15 Feb 2017 11:04:17 -0700 [thread overview]
Message-ID: <9fe1b3a9-2a80-8891-1044-f83558e28d15@fb.com> (raw)
In-Reply-To: <a3e62a57-d401-99e0-6487-fc8277204e71@fb.com>
On 02/15/2017 10:58 AM, Jens Axboe wrote:
> On 02/15/2017 10:24 AM, Paolo Valente wrote:
>>
>>> Il giorno 10 feb 2017, alle ore 19:32, Omar Sandoval <osandov@osandov.com> ha scritto:
>>>
>>> From: Omar Sandoval <osandov@fb.com>
>>>
>>> None of the other blk-mq elevator hooks are called with this lock held.
>>> Additionally, it can lead to circular locking dependencies between
>>> queue_lock and the private scheduler lock.
>>>
>>
>> Hi Omar,
>> I'm sorry but it seems that a new potential deadlock has showed up.
>> See lockdep splat below.
>>
>> I've tried to think about different solutions than turning back to
>> deferring the body of exit_icq, but at no avail.
>
> Looks like a interaction between bfqd->lock and q->queue_lock. Since the
> core has no notion of you bfqd->lock, the naturally dependency here
> would be to nest bfqd->lock inside q->queue_lock. Is that possible for
> you?
>
> Looking at the code a bit, maybe it'd just be simpler to get rid of
> holding the queue lock for that spot. For the mq scheduler, we really
> don't want places where we invoke with that held already. Does the below
> work for you?
Would need to remove one more lockdep assert. And only test this for
the mq parts, we'd need to spread a bit of love on the classic
scheduling icq exit path for this to work on that side.
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index b12f9c87b4c3..546ff8f81ede 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -54,7 +54,7 @@ static void ioc_exit_icq(struct io_cq *icq)
icq->flags |= ICQ_EXITED;
}
-/* Release an icq. Called with both ioc and q locked. */
+/* Release an icq. Called with ioc locked. */
static void ioc_destroy_icq(struct io_cq *icq)
{
struct io_context *ioc = icq->ioc;
@@ -62,7 +62,6 @@ static void ioc_destroy_icq(struct io_cq *icq)
struct elevator_type *et = q->elevator->type;
lockdep_assert_held(&ioc->lock);
- lockdep_assert_held(q->queue_lock);
radix_tree_delete(&ioc->icq_tree, icq->q->id);
hlist_del_init(&icq->ioc_node);
@@ -222,25 +221,34 @@ void exit_io_context(struct task_struct *task)
put_io_context_active(ioc);
}
+static void __ioc_clear_queue(struct list_head *icq_list)
+{
+ while (!list_empty(icq_list)) {
+ struct io_cq *icq = list_entry(icq_list->next,
+ struct io_cq, q_node);
+ struct io_context *ioc = icq->ioc;
+
+ spin_lock_irq(&ioc->lock);
+ ioc_destroy_icq(icq);
+ spin_unlock_irq(&ioc->lock);
+ }
+}
+
/**
* ioc_clear_queue - break any ioc association with the specified queue
* @q: request_queue being cleared
*
- * Walk @q->icq_list and exit all io_cq's. Must be called with @q locked.
+ * Walk @q->icq_list and exit all io_cq's.
*/
void ioc_clear_queue(struct request_queue *q)
{
- lockdep_assert_held(q->queue_lock);
+ LIST_HEAD(icq_list);
- while (!list_empty(&q->icq_list)) {
- struct io_cq *icq = list_entry(q->icq_list.next,
- struct io_cq, q_node);
- struct io_context *ioc = icq->ioc;
+ spin_lock_irq(q->queue_lock);
+ list_splice_init(&q->icq_list, &icq_list);
+ spin_unlock_irq(q->queue_lock);
- spin_lock(&ioc->lock);
- ioc_destroy_icq(icq);
- spin_unlock(&ioc->lock);
- }
+ __ioc_clear_queue(&icq_list);
}
int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 070d81bae1d5..1944aa1cb899 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -815,9 +815,7 @@ static void blk_release_queue(struct kobject *kobj)
blkcg_exit_queue(q);
if (q->elevator) {
- spin_lock_irq(q->queue_lock);
ioc_clear_queue(q);
- spin_unlock_irq(q->queue_lock);
elevator_exit(q->elevator);
}
diff --git a/block/elevator.c b/block/elevator.c
index a25bdd90b270..aaa1e9836512 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -985,9 +985,7 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
if (old_registered)
elv_unregister_queue(q);
- spin_lock_irq(q->queue_lock);
ioc_clear_queue(q);
- spin_unlock_irq(q->queue_lock);
}
/* allocate, init and register new elevator */
--
Jens Axboe
next prev parent reply other threads:[~2017-02-15 18:04 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-10 18:32 [PATCH] blk-mq-sched: don't hold queue_lock when calling exit_icq Omar Sandoval
2017-02-10 18:35 ` Jens Axboe
2017-02-15 17:24 ` Paolo Valente
2017-02-15 17:58 ` Jens Axboe
2017-02-15 18:04 ` Jens Axboe [this message]
2017-02-16 10:31 ` Paolo Valente
2017-02-17 10:30 ` Paolo Valente
2017-02-22 21:21 ` Paolo Valente
2017-03-02 10:28 ` Paolo Valente
2017-03-02 15:00 ` Jens Axboe
2017-03-02 15:13 ` Jens Axboe
2017-03-02 16:07 ` Paolo Valente
2017-03-02 16:13 ` Jens Axboe
2017-03-02 18:07 ` Paolo Valente
2017-03-02 18:25 ` Jens Axboe
2017-03-02 20:31 ` Paolo Valente
2017-03-02 20:53 ` Omar Sandoval
2017-03-02 20:59 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9fe1b3a9-2a80-8891-1044-f83558e28d15@fb.com \
--to=axboe@fb.com \
--cc=kernel-team@fb.com \
--cc=linux-block@vger.kernel.org \
--cc=osandov@osandov.com \
--cc=paolo.valente@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).