| Message ID | 1657251175-116098-1-git-send-email-kanie@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: target: tcmu: check cmd aborted earlier | expand |
Hi Liu, to be honest I don't like this change. Why do we check CMD_T_ABORTED here? It's just because a concurrently incoming TMR might already have aborted the cmd, and we don't want to queue up aborted cmds in tcmu. But that means, we better check CMD_T_ABORTED after taking cmdr_lock, because we might sleep a while waiting for the lock, which opens a possibly long time frame for new TMRs. So, I'd prefer to have the overhead of alloc/free over not catching aborted cmds, which causes even more overhead. Bodo On 08.07.22 05:32, Guixin Liu wrote: > Lift the check of cmd aborted to the head of tcmu_queue_cmd(), which > avoids pointless tcmu_cmd allocation if the cmd is already aborted. > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> > --- > drivers/target/target_core_user.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index 3deaeec..c1c1d2f 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -1204,16 +1204,18 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err) > struct se_device *se_dev = se_cmd->se_dev; > struct tcmu_dev *udev = TCMU_DEV(se_dev); > struct tcmu_cmd *tcmu_cmd; > - sense_reason_t scsi_ret = TCM_CHECK_CONDITION_ABORT_CMD; > - int ret = -1; > + sense_reason_t scsi_ret; > + int ret; > + > + if (se_cmd->transport_state & CMD_T_ABORTED) > + return TCM_CHECK_CONDITION_ABORT_CMD; > > tcmu_cmd = tcmu_alloc_cmd(se_cmd); > if (!tcmu_cmd) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > > mutex_lock(&udev->cmdr_lock); > - if (!(se_cmd->transport_state & CMD_T_ABORTED)) > - ret = queue_cmd_ring(tcmu_cmd, &scsi_ret); > + ret = queue_cmd_ring(tcmu_cmd, &scsi_ret); > if (ret < 0) > tcmu_free_cmd(tcmu_cmd); > else
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 3deaeec..c1c1d2f 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1204,16 +1204,18 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err) struct se_device *se_dev = se_cmd->se_dev; struct tcmu_dev *udev = TCMU_DEV(se_dev); struct tcmu_cmd *tcmu_cmd; - sense_reason_t scsi_ret = TCM_CHECK_CONDITION_ABORT_CMD; - int ret = -1; + sense_reason_t scsi_ret; + int ret; + + if (se_cmd->transport_state & CMD_T_ABORTED) + return TCM_CHECK_CONDITION_ABORT_CMD; tcmu_cmd = tcmu_alloc_cmd(se_cmd); if (!tcmu_cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; mutex_lock(&udev->cmdr_lock); - if (!(se_cmd->transport_state & CMD_T_ABORTED)) - ret = queue_cmd_ring(tcmu_cmd, &scsi_ret); + ret = queue_cmd_ring(tcmu_cmd, &scsi_ret); if (ret < 0) tcmu_free_cmd(tcmu_cmd); else
Lift the check of cmd aborted to the head of tcmu_queue_cmd(), which avoids pointless tcmu_cmd allocation if the cmd is already aborted. Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> --- drivers/target/target_core_user.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)