| Message ID | 20230816195447.3703954-5-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve performance for zoned UFS devices | expand |
On 8/17/23 04:53, Bart Van Assche wrote: > Introduce the .eh_prepare_resubmit function pointer in struct scsi_driver. > Make the error handler call .eh_prepare_resubmit() before resubmitting > commands. A later patch will use this functionality to sort SCSI commands > by LBA from inside the SCSI disk driver. > > 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 | 1 + > 3 files changed, 67 insertions(+) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index c67cdcdc3ba8..4393a7fd8a07 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 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... > + > + /* Sort pending SCSI commands by ULD. */ > + list_sort(NULL, done_q, scsi_cmp_uld); > + > + /* > + * Call .eh_prepare_resubmit for each range of commands with identical > + * ULD driver pointer. > + */ > + list_for_each_entry_safe(scmd, next, done_q, eh_entry) { > + struct scsi_driver *uld = scsi_cmd_to_driver(scmd); > + struct list_head *prev, uld_cmd_list; > + > + while (&next->eh_entry != done_q && > + scsi_cmd_to_driver(next) == uld) > + next = list_next_entry(next, eh_entry); > + if (!uld->eh_prepare_resubmit) > + continue; > + prev = scmd->eh_entry.prev; > + list_cut_position(&uld_cmd_list, prev, next->eh_entry.prev); > + uld->eh_prepare_resubmit(&uld_cmd_list); > + list_splice(&uld_cmd_list, prev); > + } > +} > + > /** > * scsi_eh_flush_done_q - finish processed commands or retry them. > * @done_q: list_head of processed commands. > @@ -2194,6 +2257,8 @@ void scsi_eh_flush_done_q(struct list_head *done_q) > { > struct scsi_cmnd *scmd, *next; > > + scsi_call_prepare_resubmit(done_q); > + > list_for_each_entry_safe(scmd, next, done_q, eh_entry) { > list_del_init(&scmd->eh_entry); > if (scsi_device_online(scmd->device) && > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h > index f42388ecb024..df4af4645430 100644 > --- a/drivers/scsi/scsi_priv.h > +++ b/drivers/scsi/scsi_priv.h > @@ -101,6 +101,7 @@ int scsi_eh_get_sense(struct list_head *work_q, > struct list_head *done_q); > bool scsi_noretry_cmd(struct scsi_cmnd *scmd); > void scsi_eh_done(struct scsi_cmnd *scmd); > +void scsi_call_prepare_resubmit(struct list_head *done_q); > > /* scsi_lib.c */ > extern int scsi_maybe_unblock_host(struct scsi_device *sdev); > diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h > index 4ce1988b2ba0..2b11be896eee 100644 > --- a/include/scsi/scsi_driver.h > +++ b/include/scsi/scsi_driver.h > @@ -18,6 +18,7 @@ struct scsi_driver { > int (*done)(struct scsi_cmnd *); > int (*eh_action)(struct scsi_cmnd *, int); > void (*eh_reset)(struct scsi_cmnd *); > + void (*eh_prepare_resubmit)(struct list_head *cmd_list); > }; > #define to_scsi_driver(drv) \ > container_of((drv), struct scsi_driver, gendrv)
On 8/17/23 04:10, Damien Le Moal wrote: > On 8/17/23 04:53, Bart Van Assche wrote: >> +/* >> + * 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. Thanks, Bart.
On 2023/08/17 23:26, Bart Van Assche wrote: > On 8/17/23 04:10, Damien Le Moal wrote: >> On 8/17/23 04:53, Bart Van Assche wrote: >>> +/* >>> + * 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.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index c67cdcdc3ba8..4393a7fd8a07 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 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; + + /* Sort pending SCSI commands by ULD. */ + list_sort(NULL, done_q, scsi_cmp_uld); + + /* + * Call .eh_prepare_resubmit for each range of commands with identical + * ULD driver pointer. + */ + list_for_each_entry_safe(scmd, next, done_q, eh_entry) { + struct scsi_driver *uld = scsi_cmd_to_driver(scmd); + struct list_head *prev, uld_cmd_list; + + while (&next->eh_entry != done_q && + scsi_cmd_to_driver(next) == uld) + next = list_next_entry(next, eh_entry); + if (!uld->eh_prepare_resubmit) + continue; + prev = scmd->eh_entry.prev; + list_cut_position(&uld_cmd_list, prev, next->eh_entry.prev); + uld->eh_prepare_resubmit(&uld_cmd_list); + list_splice(&uld_cmd_list, prev); + } +} + /** * scsi_eh_flush_done_q - finish processed commands or retry them. * @done_q: list_head of processed commands. @@ -2194,6 +2257,8 @@ void scsi_eh_flush_done_q(struct list_head *done_q) { struct scsi_cmnd *scmd, *next; + scsi_call_prepare_resubmit(done_q); + list_for_each_entry_safe(scmd, next, done_q, eh_entry) { list_del_init(&scmd->eh_entry); if (scsi_device_online(scmd->device) && diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index f42388ecb024..df4af4645430 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -101,6 +101,7 @@ int scsi_eh_get_sense(struct list_head *work_q, struct list_head *done_q); bool scsi_noretry_cmd(struct scsi_cmnd *scmd); void scsi_eh_done(struct scsi_cmnd *scmd); +void scsi_call_prepare_resubmit(struct list_head *done_q); /* scsi_lib.c */ extern int scsi_maybe_unblock_host(struct scsi_device *sdev); diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h index 4ce1988b2ba0..2b11be896eee 100644 --- a/include/scsi/scsi_driver.h +++ b/include/scsi/scsi_driver.h @@ -18,6 +18,7 @@ struct scsi_driver { int (*done)(struct scsi_cmnd *); int (*eh_action)(struct scsi_cmnd *, int); void (*eh_reset)(struct scsi_cmnd *); + void (*eh_prepare_resubmit)(struct list_head *cmd_list); }; #define to_scsi_driver(drv) \ container_of((drv), struct scsi_driver, gendrv)
Introduce the .eh_prepare_resubmit function pointer in struct scsi_driver. Make the error handler call .eh_prepare_resubmit() before resubmitting commands. A later patch will use this functionality to sort SCSI commands by LBA from inside the SCSI disk driver. 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 | 1 + 3 files changed, 67 insertions(+)