On 7/13/22 3:42 PM, Dmitry Bogdanov wrote:
> Sometimes an initiator does not send data for WRITE commands and tries
> to abort it. The abort hangs waiting for frontend driver completion.
> iSCSI driver waits for for data and that timeout eventually initiates
> connection reinstatment. The connection closing releases the commands in
> the connection, but those aborted commands still did not handle the
> abort and did not decrease a command ref counter. Because of that the
> connection reinstatement hangs indefinitely and prevents re-login for
> that initiator.
>
> This patch adds a handling in TCM of the abort for the WRITE_PENDING
> commands at connection closing moment to make it possible to release
> them.
>
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@xxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index e368f038ff5c..27eca5e72f52 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -26,6 +26,7 @@
> #include <target/target_core_base.h>
> #include <target/target_core_fabric.h>
>
> +#include <target/target_core_backend.h>
> #include <target/iscsi/iscsi_target_core.h>
> #include "iscsi_target_parameters.h"
> #include "iscsi_target_seq_pdu_list.h"
> @@ -4171,7 +4172,8 @@ static void iscsit_release_commands_from_conn(struct iscsit_conn *conn)
>
> if (se_cmd->se_tfo != NULL) {
> spin_lock_irq(&se_cmd->t_state_lock);
> - if (se_cmd->transport_state & CMD_T_ABORTED) {
> + if (se_cmd->t_state != TRANSPORT_WRITE_PENDING &&
> + se_cmd->transport_state & CMD_T_ABORTED) {
> /*
> * LIO's abort path owns the cleanup for this,
> * so put it back on the list and let
> @@ -4191,8 +4193,13 @@ static void iscsit_release_commands_from_conn(struct iscsit_conn *conn)
> list_del_init(&cmd->i_conn_node);
>
> iscsit_increment_maxcmdsn(cmd, sess);
> - iscsit_free_cmd(cmd, true);
> -
> + if (cmd->se_cmd.t_state == TRANSPORT_WRITE_PENDING &&
> + cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> + /* handle an abort in TCM */
> + target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED);
>
Will we have an extra ref left on the se_cmd if TAS is used so the se_cmd
does not get freed?
For TAS, it looks like we would do:
- target_handle_abort -> queue_status. This would not do anything because
before calling iscsit_release_commands_from_conn we have killed the iscsi tx
thread.
- target_handle_abort -> transport_cmd_check_stop_to_fabric -> check_stop_free ->
target_put_sess_cmd.
iscsi creates the se_cmd with TARGET_SCF_ACK_KREF set so do we have one ref
left?
For the non TAS case we do two puts:
target_handle_abort -> SCF_ACK_KREF is set so we do a target_put_sess_cmd here.
target_handle_abort -> transport_cmd_check_stop_to_fabric -> check_stop_free ->
target_put_sess_cmd which does a second put.
