Thank you Bart for such a fast response!
This LGTM.
Pavel
> On Feb 10, 2020, at 12:12 AM, Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> Commit 83f85b8ec305 postponed the percpu_ref_put(&se_cmd->se_lun->lun_ref)
> call from command completion to the time when the final command reference
> is dropped. That approach is not compatible with the iSCSI target driver
> because the iSCSI target driver keeps the command with the highest stat_sn
> after it has completed until the next command is received (see also
> iscsit_ack_from_expstatsn()). Fix this regression by reverting commit
> 83f85b8ec305.
>
> Reported-by: Pavel Zakharov <pavel.zakharov@xxxxxxxxxxx>
> Cc: Pavel Zakharov <pavel.zakharov@xxxxxxxxxxx>
> Cc: Mike Christie <mchristi@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: 83f85b8ec305 ("scsi: target/core: Inline transport_lun_remove_cmd()")
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index ea482d4b1f00..0ae9e60fc4d5 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -666,6 +666,11 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
>
> target_remove_from_state_list(cmd);
>
> + /*
> + * Clear struct se_cmd->se_lun before the handoff to FE.
> + */
> + cmd->se_lun = NULL;
> +
> spin_lock_irqsave(&cmd->t_state_lock, flags);
> /*
> * Determine if frontend context caller is requesting the stopping of
> @@ -693,6 +698,17 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
> return cmd->se_tfo->check_stop_free(cmd);
> }
>
> +static void transport_lun_remove_cmd(struct se_cmd *cmd)
> +{
> + struct se_lun *lun = cmd->se_lun;
> +
> + if (!lun)
> + return;
> +
> + if (cmpxchg(&cmd->lun_ref_active, true, false))
> + percpu_ref_put(&lun->lun_ref);
> +}
> +
> static void target_complete_failure_work(struct work_struct *work)
> {
> struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> @@ -783,6 +799,8 @@ static void target_handle_abort(struct se_cmd *cmd)
>
> WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
>
> + transport_lun_remove_cmd(cmd);
> +
> transport_cmd_check_stop_to_fabric(cmd);
> }
>
> @@ -1708,6 +1726,7 @@ static void target_complete_tmr_failure(struct work_struct *work)
> se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
> se_cmd->se_tfo->queue_tm_rsp(se_cmd);
>
> + transport_lun_remove_cmd(se_cmd);
> transport_cmd_check_stop_to_fabric(se_cmd);
> }
>
> @@ -1898,6 +1917,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
> goto queue_full;
>
> check_stop:
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
>
> @@ -2195,6 +2215,7 @@ static void transport_complete_qf(struct se_cmd *cmd)
> transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
> return;
> }
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> }
>
> @@ -2289,6 +2310,7 @@ static void target_complete_ok_work(struct work_struct *work)
> if (ret)
> goto queue_full;
>
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
> }
> @@ -2314,6 +2336,7 @@ static void target_complete_ok_work(struct work_struct *work)
> if (ret)
> goto queue_full;
>
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
> }
> @@ -2349,6 +2372,7 @@ static void target_complete_ok_work(struct work_struct *work)
> if (ret)
> goto queue_full;
>
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
> }
> @@ -2384,6 +2408,7 @@ static void target_complete_ok_work(struct work_struct *work)
> break;
> }
>
> + transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
>
> @@ -2710,6 +2735,9 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> */
> if (cmd->state_active)
> target_remove_from_state_list(cmd);
> +
> + if (cmd->se_lun)
> + transport_lun_remove_cmd(cmd);
> }
> if (aborted)
> cmd->free_compl = &compl;
> @@ -2781,9 +2809,6 @@ static void target_release_cmd_kref(struct kref *kref)
> struct completion *abrt_compl = se_cmd->abrt_compl;
> unsigned long flags;
>
> - if (se_cmd->lun_ref_active)
> - percpu_ref_put(&se_cmd->se_lun->lun_ref);
> -
> if (se_sess) {
> spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> list_del_init(&se_cmd->se_cmd_list);
