Re: [PATCH v11 04/16] scsi: core: Introduce a mechanism for reordering requests in the error handler

[Hannes Reinecke <hare@suse.de>]

On 8/22/23 21:16, Bart Van Assche wrote:
> Introduce the .eh_needs_prepare_resubmit and the .eh_prepare_resubmit
> function pointers in struct scsi_driver. Make the error handler call
> .eh_prepare_resubmit() before resubmitting commands if any of the
> .eh_needs_prepare_resubmit() invocations return true. A later patch
> will use this functionality to sort SCSI commands by LBA from inside
> the SCSI disk driver before these are resubmitted by the error handler.
> 
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_error.c  | 65 ++++++++++++++++++++++++++++++++++++++
>   drivers/scsi/scsi_priv.h   |  1 +
>   include/scsi/scsi_driver.h |  2 ++
>   3 files changed, 68 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index c67cdcdc3ba8..c4d817f044a0 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -27,6 +27,7 @@
>   #include <linux/blkdev.h>
>   #include <linux/delay.h>
>   #include <linux/jiffies.h>
> +#include <linux/list_sort.h>
>   
>   #include <scsi/scsi.h>
>   #include <scsi/scsi_cmnd.h>
> @@ -2186,6 +2187,68 @@ void scsi_eh_ready_devs(struct Scsi_Host *shost,
>   }
>   EXPORT_SYMBOL_GPL(scsi_eh_ready_devs);
>   
> +/*
> + * Returns true if .eh_prepare_resubmit should be called for the commands in
> + * @done_q.
> + */
> +static bool scsi_needs_preparation(struct list_head *done_q)
> +{
> +	struct scsi_cmnd *scmd;
> +
> +	list_for_each_entry(scmd, done_q, eh_entry) {
> +		struct scsi_driver *uld = scsi_cmd_to_driver(scmd);
> +		bool (*npr)(struct scsi_cmnd *) = uld->eh_needs_prepare_resubmit;
> +
> +		if (npr && npr(scmd))
> +			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);

I have to agree with Christoph here.
Comparing LBA numbers at the SCSI level is really the wrong place.
SCSI commands might be anything, and quite some of these commands don't
even have LBA numbers. So trying to order them will be pointless.

The reordering mechanism really has to go into the block layer, with
the driver failing the request and the block layer resubmitting in-order.

Sorry.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman