On 12/27/18 12:33 AM, James Smart wrote:
A scsi host lock is taken on every io completion to check whether
someone is waiting on the io completion. The lock doesn't have to be
taken on all ios, only those that have been marked as aborted.
Rework to avoid the lock on non-aborted ios.
Signed-off-by: Dick Kennedy <dick.kennedy@xxxxxxxxxxxx>
Signed-off-by: James Smart <jsmart2021@xxxxxxxxx>
---
drivers/scsi/lpfc/lpfc_scsi.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index efc24393d927..f1545ee3da7c 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -739,7 +739,6 @@ lpfc_get_scsi_buf_s4(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
lpfc_cmd->cur_iocbq.iocb_flag = LPFC_IO_FCP;
lpfc_cmd->prot_seg_cnt = 0;
lpfc_cmd->seg_cnt = 0;
- lpfc_cmd->waitq = NULL;
lpfc_cmd->timeout = 0;
lpfc_cmd->flags = 0;
lpfc_cmd->start_time = jiffies;
@@ -3939,13 +3938,15 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
cmd->scsi_done(cmd);
/*
- * If there is a thread waiting for command completion
+ * If there is an abort thread waiting for command completion
* wake up the thread.
*/
- spin_lock_irqsave(shost->host_lock, flags);
- if (lpfc_cmd->waitq)
- wake_up(lpfc_cmd->waitq);
- spin_unlock_irqrestore(shost->host_lock, flags);
+ if (unlikely(lpfc_cmd->cur_iocbq.iocb_flag & LPFC_DRIVER_ABORTED)) {
+ spin_lock_irqsave(shost->host_lock, flags);
+ if (lpfc_cmd->waitq)
+ wake_up(lpfc_cmd->waitq);
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ }
lpfc_release_scsi_buf(phba, lpfc_cmd);
}
You sure about this?
For this to work properly it relies on the LPFC_DRIVER_ABORTED flag to
be set, _and_ the new value propagated to all CPUs.
Yet no such mechanism are present here.
What _might_ work is to invert the logic here; namely have a
'LPFC_DRIVER_INFLIGHT' bit in the iocb, which is getting set prior to
submit the command, and cleared in the _normal_ iocb completion routine.
So when a timeout is hit we can check this flag, and only take the lock
if the flag is still set.
So if the flag is still set we might have hit the race condition, but
then we need to check the lock anyway which will resolve the ambiguity.
And we don't impact the fast path with this.
Cheers,
Hannes
