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
