On 9/28/22 08:12, Jens Axboe wrote:
> On 9/27/22 5:07 PM, 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;
>>
>> 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;
>>
>> }
>> }
>
> I'd be fine with that (using a shared helper), but let's please just
> make it:
>
> static inline bool op_is_zoned_write(bdev, op)
> {
> if (!bdev_is_zoned(bio->bi_bdev))
> return false;
>
> return op == REQ_OP_WRITE_ZEROES || op == REQ_OP_WRITE;
Works for me. Nit: should have REQ_OP_WRITE first as that is the most
common case.
> }
>
> and avoid a switch for this basic case and name it a bit more logically
> too. Not married to the above name, but the helper should not imply
> anything about zone locking. That's for the caller.
blk_req_needs_zone_write_lock() would become:
bool blk_req_needs_zone_write_lock(struct request *rq)
{
if (blk_rq_is_passthrough(rq))
return false;
if (!rq->q->disk->seq_zones_wlock)
return false;
return op_is_zoned_write(rq->q->disk->part0, req_op(rq));
}
--
Damien Le Moal
Western Digital Research
