[PATCH 14/17] target/qla2xxx: Simplify the code for handling aborted commands

[PATCH 14/17] target/qla2xxx: Simplify the code for handling aborted commands

[Bart Van Assche]

Since the target core waits anyway until a target driver has
finished processing a command, remove similar waiting code from
the tcm_qla2xxx driver. With this change applied command abortion
works as follows:
* tcm_qla2xxx receives an ABTS and calls target_submit_tmr().
* The target core calls core_tmr_abort_task(). That function
  sets the CMD_T_ABORTED flag and next calls transport_wait_for_tasks().
* If CMD_T_ACTIVE is still set, __transport_wait_for_tasks() sets
  CMD_T_STOP and waits for t_transport_stop_comp.
* When tcm_qla2xxx_handle_data_work() gets called, it either invokes
  transport_generic_request_failure() or target_execute_cmd().
* Both functions start with calling __transport_check_aborted_status()
  and return 1 if CMD_T_ABORTED was set. Otherwise the command that
  is being executed is completed and target_complete_cmd() completes
  t_transport_stop_comp.
* Once transport_wait_for_tasks() returns the target core considers
  the TMF as finished.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Quinn Tran <quinn.tran@cavium.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index e03d12a5f986..2cedfd4f15a3 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -414,21 +414,14 @@ static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
 static int tcm_qla2xxx_write_pending_status(struct se_cmd *se_cmd)
 {
 	unsigned long flags;
-	/*
-	 * Check for WRITE_PENDING status to determine if we need to wait for
-	 * CTIO aborts to be posted via hardware in tcm_qla2xxx_handle_data().
-	 */
+	bool wp;
+
 	spin_lock_irqsave(&se_cmd->t_state_lock, flags);
-	if (se_cmd->t_state = TRANSPORT_WRITE_PENDING ||
-	    se_cmd->t_state = TRANSPORT_COMPLETE_QF_WP) {
-		spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
-		wait_for_completion_timeout(&se_cmd->t_transport_stop_comp,
-						50);
-		return 0;
-	}
+	wp = se_cmd->t_state = TRANSPORT_WRITE_PENDING ||
+		se_cmd->t_state = TRANSPORT_COMPLETE_QF_WP;
 	spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
 
-	return 0;
+	return wp;
 }
 
 static void tcm_qla2xxx_set_default_node_attrs(struct se_node_acl *nacl)
@@ -508,15 +501,6 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
 
 	cmd->qpair->tgt_counters.qla_core_ret_ctio++;
 	if (!cmd->write_data_transferred) {
-		/*
-		 * Check if se_cmd has already been aborted via LUN_RESET, and
-		 * waiting upon completion in tcm_qla2xxx_write_pending_status()
-		 */
-		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
-			complete(&cmd->se_cmd.t_transport_stop_comp);
-			return;
-		}
-
 		switch (cmd->dif_err_code) {
 		case DIF_ERR_GRD:
 			cmd->se_cmd.pi_err -- 
2.18.0

Re: [PATCH 14/17] target/qla2xxx: Simplify the code for handling aborted commands

[Bart Van Assche]

On Mon, 2018-09-17 at 14:35 -0700, Bart Van Assche wrote:
+AD4 Since the target core waits anyway until a target driver has
+AD4 finished processing a command, remove similar waiting code from
+AD4 the tcm+AF8-qla2xxx driver. With this change applied command abortion
+AD4 works as follows:
+AD4 +ACo tcm+AF8-qla2xxx receives an ABTS and calls target+AF8-submit+AF8-tmr().
+AD4 +ACo The target core calls core+AF8-tmr+AF8-abort+AF8-task(). That function
+AD4   sets the CMD+AF8-T+AF8-ABORTED flag and next calls transport+AF8-wait+AF8-for+AF8-tasks().
+AD4 +ACo If CMD+AF8-T+AF8-ACTIVE is still set, +AF8AXw-transport+AF8-wait+AF8-for+AF8-tasks() sets
+AD4   CMD+AF8-T+AF8-STOP and waits for t+AF8-transport+AF8-stop+AF8-comp.
+AD4 +ACo When tcm+AF8-qla2xxx+AF8-handle+AF8-data+AF8-work() gets called, it either invokes
+AD4   transport+AF8-generic+AF8-request+AF8-failure() or target+AF8-execute+AF8-cmd().
+AD4 +ACo Both functions start with calling +AF8AXw-transport+AF8-check+AF8-aborted+AF8-status()
+AD4   and return 1 if CMD+AF8-T+AF8-ABORTED was set. Otherwise the command that
+AD4   is being executed is completed and target+AF8-complete+AF8-cmd() completes
+AD4   t+AF8-transport+AF8-stop+AF8-comp.
+AD4 +ACo Once transport+AF8-wait+AF8-for+AF8-tasks() returns the target core considers
+AD4   the TMF as finished.
+AD4 
+AD4 Signed-off-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
+AD4 Cc: Himanshu Madhani +ADw-himanshu.madhani+AEA-cavium.com+AD4
+AD4 Cc: Quinn Tran +ADw-quinn.tran+AEA-cavium.com+AD4
+AD4 Cc: Nicholas Bellinger +ADw-nab+AEA-linux-iscsi.org+AD4
+AD4 Cc: Mike Christie +ADw-mchristi+AEA-redhat.com+AD4
+AD4 Cc: Christoph Hellwig +ADw-hch+AEA-lst.de+AD4
+AD4 Cc: Hannes Reinecke +ADw-hare+AEA-suse.de+AD4

Hello Himanshu and Quinn,

Can one of you review this patch?

Thanks,

Bart.

Re: [PATCH 14/17] target/qla2xxx: Simplify the code for handling aborted commands

[Madhani, Himanshu]

> On Oct 3, 2018, at 4:20 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> External Email
> 
> On Mon, 2018-09-17 at 14:35 -0700, Bart Van Assche wrote:
>> Since the target core waits anyway until a target driver has
>> finished processing a command, remove similar waiting code from
>> the tcm_qla2xxx driver. With this change applied command abortion
>> works as follows:
>> * tcm_qla2xxx receives an ABTS and calls target_submit_tmr().
>> * The target core calls core_tmr_abort_task(). That function
>>  sets the CMD_T_ABORTED flag and next calls transport_wait_for_tasks().
>> * If CMD_T_ACTIVE is still set, __transport_wait_for_tasks() sets
>>  CMD_T_STOP and waits for t_transport_stop_comp.
>> * When tcm_qla2xxx_handle_data_work() gets called, it either invokes
>>  transport_generic_request_failure() or target_execute_cmd().
>> * Both functions start with calling __transport_check_aborted_status()
>>  and return 1 if CMD_T_ABORTED was set. Otherwise the command that
>>  is being executed is completed and target_complete_cmd() completes
>>  t_transport_stop_comp.
>> * Once transport_wait_for_tasks() returns the target core considers
>>  the TMF as finished.
>> 
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
>> Cc: Quinn Tran <quinn.tran@cavium.com>
>> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
>> Cc: Mike Christie <mchristi@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Hannes Reinecke <hare@suse.de>
> 
> Hello Himanshu and Quinn,
> 
> Can one of you review this patch?
> 
> Thanks,
> 
> Bart.
> 

Apologies for delay. Still reviewing the patch

Thanks,
- Himanshu

Re: [PATCH 14/17] target/qla2xxx: Simplify the code for handling aborted commands

[Madhani, Himanshu]

Hi Bart, 

Apologies for long delay

> On Sep 17, 2018, at 2:35 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> External Email
> 
> Since the target core waits anyway until a target driver has
> finished processing a command, remove similar waiting code from
> the tcm_qla2xxx driver. With this change applied command abortion
> works as follows:
> * tcm_qla2xxx receives an ABTS and calls target_submit_tmr().
> * The target core calls core_tmr_abort_task(). That function
>  sets the CMD_T_ABORTED flag and next calls transport_wait_for_tasks().
> * If CMD_T_ACTIVE is still set, __transport_wait_for_tasks() sets
>  CMD_T_STOP and waits for t_transport_stop_comp.
> * When tcm_qla2xxx_handle_data_work() gets called, it either invokes
>  transport_generic_request_failure() or target_execute_cmd().
> * Both functions start with calling __transport_check_aborted_status()
>  and return 1 if CMD_T_ABORTED was set. Otherwise the command that
>  is being executed is completed and target_complete_cmd() completes
>  t_transport_stop_comp.
> * Once transport_wait_for_tasks() returns the target core considers
>  the TMF as finished.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
> Cc: Quinn Tran <quinn.tran@cavium.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> ---
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 26 +++++---------------------
> 1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index e03d12a5f986..2cedfd4f15a3 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -414,21 +414,14 @@ static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
> static int tcm_qla2xxx_write_pending_status(struct se_cmd *se_cmd)
> {
>        unsigned long flags;
> -       /*
> -        * Check for WRITE_PENDING status to determine if we need to wait for
> -        * CTIO aborts to be posted via hardware in tcm_qla2xxx_handle_data().
> -        */
> +       bool wp;
> +
>        spin_lock_irqsave(&se_cmd->t_state_lock, flags);
> -       if (se_cmd->t_state == TRANSPORT_WRITE_PENDING ||
> -           se_cmd->t_state == TRANSPORT_COMPLETE_QF_WP) {
> -               spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
> -               wait_for_completion_timeout(&se_cmd->t_transport_stop_comp,
> -                                               50);
> -               return 0;
> -       }
> +       wp = se_cmd->t_state == TRANSPORT_WRITE_PENDING ||
> +               se_cmd->t_state == TRANSPORT_COMPLETE_QF_WP;
>        spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
> 
> -       return 0;
> +       return wp;
> }
> 
> static void tcm_qla2xxx_set_default_node_attrs(struct se_node_acl *nacl)
> @@ -508,15 +501,6 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
> 
>        cmd->qpair->tgt_counters.qla_core_ret_ctio++;
>        if (!cmd->write_data_transferred) {
> -               /*
> -                * Check if se_cmd has already been aborted via LUN_RESET, and
> -                * waiting upon completion in tcm_qla2xxx_write_pending_status()
> -                */
> -               if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> -                       complete(&cmd->se_cmd.t_transport_stop_comp);
> -                       return;
> -               }
> -
>                switch (cmd->dif_err_code) {
>                case DIF_ERR_GRD:
>                        cmd->se_cmd.pi_err =
> --
> 2.18.0
> 

Patch Looks good. 

Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>

Thanks,
- Himanshu