diff mbox series

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

Message ID 20230816195447.3703954-7-bvanassche@acm.org (mailing list archive)
State Superseded
Headers show
Series Improve performance for zoned UFS devices | expand

Commit Message

Bart Van Assche Aug. 16, 2023, 7:53 p.m. UTC
Sort SCSI commands by LBA before the SCSI error handler resubmits
these commands. This is necessary when resubmitting zoned writes
(REQ_OP_WRITE) if multiple writes have been queued for a single zone.

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/sd.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Damien Le Moal Aug. 17, 2023, 11:13 a.m. UTC | #1
On 8/17/23 04:53, Bart Van Assche wrote:
> Sort SCSI commands by LBA before the SCSI error handler resubmits
> these commands. This is necessary when resubmitting zoned writes
> (REQ_OP_WRITE) if multiple writes have been queued for a single zone.
> 
> 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/sd.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3c668cfb146d..8a4b0874e7fe 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -47,6 +47,7 @@
>  #include <linux/blkpg.h>
>  #include <linux/blk-pm.h>
>  #include <linux/delay.h>
> +#include <linux/list_sort.h>
>  #include <linux/major.h>
>  #include <linux/mutex.h>
>  #include <linux/string_helpers.h>
> @@ -117,6 +118,7 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt);
>  static int sd_done(struct scsi_cmnd *);
>  static void sd_eh_reset(struct scsi_cmnd *);
>  static int sd_eh_action(struct scsi_cmnd *, int);
> +static void sd_prepare_resubmit(struct list_head *cmd_list);
>  static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
>  static void scsi_disk_release(struct device *cdev);
>  
> @@ -617,6 +619,7 @@ static struct scsi_driver sd_template = {
>  	.done			= sd_done,
>  	.eh_action		= sd_eh_action,
>  	.eh_reset		= sd_eh_reset,
> +	.eh_prepare_resubmit	= sd_prepare_resubmit,
>  };
>  
>  /*
> @@ -2018,6 +2021,38 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
>  	return eh_disp;
>  }
>  
> +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.

> +}
> +
>  static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
>  {
>  	struct request *req = scsi_cmd_to_rq(scmd);
Bart Van Assche Aug. 17, 2023, 2:34 p.m. UTC | #2
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?

Thanks,

Bart.
Damien Le Moal Aug. 18, 2023, 2:41 a.m. UTC | #3
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.
>
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3c668cfb146d..8a4b0874e7fe 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -47,6 +47,7 @@ 
 #include <linux/blkpg.h>
 #include <linux/blk-pm.h>
 #include <linux/delay.h>
+#include <linux/list_sort.h>
 #include <linux/major.h>
 #include <linux/mutex.h>
 #include <linux/string_helpers.h>
@@ -117,6 +118,7 @@  static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
 static void sd_eh_reset(struct scsi_cmnd *);
 static int sd_eh_action(struct scsi_cmnd *, int);
+static void sd_prepare_resubmit(struct list_head *cmd_list);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 
@@ -617,6 +619,7 @@  static struct scsi_driver sd_template = {
 	.done			= sd_done,
 	.eh_action		= sd_eh_action,
 	.eh_reset		= sd_eh_reset,
+	.eh_prepare_resubmit	= sd_prepare_resubmit,
 };
 
 /*
@@ -2018,6 +2021,38 @@  static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 	return eh_disp;
 }
 
+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);
+}
+
 static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 {
 	struct request *req = scsi_cmd_to_rq(scmd);