From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751254AbdAQJS3 (ORCPT ); Tue, 17 Jan 2017 04:18:29 -0500 Received: from mail-wm0-f44.google.com ([74.125.82.44]:37173 "EHLO mail-wm0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751059AbdAQJS0 (ORCPT ); Tue, 17 Jan 2017 04:18:26 -0500 Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH 6/8] blk-mq-sched: add framework for MQ capable IO schedulers From: Paolo Valente In-Reply-To: Date: Tue, 17 Jan 2017 10:17:09 +0100 Cc: Jens Axboe , linux-block@vger.kernel.org, Linux-Kernal , osandov@fb.com Message-Id: References: <1481933536-12844-1-git-send-email-axboe@fb.com> <1481933536-12844-7-git-send-email-axboe@fb.com> <9936D33B-AE98-46DF-B1DA-D54142A338FD@linaro.org> To: Jens Axboe X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v0H9IkTt025298 > Il giorno 17 gen 2017, alle ore 03:47, Jens Axboe ha scritto: > > On 12/22/2016 02:59 AM, Paolo Valente wrote: >> >>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe ha scritto: >>> >>> This adds a set of hooks that intercepts the blk-mq path of >>> allocating/inserting/issuing/completing requests, allowing >>> us to develop a scheduler within that framework. >>> >>> We reuse the existing elevator scheduler API on the registration >>> side, but augment that with the scheduler flagging support for >>> the blk-mq interfce, and with a separate set of ops hooks for MQ >>> devices. >>> >>> Schedulers can opt in to using shadow requests. Shadow requests >>> are internal requests that the scheduler uses for for the allocate >>> and insert part, which are then mapped to a real driver request >>> at dispatch time. This is needed to separate the device queue depth >>> from the pool of requests that the scheduler has to work with. >>> >>> Signed-off-by: Jens Axboe >>> >> ... >> >>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c >>> new file mode 100644 >>> index 000000000000..b7e1839d4785 >>> --- /dev/null >>> +++ b/block/blk-mq-sched.c >> >>> ... >>> +static inline bool >>> +blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq, >>> + struct bio *bio) >>> +{ >>> + struct elevator_queue *e = q->elevator; >>> + >>> + if (e && e->type->ops.mq.allow_merge) >>> + return e->type->ops.mq.allow_merge(q, rq, bio); >>> + >>> + return true; >>> +} >>> + >> >> Something does not seem to add up here: >> e->type->ops.mq.allow_merge may be called only in >> blk_mq_sched_allow_merge, which, in its turn, may be called only in >> blk_mq_attempt_merge, which, finally, may be called only in >> blk_mq_merge_queue_io. Yet the latter may be called only if there is >> no elevator (line 1399 and 1507 in blk-mq.c). >> >> Therefore, e->type->ops.mq.allow_merge can never be called, both if >> there is and if there is not an elevator. Be patient if I'm missing >> something huge, but I thought it was worth reporting this. > > I went through the current branch, and it seems mostly fine. There was > a double call to allow_merge() that I killed in the plug path, and one > set missing in blk_mq_sched_try_merge(). The rest looks OK. > Yes, I missed a path, sorry. I'm happy that at least your check has not been a waste of time for other reasons. Thanks, Paolo > -- > Jens Axboe >