Re: [PATCH v5 10/11] block: mq-deadline: Handle requeued requests correctly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

I tried and it does not work. The compiler does not remove the if block for the
!CONFIG_BLK_DEV_ZONED case. Unfortunate.

> 
> Thanks,
> 
> Bart.
> 

-- 
Damien Le Moal
Western Digital Research




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux