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: <20141030195614.GL28444@quack.suse.cz>
Date:	Thu, 30 Oct 2014 20:56:14 +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 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.

								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/