| Message ID | 20170227161452.GB10715@vader (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/27/2017 09:14 AM, Omar Sandoval wrote: > On Mon, Feb 27, 2017 at 06:10:01PM +0200, Sagi Grimberg wrote: >> >>>> Hm, this may fix the crash, but I'm not sure it'll work as intended. >>>> When we allocate the request, we'll get a reserved scheduler tag, but >>>> then when we go to dispatch the request and call >>>> blk_mq_get_driver_tag(), we'll be competing with all of the normal >>>> requests for a regular driver tag. So maybe on top of this we should add >>>> the BLK_MQ_REQ_RESERVED flag to the allocation attempt in >>>> blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on >>>> what we expect from reserved tags, so feel free to call me crazy. >>> >>> Yeah good point, we need to carry it through. Reserved tags exist >>> because drivers often need a request/tag for error handling. If all >>> tags currently are used up for regular IO that is stuck, you need >>> a reserved tag for error handling to guarantee progress. >>> >>> So Sagi's patch does take it half the way there, but get_driver_tag >>> really needs to know about this as well, or we will just get stuck >>> there as well. Two solutions, I can think of: >>> >>> 1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED >>> when allocating a driver tag if above X. >>> 2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in >>> get_driver_tag if that is set. >>> >>> Comments? > > Option 1 looks simple enough that I don't think it warrants a new > request flag (compile tested only): > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 9e6b064e5339..87590f7d4f80 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -852,6 +852,9 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx, > return true; > } > > + if (rq->internal_tag < data.hctx->sched_tags->nr_reserved_tags) > + data.flags |= BLK_MQ_REQ_RESERVED; > + > rq->tag = blk_mq_get_tag(&data); > if (rq->tag >= 0) { > if (blk_mq_tag_busy(data.hctx)) { Agree, that's identical to what I just sent out as well, functionally. >> Can't we just not go through the scheduler for reserved tags? Obviously >> there is no point in scheduling them... > > That sounds nice, since I'd be worried about the scheduler also needing > to be aware of the reserved status lest it also get the reserved request > stuck behind some normal requests. But, we special case flush in this > way, and it has been a huge pain. The caller better be using head insertion for this, in case we already have requests in the queue. But that's no different than the current logic. So I think it should work fine.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 9e6b064e5339..87590f7d4f80 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -852,6 +852,9 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx, return true; } + if (rq->internal_tag < data.hctx->sched_tags->nr_reserved_tags) + data.flags |= BLK_MQ_REQ_RESERVED; + rq->tag = blk_mq_get_tag(&data); if (rq->tag >= 0) { if (blk_mq_tag_busy(data.hctx)) {