Messages in this thread | | | Date | Fri, 31 Oct 2014 09:30:22 +0100 | From | Jan Kara <> | 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@suse.cz> SUSE Labs, CR
| |