On Mon, Jun 05, 2017 at 09:25:29PM +0000, Bart Van Assche wrote:
> On Mon, 2017-06-05 at 23:59 +0800, Ming Lei wrote:
> > +static void __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> > + struct request *rq,
> > + blk_qc_t *cookie, bool may_sleep)
> > {
> > struct request_queue *q = rq->q;
> > struct blk_mq_queue_data bd = {
> > .rq = rq,
> > .last = true,
> > };
> > - struct blk_mq_hw_ctx *hctx;
> > blk_qc_t new_cookie;
> > int ret;
> > + bool run_queue = true;
> > +
> > + if (blk_mq_hctx_stopped(hctx)) {
> > + run_queue = false;
> > + goto insert;
> > + }
> >
> > if (q->elevator)
> > goto insert;
> >
> > - if (!blk_mq_get_driver_tag(rq, &hctx, false))
> > + if (!blk_mq_get_driver_tag(rq, NULL, false))
> > goto insert;
> >
> > new_cookie = request_to_qc_t(hctx, rq);
> > @@ -1439,7 +1445,7 @@ static void __blk_mq_try_issue_directly(struct request *rq, blk_qc_t *cookie,
> >
> > __blk_mq_requeue_request(rq);
> > insert:
> > - blk_mq_sched_insert_request(rq, false, true, false, may_sleep);
> > + blk_mq_sched_insert_request(rq, false, run_queue, false, may_sleep);
> > }
> >
> > static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> > @@ -1447,7 +1453,7 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> > {
> > if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > rcu_read_lock();
> > - __blk_mq_try_issue_directly(rq, cookie, false);
> > + __blk_mq_try_issue_directly(hctx, rq, cookie, false);
> > rcu_read_unlock();
> > } else {
> > unsigned int srcu_idx;
> > @@ -1455,7 +1461,7 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> > might_sleep();
> >
> > srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
> > - __blk_mq_try_issue_directly(rq, cookie, true);
> > + __blk_mq_try_issue_directly(hctx, rq, cookie, true);
> > srcu_read_unlock(&hctx->queue_rq_srcu, srcu_idx);
> > }
> > }
>
> Hello Ming,
>
> It seems like you are assuming that the hardware queue of the rq argument
> passed to __blk_mq_try_issue_directly() matches the hctx argument? Sorry
> but I think that's an incorrect assumption. Please have a look at the
> following code in blk_mq_make_request():
>
> if (same_queue_rq)
> blk_mq_try_issue_directly(data.hctx, same_queue_rq,
> &cookie);
IMO it is a bug which need to be fixed, since
blk_mq_try_issue_directly() may take a wrong SRCU lock
and performance can be hurt under this situation.
Thanks,
Ming
