Re: [PATCH 2/8] block: Introduce the blk_rq_is_seq_zone_write() function

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

On 1/10/23 21:13, Niklas Cassel wrote:
> On Tue, Jan 10, 2023 at 08:54:24PM +0900, Damien Le Moal wrote:
>> On 1/10/23 18:52, Niklas Cassel wrote:
>>> On Mon, Jan 09, 2023 at 03:52:23PM -0800, Bart Van Assche wrote:
>>>> On 1/9/23 15:38, Damien Le Moal wrote:
>>>>> On 1/10/23 08:27, Bart Van Assche wrote:
>>>>>> +static inline bool blk_rq_is_seq_zone_write(struct request *rq)
>>>>>> +{
>>>>>> +	switch (req_op(rq)) {
>>>>>> +	case REQ_OP_WRITE:
>>>>>> +	case REQ_OP_WRITE_ZEROES:
>>>>>
>>>>> REQ_OP_ZONE_APPEND ?
>>>>
>>>> I will add REQ_OP_ZONE_APPEND.
>>>>
>>>
>>> Hello Bart, Damien,
>>>
>>> +       if (blk_queue_pipeline_zoned_writes(rq->q) &&
>>> +           blk_rq_is_seq_zone_write(rq))
>>> +               cmd->allowed += rq->q->nr_requests;
>>>
>>> Considering that this function, blk_rq_is_seq_zone_write(), only seems to
>>> be used to determine if a request should be allowed to be retried, I think
>>> that it is incorrect to add REQ_OP_ZONE_APPEND, since a zone append
>>> operation will never result in a ILLEGAL REQUEST/UNALIGNED WRITE COMMAND.
>>>
>>> (If this instead was a function that said which operations that needed to
>>> be held back, then you would probably need to include REQ_OP_ZONE_APPEND,
>>> as otherwise the reordered+retried write would never be able to succeed.)
>>
>> Unless UFS defines a zone append operation, REQ_OP_ZONE_APPEND will be
>> processed using regular writes in the sd driver.
> 
> Sure, but I still think that my point is valid.
> 
> A REQ_OP_ZONE_APPEND should never be able to result in a
> "UNALIGNED WRITE COMMAND".

Yes, but that semantic should not be associated with a function named
blk_rq_is_seq_zone_write() :)

> 
> If the SCSI REQ_OP_ZONE_APPEND emulation can result in a
> "UNALIGNED WRITE COMMAND", I would argue that the SCSI zone append
> emulation is faulty.

or a passthrough command was used and screwed up the zone write pointer
tracking....

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research