Re: IO request merging

[Jan Kara <jack@suse.cz>]

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@suse.cz>
SUSE Labs, CR