lkml.org 
[lkml]   [2016]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 5/7] blk-mq-sched: add framework for MQ capable IO schedulers
From
Date
On 12/08/2016 09:13 PM, Jens Axboe wrote:
> +/*
> + * Empty set
> + */
> +static struct blk_mq_ops mq_sched_tag_ops = {
> + .queue_rq = NULL,
> +};

Hello Jens,

Would "static struct blk_mq_ops mq_sched_tag_ops;" have been sufficient?
Can this data structure be declared 'const' if the blk_mq_ops pointers
in struct blk_mq_tag_set and struct request_queue are also declared const?

> +struct request *blk_mq_sched_alloc_shadow_request(struct request_queue *q,
> + struct blk_mq_alloc_data *data,
> + struct blk_mq_tags *tags,
> + atomic_t *wait_index)
> +{

Using the word "shadow" in the function name suggests to me that there
is a shadow request for every request and a request for every shadow
request. However, my understanding from the code is that there can be
requests without shadow requests (for e.g. a flush) and shadow requests
without requests. Shouldn't the name of this function reflect that, e.g.
by using "sched" or "elv" in the function name instead of "shadow"?

> +struct request *
> +blk_mq_sched_request_from_shadow(struct blk_mq_hw_ctx *hctx,
> + struct request *(*get_sched_rq)(struct blk_mq_hw_ctx *))

This function dequeues a request from the I/O scheduler queue, allocates
a request, copies the relevant request structure members into that
request and makes the request refer to the shadow request. Isn't the
request dispatching more important than associating the request with the
shadow request? If so, how about making the function name reflect that?

> +{
> + struct blk_mq_alloc_data data;
> + struct request *sched_rq, *rq;
> +
> + data.q = hctx->queue;
> + data.flags = BLK_MQ_REQ_NOWAIT;
> + data.ctx = blk_mq_get_ctx(hctx->queue);
> + data.hctx = hctx;
> +
> + rq = __blk_mq_alloc_request(&data, 0);
> + blk_mq_put_ctx(data.ctx);
> +
> + if (!rq) {
> + blk_mq_stop_hw_queue(hctx);
> + return NULL;
> + }
> +
> + sched_rq = get_sched_rq(hctx);
> +
> + if (!sched_rq) {
> + blk_queue_enter_live(hctx->queue);
> + __blk_mq_free_request(hctx, data.ctx, rq);
> + return NULL;
> + }

The mq deadline scheduler calls this function with get_sched_rq ==
__dd_dispatch_request. If __blk_mq_alloc_request() fails, shouldn't the
request that was removed from the scheduler queue be pushed back onto
that queue? Additionally, are you sure it's necessary to call
blk_queue_enter_live() from the error path?

Bart.

\
 
 \ /
  Last update: 2016-12-13 14:57    [W:0.188 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site