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

[Jens Axboe <axboe@fb.com>]

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.

-- 
Jens Axboe