linux-kernel - Re: IO request merging

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: <20141031083022.GA24172@quack.suse.cz>
Date:	Fri, 31 Oct 2014 09:30:22 +0100
From:	Jan Kara <jack@...e.cz>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	Jan Kara <jack@...e.cz>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: IO request merging

On Thu 30-10-14 20:56:14, Jan Kara wrote:
> On Thu 16-10-14 10:10:39, Jens Axboe wrote:
> > On 10/16/2014 06:27 AM, Jan Kara wrote:
> > >   Hello,
> > > 
> > >   one of our customers was complaining that elv_attempt_insert_merge()
> > > merges two requests (via blk_attempt_req_merge()) without asking IO
> > > scheduler for permission (->elevator_allow_merge_fn() callback). Now for
> > > them this is a problem because of their custom IO scheduler but looking
> > > into the code this can result in somewhat suboptimal behavior for CFQ as
> > > well (merging two requests from different IO contexts, possibly merging
> > > sync & async request). What do others think about this?
> > > 
> > > Regarding possible fix, we cannot really call ->elevator_allow_merge_fn()
> > > because that assumes it is called from a context of a process submitting the
> > > passed bio. So we would need to create a separate allow merge callback for
> > > this.
>   Sorry for a delayed reply, I was asking the customer for some more
> details and it took them a while to get back to me...
> 
> > It would need a new (rq to rq) merge hook, if they have a custom IO
> > scheduler, they should submit a change to allow that kind of behaviour.
>   OK, but since they would be the only ones using the hook, I don't think
> upstream kernel would be that much interested in carrying it... That's why
> I was asking whether CFQ wouldn't use the hook as well. But from what you
> write below, I tend to agree that it would be an overkill for CFQ.
> 
> > Outside of potentially mixing sync and async IO (which seems like
> > something that should rarely/never happen), not sure I see a lot of
> > downsides. And that case could be explicitly checked in attempt_merge()
> > or blk_attempt_req_merge() without having to define a new hook to catch
> > that specific case. For the hook, cfq would lookup the io contexts and
> > compare, and basically disallow any merge that crosses a cfq io context
> > boundary. But given that I would only expect these types of merges to
> > happen very rarely, the sync vs async check would be good enough for me.
>   Yeah. So what is a real problem for their custom scheduler is when two
> requests with different IO priorities get merged (BTW, ioprio_best() has
> a bug which they found and I just submitted a patch to fix it). For some
> reason they don't want requests with different priorities merged (even if
> resulting priority is computed properly). And we don't want checks like
> this in generic code.
  Thinking about it a bit more - is it really that beneficial to merge
requests with different priorities? I wouldn't expect that to happen often
enough to bring significant improvement in request sizes. Or do you have
some usecase for that?

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/