linux-kernel - Re: [PATCH 5/7] blk-mq-sched: add framework for MQ capable IO schedulers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161213152038.GB32618@kernel.dk>
Date:   Tue, 13 Dec 2016 08:20:38 -0700
From:   Jens Axboe <axboe@...com>
To:     Bart Van Assche <bart.vanassche@...disk.com>
CC:     <linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <paolo.valente@...aro.org>, <osandov@...com>
Subject: Re: [PATCH 5/7] blk-mq-sched: add framework for MQ capable IO
 schedulers

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