| Message ID | 20230816195447.3703954-7-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve performance for zoned UFS devices | expand |
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);
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.
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 --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);
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(+)