| Message ID | 20170714084136.24850-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 14, 2017 at 04:41:36PM +0800, Ming Lei wrote: > From: Ming Lei <minlei@redhat.com> > > When blk_mq_get_request() failed, preempt counter isn't > released, and blk_mq_make_request() doesn't release the counter > too. > > This patch fixes the issue, and makes sure that preempt counter > is only held if rq is allocated successfully. The same policy is > applied on .q_usage_counter too. Hi Jens, Given NOWAIT has been merged, could you consider this patch for v4.13? Thanks, Ming
On 07/14/2017 02:41 AM, Ming Lei wrote: > From: Ming Lei <minlei@redhat.com> > > When blk_mq_get_request() failed, preempt counter isn't > released, and blk_mq_make_request() doesn't release the counter > too. > > This patch fixes the issue, and makes sure that preempt counter > is only held if rq is allocated successfully. The same policy is > applied on .q_usage_counter too. Can you replace the 'get_cpu' bool with just a ctx, and change the logic to put it if set? I think that would be cleaner to read, generally I hate bool 'do_something' variables that are set. It's much cleaner to have: if (likely(!data->ctx)) data->ctx = local_ctx = blk_mq_get_ctx(q); and have the put case be if (local_ctx) blk_mq_put_ctx(local_ctx); Either that, or at least just have blk_mq_get_request() do: drop_ctx = data->ctx == NULL; instead. The 'get_cpu' naming is confusing, we're worried about dropping the sw queue here, the fact that it's related to get/put_cpu() need not bubble up here.
On Tue, Aug 01, 2017 at 09:18:23AM -0600, Jens Axboe wrote: > On 07/14/2017 02:41 AM, Ming Lei wrote: > > From: Ming Lei <minlei@redhat.com> > > > > When blk_mq_get_request() failed, preempt counter isn't > > released, and blk_mq_make_request() doesn't release the counter > > too. > > > > This patch fixes the issue, and makes sure that preempt counter > > is only held if rq is allocated successfully. The same policy is > > applied on .q_usage_counter too. > > Can you replace the 'get_cpu' bool with just a ctx, and change > the logic to put it if set? I think that would be cleaner to read, > generally I hate bool 'do_something' variables that are set. It's > much cleaner to have: > > if (likely(!data->ctx)) > data->ctx = local_ctx = blk_mq_get_ctx(q); > > and have the put case be > > if (local_ctx) > blk_mq_put_ctx(local_ctx); > > Either that, or at least just have blk_mq_get_request() do: > > drop_ctx = data->ctx == NULL; > > instead. The 'get_cpu' naming is confusing, we're worried about dropping > the sw queue here, the fact that it's related to get/put_cpu() need not > bubble up here. Good point, V2 is sent out with this change. Thanks your suggestion!
diff --git a/block/blk-mq.c b/block/blk-mq.c index 041f7b7fa0d6..4f2561412f13 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -301,11 +301,14 @@ static struct request *blk_mq_get_request(struct request_queue *q, struct elevator_queue *e = q->elevator; struct request *rq; unsigned int tag; + bool get_cpu = false; blk_queue_enter_live(q); data->q = q; - if (likely(!data->ctx)) + if (likely(!data->ctx)) { data->ctx = blk_mq_get_ctx(q); + get_cpu = true; + } if (likely(!data->hctx)) data->hctx = blk_mq_map_queue(q, data->ctx->cpu); if (op & REQ_NOWAIT) @@ -324,6 +327,8 @@ static struct request *blk_mq_get_request(struct request_queue *q, tag = blk_mq_get_tag(data); if (tag == BLK_MQ_TAG_FAIL) { + if (get_cpu) + blk_mq_put_ctx(data->ctx); blk_queue_exit(q); return NULL; } @@ -356,12 +361,12 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op, rq = blk_mq_get_request(q, NULL, op, &alloc_data); - blk_mq_put_ctx(alloc_data.ctx); - blk_queue_exit(q); - if (!rq) return ERR_PTR(-EWOULDBLOCK); + blk_mq_put_ctx(alloc_data.ctx); + blk_queue_exit(q); + rq->__data_len = 0; rq->__sector = (sector_t) -1; rq->bio = rq->biotail = NULL; @@ -407,11 +412,11 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, rq = blk_mq_get_request(q, NULL, op, &alloc_data); - blk_queue_exit(q); - if (!rq) return ERR_PTR(-EWOULDBLOCK); + blk_queue_exit(q); + return rq; } EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);