[v4] ibmvscsis: Do not send aborted task response
diff mbox

Message ID 1494281262-56000-1-git-send-email-bryantly@linux.vnet.ibm.com
State New, archived
Headers show

Commit Message

Bryant G. Ly May 8, 2017, 10:07 p.m. UTC
The driver is sending a response to the actual scsi op that was
aborted by an abort task TM, while LIO is sending a response to
the abort task TM.

ibmvscsis_tgt does not send the response to the client until
release_cmd time. The reason for this was because if we did it
at queue_status time, then the client would be free to reuse the
tag for that command, but we're still using the tag until the
command is released at release_cmd time, so we chose to delay
sending the response until then. That then caused this issue, because
release_cmd is always called, even if queue_status is not.

SCSI spec says that the initiator that sends the abort task
TM NEVER gets a response to the aborted op and with the current
code it will send a response. Thus this fix will remove that response
if the CMD_T_ABORTED && !CMD_T_TAS.

Another case with a small timing window is the case where if LIO sends a
TMR_DOES_NOT_EXIST, and the release_cmd callback is called for the TMR Abort
cmd before the release_cmd for the (attemped) aborted cmd, then we need to
ensure that we send the response for the (attempted) abort cmd to the client
before we send the response for the TMR Abort cmd.

Forgot to reset the cmd->abort_cmd pointer after finding it in send_messages.

Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Signed-off-by: Michael Cyr <mikecyr@linux.vnet.ibm.com>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 117 ++++++++++++++++++++++++-------
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |   2 +
 2 files changed, 94 insertions(+), 25 deletions(-)

Comments

Nicholas A. Bellinger May 9, 2017, 4:55 a.m. UTC | #1
Hi Bryant,

Given running we're almost out of time for -rc1, I'd like to avoid
having to rebase the handful of patches that are atop the -v3 that was
applied to target-pending/for-next over the weekend...

So if you'd be so kind, please post an incremental patch atop -v3, and
I'll apply that instead.

On Mon, 2017-05-08 at 17:07 -0500, Bryant G. Ly wrote:
> The driver is sending a response to the actual scsi op that was
> aborted by an abort task TM, while LIO is sending a response to
> the abort task TM.
> 
> ibmvscsis_tgt does not send the response to the client until
> release_cmd time. The reason for this was because if we did it
> at queue_status time, then the client would be free to reuse the
> tag for that command, but we're still using the tag until the
> command is released at release_cmd time, so we chose to delay
> sending the response until then. That then caused this issue, because
> release_cmd is always called, even if queue_status is not.
> 
> SCSI spec says that the initiator that sends the abort task
> TM NEVER gets a response to the aborted op and with the current
> code it will send a response. Thus this fix will remove that response
> if the CMD_T_ABORTED && !CMD_T_TAS.
> 
> Another case with a small timing window is the case where if LIO sends a
> TMR_DOES_NOT_EXIST, and the release_cmd callback is called for the TMR Abort
> cmd before the release_cmd for the (attemped) aborted cmd, then we need to
> ensure that we send the response for the (attempted) abort cmd to the client
> before we send the response for the TMR Abort cmd.
> 
> Forgot to reset the cmd->abort_cmd pointer after finding it in send_messages.
> 
> Cc: <stable@vger.kernel.org> # v4.8+
> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Signed-off-by: Michael Cyr <mikecyr@linux.vnet.ibm.com>
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 117 ++++++++++++++++++++++++-------
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |   2 +
>  2 files changed, 94 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 0f80779..ee64241 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1170,6 +1170,9 @@ static struct ibmvscsis_cmd *ibmvscsis_get_free_cmd(struct scsi_info *vscsi)
>  		cmd = list_first_entry_or_null(&vscsi->free_cmd,
>  					       struct ibmvscsis_cmd, list);
>  		if (cmd) {
> +			if (cmd->abort_cmd)
> +				cmd->abort_cmd = NULL;
> +			cmd->flags &= ~(DELAY_SEND);
>  			list_del(&cmd->list);
>  			cmd->iue = iue;
>  			cmd->type = UNSET_TYPE;
> @@ -1749,45 +1752,80 @@ static void srp_snd_msg_failed(struct scsi_info *vscsi, long rc)
>  static void ibmvscsis_send_messages(struct scsi_info *vscsi)
>  {
>  	u64 msg_hi = 0;
> -	/* note do not attmempt to access the IU_data_ptr with this pointer
> +	/* note do not attempt to access the IU_data_ptr with this pointer
>  	 * it is not valid
>  	 */
>  	struct viosrp_crq *crq = (struct viosrp_crq *)&msg_hi;
>  	struct ibmvscsis_cmd *cmd, *nxt;
>  	struct iu_entry *iue;
>  	long rc = ADAPT_SUCCESS;
> +	bool retry = false;
>  
>  	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
> -		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
> -			iue = cmd->iue;
> +		do {
> +			retry = false;
> +			list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp,
> +						 list) {
> +				/*
> +				 * Check to make sure abort cmd gets processed
> +				 * prior to the abort tmr cmd
> +				 */
> +				if (cmd->flags & DELAY_SEND)
> +					continue;
>  
> -			crq->valid = VALID_CMD_RESP_EL;
> -			crq->format = cmd->rsp.format;
> +				if (cmd->abort_cmd) {
> +					retry = true;
> +					cmd->abort_cmd->flags &= ~(DELAY_SEND);
> +					cmd->abort_cmd = NULL;
> +				}
>  
> -			if (cmd->flags & CMD_FAST_FAIL)
> -				crq->status = VIOSRP_ADAPTER_FAIL;
> +				/*
> +				 * If CMD_T_ABORTED w/o CMD_T_TAS scenarios and
> +				 * the case where LIO issued a
> +				 * ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST
> +				 * case then we dont send a response, since it
> +				 * was already done.
> +				 */
> +				if (cmd->se_cmd.transport_state & CMD_T_ABORTED &&
> +				    !(cmd->se_cmd.transport_state & CMD_T_TAS)) {
> +					list_del(&cmd->list);
> +					ibmvscsis_free_cmd_resources(vscsi,
> +								     cmd);
> +				} else {
> +					iue = cmd->iue;
>  
> -			crq->IU_length = cpu_to_be16(cmd->rsp.len);
> +					crq->valid = VALID_CMD_RESP_EL;
> +					crq->format = cmd->rsp.format;
>  
> -			rc = h_send_crq(vscsi->dma_dev->unit_address,
> -					be64_to_cpu(msg_hi),
> -					be64_to_cpu(cmd->rsp.tag));
> +					if (cmd->flags & CMD_FAST_FAIL)
> +						crq->status = VIOSRP_ADAPTER_FAIL;
>  
> -			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
> -				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
> +					crq->IU_length = cpu_to_be16(cmd->rsp.len);
>  
> -			/* if all ok free up the command element resources */
> -			if (rc == H_SUCCESS) {
> -				/* some movement has occurred */
> -				vscsi->rsp_q_timer.timer_pops = 0;
> -				list_del(&cmd->list);
> +					rc = h_send_crq(vscsi->dma_dev->unit_address,
> +							be64_to_cpu(msg_hi),
> +							be64_to_cpu(cmd->rsp.tag));
>  
> -				ibmvscsis_free_cmd_resources(vscsi, cmd);
> -			} else {
> -				srp_snd_msg_failed(vscsi, rc);
> -				break;
> +					pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
> +						 cmd, be64_to_cpu(cmd->rsp.tag), rc);
> +
> +					/* if all ok free up the command
> +					 * element resources
> +					 */
> +					if (rc == H_SUCCESS) {
> +						/* some movement has occurred */
> +						vscsi->rsp_q_timer.timer_pops = 0;
> +						list_del(&cmd->list);
> +
> +						ibmvscsis_free_cmd_resources(vscsi,
> +									     cmd);
> +					} else {
> +						srp_snd_msg_failed(vscsi, rc);
> +						break;
> +					}
> +				}
>  			}
> -		}
> +		} while (retry);
>  
>  		if (!rc) {
>  			/*
> @@ -2708,6 +2746,7 @@ static int ibmvscsis_alloc_cmds(struct scsi_info *vscsi, int num)
>  
>  	for (i = 0, cmd = (struct ibmvscsis_cmd *)vscsi->cmd_pool; i < num;
>  	     i++, cmd++) {
> +		cmd->abort_cmd = NULL;
>  		cmd->adapter = vscsi;
>  		INIT_WORK(&cmd->work, ibmvscsis_scheduler);
>  		list_add_tail(&cmd->list, &vscsi->free_cmd);
> @@ -3579,9 +3618,20 @@ static int ibmvscsis_write_pending(struct se_cmd *se_cmd)
>  {
>  	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
>  						 se_cmd);
> +	struct scsi_info *vscsi = cmd->adapter;
>  	struct iu_entry *iue = cmd->iue;
>  	int rc;
>  
> +	/*
> +	 * If CLIENT_FAILED OR RESPONSE_Q_DOWN, then just return success
> +	 * since LIO can't do anything about it, and we dont want to
> +	 * attempt an srp_transfer_data.
> +	 */
> +	if ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
> +		pr_err("write_pending failed since: %d\n", vscsi->flags);
> +		return 0;
> +	}
> +
>  	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma,
>  			       1, 1);
>  	if (rc) {
> @@ -3660,11 +3710,28 @@ static void ibmvscsis_queue_tm_rsp(struct se_cmd *se_cmd)
>  	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
>  						 se_cmd);
>  	struct scsi_info *vscsi = cmd->adapter;
> +	struct ibmvscsis_cmd *cmd_itr;
> +	struct iu_entry *iue = iue = cmd->iue;
> +	struct srp_tsk_mgmt *srp_tsk = &vio_iu(iue)->srp.tsk_mgmt;
> +	u64 tag_to_abort = be64_to_cpu(srp_tsk->task_tag);
>  	uint len;
>  
>  	pr_debug("queue_tm_rsp %p, status %d\n",
>  		 se_cmd, (int)se_cmd->se_tmr_req->response);
>  
> +	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK &&
> +	    cmd->se_cmd.se_tmr_req->response == TMR_TASK_DOES_NOT_EXIST) {
> +		spin_lock_bh(&vscsi->intr_lock);
> +		list_for_each_entry(cmd_itr, &vscsi->active_q, list) {
> +			if (tag_to_abort == cmd_itr->se_cmd.tag) {
> +				cmd_itr->abort_cmd = cmd;
> +				cmd->flags |= DELAY_SEND;
> +				break;
> +			}
> +		}
> +		spin_unlock_bh(&vscsi->intr_lock);
> +	}
> +
>  	srp_build_response(vscsi, cmd, &len);
>  	cmd->rsp.format = SRP_FORMAT;
>  	cmd->rsp.len = len;
> @@ -3672,8 +3739,8 @@ static void ibmvscsis_queue_tm_rsp(struct se_cmd *se_cmd)
>  
>  static void ibmvscsis_aborted_task(struct se_cmd *se_cmd)
>  {
> -	/* TBD: What (if anything) should we do here? */
> -	pr_debug("ibmvscsis_aborted_task %p\n", se_cmd);
> +	pr_debug("ibmvscsis_aborted_task %p task_tag: %llu\n",
> +		 se_cmd, se_cmd->tag);
>  }
>  
>  static struct se_wwn *ibmvscsis_make_tport(struct target_fabric_configfs *tf,
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> index 65c6189..b4391a8 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> @@ -168,10 +168,12 @@ struct ibmvscsis_cmd {
>  	struct iu_rsp rsp;
>  	struct work_struct work;
>  	struct scsi_info *adapter;
> +	struct ibmvscsis_cmd *abort_cmd;
>  	/* Sense buffer that will be mapped into outgoing status */
>  	unsigned char sense_buf[TRANSPORT_SENSE_BUFFER];
>  	u64 init_time;
>  #define CMD_FAST_FAIL	BIT(0)
> +#define DELAY_SEND	BIT(1)
>  	u32 flags;
>  	char type;
>  };


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 0f80779..ee64241 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1170,6 +1170,9 @@  static struct ibmvscsis_cmd *ibmvscsis_get_free_cmd(struct scsi_info *vscsi)
 		cmd = list_first_entry_or_null(&vscsi->free_cmd,
 					       struct ibmvscsis_cmd, list);
 		if (cmd) {
+			if (cmd->abort_cmd)
+				cmd->abort_cmd = NULL;
+			cmd->flags &= ~(DELAY_SEND);
 			list_del(&cmd->list);
 			cmd->iue = iue;
 			cmd->type = UNSET_TYPE;
@@ -1749,45 +1752,80 @@  static void srp_snd_msg_failed(struct scsi_info *vscsi, long rc)
 static void ibmvscsis_send_messages(struct scsi_info *vscsi)
 {
 	u64 msg_hi = 0;
-	/* note do not attmempt to access the IU_data_ptr with this pointer
+	/* note do not attempt to access the IU_data_ptr with this pointer
 	 * it is not valid
 	 */
 	struct viosrp_crq *crq = (struct viosrp_crq *)&msg_hi;
 	struct ibmvscsis_cmd *cmd, *nxt;
 	struct iu_entry *iue;
 	long rc = ADAPT_SUCCESS;
+	bool retry = false;
 
 	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
-		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
-			iue = cmd->iue;
+		do {
+			retry = false;
+			list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp,
+						 list) {
+				/*
+				 * Check to make sure abort cmd gets processed
+				 * prior to the abort tmr cmd
+				 */
+				if (cmd->flags & DELAY_SEND)
+					continue;
 
-			crq->valid = VALID_CMD_RESP_EL;
-			crq->format = cmd->rsp.format;
+				if (cmd->abort_cmd) {
+					retry = true;
+					cmd->abort_cmd->flags &= ~(DELAY_SEND);
+					cmd->abort_cmd = NULL;
+				}
 
-			if (cmd->flags & CMD_FAST_FAIL)
-				crq->status = VIOSRP_ADAPTER_FAIL;
+				/*
+				 * If CMD_T_ABORTED w/o CMD_T_TAS scenarios and
+				 * the case where LIO issued a
+				 * ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST
+				 * case then we dont send a response, since it
+				 * was already done.
+				 */
+				if (cmd->se_cmd.transport_state & CMD_T_ABORTED &&
+				    !(cmd->se_cmd.transport_state & CMD_T_TAS)) {
+					list_del(&cmd->list);
+					ibmvscsis_free_cmd_resources(vscsi,
+								     cmd);
+				} else {
+					iue = cmd->iue;
 
-			crq->IU_length = cpu_to_be16(cmd->rsp.len);
+					crq->valid = VALID_CMD_RESP_EL;
+					crq->format = cmd->rsp.format;
 
-			rc = h_send_crq(vscsi->dma_dev->unit_address,
-					be64_to_cpu(msg_hi),
-					be64_to_cpu(cmd->rsp.tag));
+					if (cmd->flags & CMD_FAST_FAIL)
+						crq->status = VIOSRP_ADAPTER_FAIL;
 
-			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
-				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+					crq->IU_length = cpu_to_be16(cmd->rsp.len);
 
-			/* if all ok free up the command element resources */
-			if (rc == H_SUCCESS) {
-				/* some movement has occurred */
-				vscsi->rsp_q_timer.timer_pops = 0;
-				list_del(&cmd->list);
+					rc = h_send_crq(vscsi->dma_dev->unit_address,
+							be64_to_cpu(msg_hi),
+							be64_to_cpu(cmd->rsp.tag));
 
-				ibmvscsis_free_cmd_resources(vscsi, cmd);
-			} else {
-				srp_snd_msg_failed(vscsi, rc);
-				break;
+					pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
+						 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+
+					/* if all ok free up the command
+					 * element resources
+					 */
+					if (rc == H_SUCCESS) {
+						/* some movement has occurred */
+						vscsi->rsp_q_timer.timer_pops = 0;
+						list_del(&cmd->list);
+
+						ibmvscsis_free_cmd_resources(vscsi,
+									     cmd);
+					} else {
+						srp_snd_msg_failed(vscsi, rc);
+						break;
+					}
+				}
 			}
-		}
+		} while (retry);
 
 		if (!rc) {
 			/*
@@ -2708,6 +2746,7 @@  static int ibmvscsis_alloc_cmds(struct scsi_info *vscsi, int num)
 
 	for (i = 0, cmd = (struct ibmvscsis_cmd *)vscsi->cmd_pool; i < num;
 	     i++, cmd++) {
+		cmd->abort_cmd = NULL;
 		cmd->adapter = vscsi;
 		INIT_WORK(&cmd->work, ibmvscsis_scheduler);
 		list_add_tail(&cmd->list, &vscsi->free_cmd);
@@ -3579,9 +3618,20 @@  static int ibmvscsis_write_pending(struct se_cmd *se_cmd)
 {
 	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
 						 se_cmd);
+	struct scsi_info *vscsi = cmd->adapter;
 	struct iu_entry *iue = cmd->iue;
 	int rc;
 
+	/*
+	 * If CLIENT_FAILED OR RESPONSE_Q_DOWN, then just return success
+	 * since LIO can't do anything about it, and we dont want to
+	 * attempt an srp_transfer_data.
+	 */
+	if ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
+		pr_err("write_pending failed since: %d\n", vscsi->flags);
+		return 0;
+	}
+
 	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma,
 			       1, 1);
 	if (rc) {
@@ -3660,11 +3710,28 @@  static void ibmvscsis_queue_tm_rsp(struct se_cmd *se_cmd)
 	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
 						 se_cmd);
 	struct scsi_info *vscsi = cmd->adapter;
+	struct ibmvscsis_cmd *cmd_itr;
+	struct iu_entry *iue = iue = cmd->iue;
+	struct srp_tsk_mgmt *srp_tsk = &vio_iu(iue)->srp.tsk_mgmt;
+	u64 tag_to_abort = be64_to_cpu(srp_tsk->task_tag);
 	uint len;
 
 	pr_debug("queue_tm_rsp %p, status %d\n",
 		 se_cmd, (int)se_cmd->se_tmr_req->response);
 
+	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK &&
+	    cmd->se_cmd.se_tmr_req->response == TMR_TASK_DOES_NOT_EXIST) {
+		spin_lock_bh(&vscsi->intr_lock);
+		list_for_each_entry(cmd_itr, &vscsi->active_q, list) {
+			if (tag_to_abort == cmd_itr->se_cmd.tag) {
+				cmd_itr->abort_cmd = cmd;
+				cmd->flags |= DELAY_SEND;
+				break;
+			}
+		}
+		spin_unlock_bh(&vscsi->intr_lock);
+	}
+
 	srp_build_response(vscsi, cmd, &len);
 	cmd->rsp.format = SRP_FORMAT;
 	cmd->rsp.len = len;
@@ -3672,8 +3739,8 @@  static void ibmvscsis_queue_tm_rsp(struct se_cmd *se_cmd)
 
 static void ibmvscsis_aborted_task(struct se_cmd *se_cmd)
 {
-	/* TBD: What (if anything) should we do here? */
-	pr_debug("ibmvscsis_aborted_task %p\n", se_cmd);
+	pr_debug("ibmvscsis_aborted_task %p task_tag: %llu\n",
+		 se_cmd, se_cmd->tag);
 }
 
 static struct se_wwn *ibmvscsis_make_tport(struct target_fabric_configfs *tf,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
index 65c6189..b4391a8 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
@@ -168,10 +168,12 @@  struct ibmvscsis_cmd {
 	struct iu_rsp rsp;
 	struct work_struct work;
 	struct scsi_info *adapter;
+	struct ibmvscsis_cmd *abort_cmd;
 	/* Sense buffer that will be mapped into outgoing status */
 	unsigned char sense_buf[TRANSPORT_SENSE_BUFFER];
 	u64 init_time;
 #define CMD_FAST_FAIL	BIT(0)
+#define DELAY_SEND	BIT(1)
 	u32 flags;
 	char type;
 };