* [PATCH] scsi: target: tcmu: check cmd aborted earlier
@ 2022-07-08 3:32 Guixin Liu
2022-07-11 10:33 ` Bodo Stroesser
0 siblings, 1 reply; 2+ messages in thread
From: Guixin Liu @ 2022-07-08 3:32 UTC (permalink / raw)
To: bostroesser, martin.petersen; +Cc: linux-scsi, target-devel
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
--
1.8.3.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] scsi: target: tcmu: check cmd aborted earlier
2022-07-08 3:32 [PATCH] scsi: target: tcmu: check cmd aborted earlier Guixin Liu
@ 2022-07-11 10:33 ` Bodo Stroesser
0 siblings, 0 replies; 2+ messages in thread
From: Bodo Stroesser @ 2022-07-11 10:33 UTC (permalink / raw)
To: Guixin Liu, martin.petersen; +Cc: linux-scsi, target-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-07-11 11:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-08 3:32 [PATCH] scsi: target: tcmu: check cmd aborted earlier Guixin Liu
2022-07-11 10:33 ` Bodo Stroesser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.