Re: [PATCH v9 04/17] scsi: core: Call .eh_prepare_resubmit() before resubmitting

On 2023/08/17 23:26, Bart Van Assche wrote:

On 8/17/23 04:10, Damien Le Moal wrote:

quoted

On 8/17/23 04:53, Bart Van Assche wrote:

quoted

+/*
+ * Returns true if the commands in @done_q should be sorted in LBA order
+ * before being resubmitted.
+ */
+static bool scsi_needs_sorting(struct list_head *done_q)
+{
+	struct scsi_cmnd *scmd;
+
+	list_for_each_entry(scmd, done_q, eh_entry) {
+		struct request *rq = scsi_cmd_to_rq(scmd);
+
+		if (!rq->q->limits.use_zone_write_lock &&
+		    blk_rq_is_seq_zoned_write(rq))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Comparison function that allows to sort SCSI commands by ULD driver.
+ */
+static int scsi_cmp_uld(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);
+
+	/* See also the comment above the list_sort() definition. */
+	return scsi_cmd_to_driver(a) > scsi_cmd_to_driver(b);
+}
+
+void scsi_call_prepare_resubmit(struct list_head *done_q)
+{
+	struct scsi_cmnd *scmd, *next;
+
+	if (!scsi_needs_sorting(done_q))
+		return;

This is strange. The eh_prepare_resubmit callback is generic and its name does
not indicate anything related to sorting by LBAs. So this check would prevent
other actions not related to sorting by LBA. This should go away.

In patch 6, based on the device characteristics, the sd driver should decides
if it needs to set .eh_prepare_resubmit or not.

And ideally, if all ULDs have eh_prepare_resubmit == NULL, this function should
return here before going through the list of commands to resubmit. Given that
this list should generally be small, going through it and doing nothing should
be OK though...

I can add a eh_prepare_resubmit == NULL check early in 
scsi_call_prepare_resubmit(). Regarding the code inside 
scsi_needs_sorting(), how about moving that code into an additional 
callback function, e.g. eh_needs_prepare_resubmit? Setting 
.eh_prepare_resubmit depending on the zone model would prevent 
constification of struct scsi_driver.

Sounds OK.

Thanks,

Bart.

-- 
Damien Le Moal
Western Digital Research