On 5/18/23 01:28, Bart Van Assche wrote:
> On 5/16/23 18:22, Damien Le Moal wrote:
>> On 5/17/23 07:33, Bart Van Assche wrote:
>>> -/* Return the first request for which blk_rq_pos() >= pos. */
>>> +/*
>>> + * Return the first request for which blk_rq_pos() >= @pos. For zoned devices,
>>> + * return the first request after the highest zone start <= @pos.
>>
>> This comment is strange. What about:
>>
>> For zoned devices, return the first request after the start of the zone
>> containing @pos.
>
> I will make this change.
>
>>> @@ -818,7 +835,20 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>>> * set expire time and add to fifo list
>>> */
>>> rq->fifo_time = jiffies + dd->fifo_expire[data_dir];
>>> - list_add_tail(&rq->queuelist, &per_prio->fifo_list[data_dir]);
>>> + insert_before = &per_prio->fifo_list[data_dir];
>>> +#ifdef CONFIG_BLK_DEV_ZONED
>>> + /*
>>> + * Insert zoned writes such that requests are sorted by
>>> + * position per zone.
>>> + */
>>> + if (blk_rq_is_seq_zoned_write(rq)) {
>>> + struct request *rq2 = deadline_latter_request(rq);
>>> +
>>> + if (rq2 && blk_rq_zone_no(rq2) == blk_rq_zone_no(rq))
>>> + insert_before = &rq2->queuelist;
>>> + }
>>> +#endif
>>> + list_add_tail(&rq->queuelist, insert_before);
>>
>> Why not:
>>
>> insert_before = &per_prio->fifo_list[data_dir];
>> if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)
>> && blk_rq_is_seq_zoned_write(rq)) {
>> /*
>> * Insert zoned writes such that requests are sorted by
>> * position per zone.
>> */
>> struct request *rq2 = deadline_latter_request(rq);
>>
>> if (rq2 && blk_rq_zone_no(rq2) == blk_rq_zone_no(rq))
>> insert_before = &rq2->queuelist;
>> }
>> list_add_tail(&rq->queuelist, insert_before);
>>
>> to avoid the ugly #ifdef ?
>
> I think the above code won't build because no blk_rq_zone_no()
> definition is available if CONFIG_BLK_DEV_ZONED=n. I prefer it this way
> because my view is that using blk_rq_zone_no() if CONFIG_BLK_DEV_ZONED=n
> is wrong.
The compiler should drop the entire if block for CONFIG_BLK_DEV_ZONED=n case.
Worth trying to check as the code is much nicer without the #ifdef.
I will test this series today and check.
>
> Thanks,
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
