Re: [PATCH] blk-mq: don't use rw_is_sync() to determine sync request

[Shaohua Li]

On Mon, Dec 01, 2014 at 07:43:37PM -0700, Jens Axboe wrote:
> On 12/01/2014 11:59 AM, Shaohua Li wrote:
> >On Sun, Nov 30, 2014 at 07:57:12PM -0800, Shaohua Li wrote:
> >>On Sun, Nov 30, 2014 at 06:35:11PM -0700, Jens Axboe wrote:
> >>>On 11/30/2014 05:01 PM, Shaohua Li wrote:
> >>>>Buffer read is counted as sync in rw_is_sync(). If we use it,
> >>>>blk_sq_make_request() will not do per-process plug any more.
> >>>>
> >>>>I haven't changed blk_mq_make_request() yet. It makes sense to dispatch
> >>>>REQ_SYNC request immediately. But for buffer read, it's weird not to do
> >>>>per-process plug, as buffer read doesn't need low latency.
> >>>>blk_mq_merge_queue_io() isn't very helpful, as we don't have delay mechanism
> >>>>there, the queue is immediately flushed, which makes the merge very
> >>>>superficial.
> >>>
> >>>A read is sync, buffered or not. A buffered read is every bit as
> >>>latency sensitive as an O_DIRECT read. I think it'd be fine to
> >>>modify rw_is_sync() to disregard REQ_AHEAD as sync (and ensure it's
> >>>carried forward in the request flags, too). At least to the extent
> >>>that we process plug and get the merging, since for streamed reads
> >>>we'd soon be waiting on them anyway.
> >>
> >>A quick search shows nobody uses REQ_AHEAD. For stream reads, only first several
> >>reads are waited I suppose, later reads are read ahead. Maybe only counts
> >>REQ_META read as sync?
> >
> >Changing rw_is_sync() sounds risky, as it will change behavior of other parts,
> >like CFQ. REQ_META/REQ_PRIO isn't an option, metadata does readahead too.
> >And nobody uses REQ_AHEAD. explictly checking REQ_SYNC in blk_sq_make_request()
> >sounds better, which is just for pluging and we use it for ages in
> >blk_queue_bio().
> 
> I'm not really disagreeing with you. The per-task plugging isn't a
> true delay mechanism like the old plugging was, and there's no
> question it makes sense to do on the single queue. For the multi
> queue, it's a bit more tricky. If it's truly a 1:1 cpu:queue
> mapping, then we can safely assume that we might as well execute it.
> Unless we can do batched submission, which would (somewhat) rely on
> having chains of requests to submit, which we'd only really get if
> we plug.
> 
> The fact that RAHEAD isn't currently really wired up is a shame, and
> it really should be. It might be problematic due to how we mix it up
> with failfast.
> 
> For blk_sq_make_request(), we should just make the change.

How about the new patch?