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@xxxxxxxxxxxxxxxxx>
---
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
