Re: [PATCH 1/2] block: modify blk_mq_plug() to allow only reads for zoned block devices

[Jens Axboe <axboe@kernel.dk>]

On 9/26/22 1:20 PM, Pankaj Raghav wrote:
> On 2022-09-26 18:32, Jens Axboe wrote:
>> On 9/26/22 8:43 AM, Christoph Hellwig wrote:
>>> On Mon, Sep 26, 2022 at 08:40:54AM -0600, Jens Axboe wrote:
>>>> On 9/26/22 8:37 AM, Christoph Hellwig wrote:
>>>>> On Sun, Sep 25, 2022 at 08:53:46PM +0200, Pankaj Raghav wrote:
>>>>>> Modify blk_mq_plug() to allow plugging only for read operations in zoned
>>>>>> block devices as there are alternative IO paths in the linux block
>>>>>> layer which can end up doing a write via driver private requests in
>>>>>> sequential write zones.
>>>>>
>>>>> We should be able to plug for all operations that are not
>>>>> REQ_OP_ZONE_APPEND just fine.
>>>>
>>>> Agree, I think we just want to make this about someone doing a series
>>>> of appends. If you mix-and-match with passthrough you will have a bad
>>>> time anyway.
>>>
>>> Err, sorry - what I wrote about is compelte garbage.  I initially
>>> wanted to say you can plug for REQ_OP_ZONE_APPEND just fine, and then
>>> realized that we also want various other ones that have the write bit
>>> set batched.  So I suspect we really want to explicitly check for
>>> REQ_OP_WRITE here.
>>
>> My memory was a bit hazy, since we have separate ops for the driver
>> in/out, I think just checking for REQ_OP_WRITE is indeed the right
>> choice. That's the single case we need to care about.
>>
> Ah. You are right. I missed it as well. There is even a comment from
> Christoph:
> 
>  *   - if the least significant bit is set transfers are TO the device
>  *   - if the least significant bit is not set transfers are FROM the device
> 
> I guess the second patch should be enough to apply plugging when
> applicable for uring_cmd based nvme passthrough requests.

Do we even need the 2nd patch? If we're just doing passthrough for the
blk_execute_nowait() API, then the condition should never trigger? If
so, then it would be a cleanup just to ensure we're using a consistent
API for getting the plug, which may be worthwhile to do separately for
sure.

-- 
Jens Axboe