On Tue, Dec 13 2016, Bart Van Assche wrote:
> On 12/08/2016 09:13 PM, Jens Axboe wrote:
> >+static inline void blk_mq_sched_put_request(struct request *rq)
> >+{
> >+ struct request_queue *q = rq->q;
> >+ struct elevator_queue *e = q->elevator;
> >+
> >+ if (e && e->type->mq_ops.put_request)
> >+ e->type->mq_ops.put_request(rq);
> >+ else
> >+ blk_mq_free_request(rq);
> >+}
>
> blk_mq_free_request() always triggers a call of blk_queue_exit().
> dd_put_request() only triggers a call of blk_queue_exit() if it is not a
> shadow request. Is that on purpose?
If the scheduler doesn't define get/put requests, then the lifetime
follows the normal setup. If we do define them, then dd_put_request()
only wants to put the request if it's one where we did setup a shadow.
> >+static inline struct request *
> >+blk_mq_sched_get_request(struct request_queue *q, unsigned int op,
> >+ struct blk_mq_alloc_data *data)
> >+{
> >+ struct elevator_queue *e = q->elevator;
> >+ struct blk_mq_hw_ctx *hctx;
> >+ struct blk_mq_ctx *ctx;
> >+ struct request *rq;
> >+
> >+ blk_queue_enter_live(q);
> >+ ctx = blk_mq_get_ctx(q);
> >+ hctx = blk_mq_map_queue(q, ctx->cpu);
> >+
> >+ blk_mq_set_alloc_data(data, q, 0, ctx, hctx);
> >+
> >+ if (e && e->type->mq_ops.get_request)
> >+ rq = e->type->mq_ops.get_request(q, op, data);
> >+ else
> >+ rq = __blk_mq_alloc_request(data, op);
> >+
> >+ if (rq)
> >+ data->hctx->queued++;
> >+
> >+ return rq;
> >+
> >+}
>
> Some but not all callers of blk_mq_sched_get_request() call blk_queue_exit()
> if this function returns NULL. Please consider to move the blk_queue_exit()
> call from the blk_mq_alloc_request() error path into this function. I think
> that will make it a lot easier to verify whether or not the
> blk_queue_enter() / blk_queue_exit() calls are balanced properly.
Agree, I'll make the change, it'll be easier to read then.
> Additionally, since blk_queue_enter() / blk_queue_exit() calls by
> blk_mq_sched_get_request() and blk_mq_sched_put_request() must be balanced
> and since the latter function only calls blk_queue_exit() for non-shadow
> requests, shouldn't blk_mq_sched_get_request() call blk_queue_enter_live()
> only if __blk_mq_alloc_request() is called?
I'll double check that part, there might be a bug or at least a chance
to clean this up a bit. I did verify most of this at some point, and
tested it with the scheduler switching. That part falls apart pretty
quickly, if the references aren't matched exactly.
--
Jens Axboe
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
