On Thu, 2017-11-30 at 20:03 -0700, Jens Axboe wrote:
> On 11/30/2017 05:08 PM, Bart Van Assche wrote:
> > This patch does not change any functionality.
>
> Unless these actually found real bugs, I think it's pointless.
> Add a comment.
Hello Jens,
As you know lockdep_assert_held() statements are verified at runtime with
LOCKDEP enabled but comments not. Hence my preference for lockdep_assert_held()
over source code comments.
> And this:
>
> > @@ -1003,9 +1007,14 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
> > static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode,
> > int flags, void *key)
> > {
> > - struct blk_mq_hw_ctx *hctx;
> > + struct blk_mq_hw_ctx *hctx =
> > + container_of(wait, struct blk_mq_hw_ctx, dispatch_wait);
> > +
> > +#ifdef CONFIG_LOCKDEP
> > + struct sbq_wait_state *ws = bt_wait_ptr(&hctx->tags->bitmap_tags, hctx);
> >
> > - hctx = container_of(wait, struct blk_mq_hw_ctx, dispatch_wait);
> > + lockdep_assert_held(&ws->wait.lock);
> > +#endif
>
> we're definitely not doing.
Can you explain me why you think the above is wrong? My understanding is
that the call chain for the above function is as follows:
blk_mq_tag_wakeup_all()
-> sbitmap_queue_wake_all()
-> wake_up()
-> __wake_up()
-> __wake_up_common_lock()
-> spin_lock_irqsave(&wq_head->lock, flags);
-> __wake_up_common()
-> list_for_each_entry_safe_from(curr, next, &wq_head->head, entry)
-> ret = curr->func(curr, mode, wake_flags, key)
-> spin_unlock_irqrestore(&wq_head->lock, flags);
In other words, blk_mq_dispatch_wake() is called with the wait queue head
lock held.
Bart.
