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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 9/28/22 08:07, Damien Le Moal wrote:
> On 9/28/22 01:52, Jens Axboe wrote:
>> On 9/27/22 10:51 AM, Christoph Hellwig wrote:
>>> On Tue, Sep 27, 2022 at 10:04:19AM -0600, Jens Axboe wrote:
>>>> Ah yes, good point. We used to have this notion of 'fs' request, don't
>>>> think we do anymore. Because it really should just be:
>>>
>>> A fs request is a !passthrough request.
>>
>> Right, that's the condition I made below too.
>>
>>>> if (zoned && (op & REQ_OP_WRITE) && fs_request)
>>>>          return NULL;
>>>>
>>>> for that condition imho. I guess we could make it:
>>>>
>>>> if (zoned && (op & REQ_OP_WRITE) && !(op & REQ_OP_DRV_OUT))
>>>>          return NULL;
>>>
>>> Well, the only opcodes we do zone locking for is REQ_OP_WRITE and
>>> REQ_OP_WRITE_ZEROES.  So this should be:
>>>
>>> 	if (zoned && (op == REQ_OP_WRITE || op == REQ_OP_WRITE_ZEROES))
>>> 		return NULL;
>>
>> I'd rather just make it explicit and use that. Pankaj, do you want
>> to spin a v2 with that?
> 
> It would be nice to reuse the bio equivalent of
> blk_req_needs_zone_write_lock().
> 
> The test would be:
> 
> 	if (bio_needs_zone_write_locking())
> 		return NULL;

Note that we could also add a "IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&" to the
condition or stub the helper to have this hunk disappear for the
!CONFIG_BLK_DEV_ZONED case.

> 
> With something like:
> 
> static inline bool bio_needs_zone_write_locking()
> {
> 	 if (!bdev_is_zoned(bio->bi_bdev))
> 		return false;
> 
> 	switch (bio_op(bio)) {
>         case REQ_OP_WRITE_ZEROES:
> 
>         case REQ_OP_WRITE:
> 
>                 return true;
>         default:
> 
>                 return false;
> 
>         }
> }
> 
> Which also has the advantage that going forward, we could refine this to
> plug writes to conventional zones (as these can be plugged).
> 

-- 
Damien Le Moal
Western Digital Research