On Thu, 2018-12-20 at 06:07 -0700, Jens Axboe wrote:
> On 12/20/18 6:02 AM, Jens Axboe wrote:
> > > I'm afraid this cannot work.
> > >
> > > The 'tags' here could be the hctx->sched_tags, but what we need to
> > > clear is hctx->tags->rqs[].
> >
> > You are right, of course, a bit too quick on the trigger. This one
> > should work better, and also avoids that silly quadratic loop. I don't
> > think we need the tag == -1 check, but probably best to be safe.
>
> Sent out the wrong one, here it is. Bart, if you can reproduce, can you
> give it a whirl?
>
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 2de972857496..fc04bb648f18 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2025,7 +2025,7 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct
> blk_mq_tags *tags,
> {
> struct page *page;
>
> - if (tags->rqs && set->ops->exit_request) {
> + if (tags->rqs) {
> int i;
>
> for (i = 0; i < tags->nr_tags; i++) {
> @@ -2033,8 +2033,14 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set,
> struct blk_mq_tags *tags,
>
> if (!rq)
> continue;
> - set->ops->exit_request(set, rq, hctx_idx);
> + if (set->ops->exit_request)
> + set->ops->exit_request(set, rq, hctx_idx);
> tags->static_rqs[i] = NULL;
> +
> + if (rq->tag == -1)
> + continue;
> + if (set->tags[hctx_idx]->rqs[rq->tag] == rq)
> + set->tags[hctx_idx]->rqs[rq->tag] = NULL;
> }
> }
>
> @@ -2113,6 +2119,7 @@ static int blk_mq_init_request(struct blk_mq_tag_set
> *set, struct request *rq,
> return ret;
> }
>
> + rq->tag = -1;
> WRITE_ONCE(rq->state, MQ_RQ_IDLE);
> return 0;
> }
Hi Jens,
Are you sure this is sufficient? My understanding is that
blk_mq_queue_tag_busy_iter() iterates over all tags in the tag set. So if the
request queue on which part_in_flight() is called and the request queue for
which blk_mq_free_rqs() is called share their tag set then part_in_flight()
and blk_mq_free_rqs() can run concurrently. That can cause ugly race
conditions. Do you think it would be a good idea to modify the inflight
accounting code such that it only considers the requests of a single request
queue instead of all requests for a given tag set?
Thanks,
Bart.
