| Message ID | 20231018175602.2148415-6-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve write performance for zoned UFS devices | expand |
On 10/18/23 17:24, Damien Le Moal wrote: > On 10/19/23 02:54, Bart Van Assche wrote: >> +void scsi_call_prepare_resubmit(struct list_head *done_q) >> +{ >> + struct scsi_cmnd *scmd, *next; >> + >> + if (!scsi_needs_preparation(done_q)) >> + return; > > This function will always go through the list of commands in done_q. That could > hurt performance for scsi hosts that do not need this prepare resubmit, which I > think is UFS only for now. So can't we just add a flag or something to avoid that ? Hi Damien, The SCSI error handler is only invoked after an RCU grace period has expired. The time spent in scsi_needs_preparation() is negligible compared to an RCU grace period, especially if the .eh_needs_prepare_resubmit pointers are NULL. Thanks, Bart.
On 10/19/23 10:53, Bart Van Assche wrote: > > On 10/18/23 17:24, Damien Le Moal wrote: >> On 10/19/23 02:54, Bart Van Assche wrote: >>> +void scsi_call_prepare_resubmit(struct list_head *done_q) >>> +{ >>> + struct scsi_cmnd *scmd, *next; >>> + >>> + if (!scsi_needs_preparation(done_q)) >>> + return; >> >> This function will always go through the list of commands in done_q. >> That could >> hurt performance for scsi hosts that do not need this prepare >> resubmit, which I >> think is UFS only for now. So can't we just add a flag or something to >> avoid that ? > > The SCSI error handler is only invoked after an RCU grace period has > expired. The time spent in scsi_needs_preparation() is negligible > compared to an RCU grace period, especially if the > .eh_needs_prepare_resubmit pointers are NULL. (replying to my own e-mail) Do you perhaps want me to drop the eh_needs_prepare_resubmit function pointer and introduce a flag instead? That sounds good to me. Thanks, Bart.
On 10/20/23 02:53, Bart Van Assche wrote: > > On 10/18/23 17:24, Damien Le Moal wrote: >> On 10/19/23 02:54, Bart Van Assche wrote: >>> +void scsi_call_prepare_resubmit(struct list_head *done_q) >>> +{ >>> + struct scsi_cmnd *scmd, *next; >>> + >>> + if (!scsi_needs_preparation(done_q)) >>> + return; >> >> This function will always go through the list of commands in done_q. That could >> hurt performance for scsi hosts that do not need this prepare resubmit, which I >> think is UFS only for now. So can't we just add a flag or something to avoid that ? > > Hi Damien, > > The SCSI error handler is only invoked after an RCU grace period has > expired. The time spent in scsi_needs_preparation() is negligible > compared to an RCU grace period, especially if the > .eh_needs_prepare_resubmit pointers are NULL. I was thinking in the context of the scsi disk driver, which is the most widely used scsi driver and does have eh_needs_prepare_resubmit set. And I now recall that we have already discussed this when I suggested passing the scsi_host as argument here to avoid that loop for hosts that do not need to do that reordering (that is, everything but ufs hosts). You did mention that this is not easily feasible if I remember correctly. That is really too bad. It would be nice to avoid this loop when it is not needed. But given that this is the error path, may be that is OK. I'll stay neutral on this one for now. I need to run some performance tests. FYI, libata triggers scsi_eh to process the completion of commands with CDL set and sense data available, to determine if the commands were aborted or not. This loop may be costly for that case, not sure. Will check.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index c67cdcdc3ba8..c877db23a72d 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,75 @@ 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; + bool (*npr)(struct scsi_cmnd *scmd); + + if (!scmd->device) + continue; + uld = scsi_cmd_to_driver(scmd); + if (!uld) + continue; + npr = 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); +} + +void scsi_call_prepare_resubmit(struct list_head *done_q) +{ + struct scsi_cmnd *scmd, *next; + + if (!scsi_needs_preparation(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 = + scmd->device ? scsi_cmd_to_driver(scmd) : NULL; + 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 +2264,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 3f0dfb97db6b..64070e530c4d 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 void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd); diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h index 4ce1988b2ba0..00ffa470724a 100644 --- a/include/scsi/scsi_driver.h +++ b/include/scsi/scsi_driver.h @@ -18,6 +18,8 @@ struct scsi_driver { int (*done)(struct scsi_cmnd *); int (*eh_action)(struct scsi_cmnd *, int); void (*eh_reset)(struct scsi_cmnd *); + bool (*eh_needs_prepare_resubmit)(struct scsi_cmnd *cmd); + void (*eh_prepare_resubmit)(struct list_head *cmd_list); }; #define to_scsi_driver(drv) \ container_of((drv), struct scsi_driver, gendrv)
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 | 72 ++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_priv.h | 1 + include/scsi/scsi_driver.h | 2 ++ 3 files changed, 75 insertions(+)