Re: [PATCH v9 06/17] scsi: sd: Sort commands by LBA before resubmitting

[Damien Le Moal <dlemoal@kernel.org>]

On 2023/08/17 23:34, Bart Van Assche wrote:
> On 8/17/23 04:13, Damien Le Moal wrote:
>> On 8/17/23 04:53, Bart Van Assche wrote:
>>> +static int sd_cmp_sector(void *priv, const struct list_head *_a,
>>> +			 const struct list_head *_b)
>>> +{
>>> +	struct scsi_cmnd *a = list_entry(_a, typeof(*a), eh_entry);
>>> +	struct scsi_cmnd *b = list_entry(_b, typeof(*b), eh_entry);
>>> +	struct request *rq_a = scsi_cmd_to_rq(a);
>>> +	struct request *rq_b = scsi_cmd_to_rq(b);
>>> +	bool use_zwl_a = rq_a->q->limits.use_zone_write_lock;
>>> +	bool use_zwl_b = rq_b->q->limits.use_zone_write_lock;
>>> +
>>> +	/*
>>> +	 * Order the commands that need zone write locking after the commands
>>> +	 * that do not need zone write locking. Order the commands that do not
>>> +	 * need zone write locking by LBA. Do not reorder the commands that
>>> +	 * need zone write locking. See also the comment above the list_sort()
>>> +	 * definition.
>>> +	 */
>>> +	if (use_zwl_a || use_zwl_b)
>>> +		return use_zwl_a > use_zwl_b;
>>> +	return blk_rq_pos(rq_a) > blk_rq_pos(rq_b);
>>> +}
>>> +
>>> +static void sd_prepare_resubmit(struct list_head *cmd_list)
>>> +{
>>> +	/*
>>> +	 * Sort pending SCSI commands in starting sector order. This is
>>> +	 * important if one of the SCSI devices associated with @shost is a
>>> +	 * zoned block device for which zone write locking is disabled.
>>> +	 */
>>> +	list_sort(NULL, cmd_list, sd_cmp_sector);
>>
>> We should not need to do this for regular devices or zoned devices using zone
>> write locking. So returning doing nothing would be better but the callback
>> lacks a pointer to the scsi_device to test that.
> 
> @cmd_list may include SCSI commands for multiple SCSI devices so I'm not 
> sure which SCSI device pointer you are referring to in the above comment?

This is called from scsi_eh_flush_done_q() which also does not have the scsi
device pointer. And places where this is called have the scsi host pointer, not
the device. OK. So let's no do that.

> 
> Thanks,
> 
> Bart.
> 

-- 
Damien Le Moal
Western Digital Research