linux-kernel - Re: [PATCH 2/2] Revert "mq-deadline: Fix request accounting"

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]

Date:   Tue, 7 Sep 2021 16:07:57 +0000
From:   Niklas Cassel <Niklas.Cassel@....com>
To:     Bart Van Assche <bvanassche@....org>
CC:     Jens Axboe <axboe@...nel.dk>,
        Damien Le Moal <Damien.LeMoal@....com>,
        Paolo Valente <paolo.valente@...aro.org>,
        Ming Lei <ming.lei@...hat.com>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] Revert "mq-deadline: Fix request accounting"

On Tue, Sep 07, 2021 at 07:54:09AM -0700, Bart Van Assche wrote:
> On 9/7/21 7:21 AM, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@....com>
> > 
> > This reverts commit b6d2b054e8baaee53fd2d4854c63cbf0f2c6262a.
> > 
> > blk-mq will no longer call the I/O scheduler .finish_request() callback
> > for requests that were never inserted to the I/O scheduler.
> > 
> > Therefore, we can remove the logic inside mq-deadline that was added to
> > workaround the (no longer existing) quirky behavior of blk-mq.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@....com>
> > ---
> >   block/mq-deadline.c | 16 +++++-----------
> >   1 file changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> > index 7f3c3932b723..b2d1e3adcb39 100644
> > --- a/block/mq-deadline.c
> > +++ b/block/mq-deadline.c
> > @@ -678,7 +678,6 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
> >   	prio = ioprio_class_to_prio[ioprio_class];
> >   	dd_count(dd, inserted, prio);
> > -	rq->elv.priv[0] = (void *)(uintptr_t)1;
> >   	if (blk_mq_sched_try_insert_merge(q, rq, &free)) {
> >   		blk_mq_free_requests(&free);
> > @@ -727,10 +726,12 @@ static void dd_insert_requests(struct blk_mq_hw_ctx *hctx,
> >   	spin_unlock(&dd->lock);
> >   }
> > -/* Callback from inside blk_mq_rq_ctx_init(). */
> > +/*
> > + * Nothing to do here. This is defined only to ensure that .finish_request
> > + * method is called upon request completion.
> > + */
> >   static void dd_prepare_request(struct request *rq)
> >   {
> > -	rq->elv.priv[0] = NULL;
> >   }
> 
> Please take a look at
> https://lore.kernel.org/linux-block/18594aff-4a94-8a48-334c-f21ae32120c6@acm.org/
> If dd_prepare_request() is removed I will have to reintroduce it.

dd_prepare_request() itself is not removed, just the
rq->elv.priv[0] = NULL; inside dd_prepare_request().

If you need to modify dd_prepare_request() in a future
commit, that should be fine, no?

Without patch 1/2, e->type->ops.requeue_request() can get called
both for requests that bypassed the I/O scheduler, and for requests
that were inserted in the I/O scheduler.

See:
block/blk-mq-sched.h:blk_mq_sched_requeue_request()
If the RQF_ELVPRIV flag is not set, e->type->ops.requeue_request()
will not be called.

Perhaps you are having issues with requests that were inserted
in the scheduler, but later requeued?

If so, shouldn't these fixes help you, since you do not need to
worry about passthrough requests resulting in spurious calls to
the I/O scheduler callbacks?


Kind regards,
Niklas