[PATCH] ibmvscsis: Do not send aborted task response

[PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

The driver is sending a response to the aborted task response
along with LIO sending the tmr response. 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
op is aborted.

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

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 4bb5635..8e2733f 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1169,6 +1169,7 @@ 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) {
+			cmd->flags &= ~(CMD_ABORTED);
 			list_del(&cmd->list);
 			cmd->iue = iue;
 			cmd->type = UNSET_TYPE;
@@ -1758,33 +1759,41 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
 
 	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
 		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
-			iue = cmd->iue;
+			/*
+			 * If an Abort flag is set then dont send response
+			 */
+			if (cmd->flags & CMD_ABORTED) {
+				list_del(&cmd->list);
+				ibmvscsis_free_cmd_resources(vscsi, cmd);
+			} else {
+				iue = cmd->iue;
 
-			crq->valid = VALID_CMD_RESP_EL;
-			crq->format = cmd->rsp.format;
+				crq->valid = VALID_CMD_RESP_EL;
+				crq->format = cmd->rsp.format;
 
-			if (cmd->flags & CMD_FAST_FAIL)
-				crq->status = VIOSRP_ADAPTER_FAIL;
+				if (cmd->flags & CMD_FAST_FAIL)
+					crq->status = VIOSRP_ADAPTER_FAIL;
 
-			crq->IU_length = cpu_to_be16(cmd->rsp.len);
+				crq->IU_length = cpu_to_be16(cmd->rsp.len);
 
-			rc = h_send_crq(vscsi->dma_dev->unit_address,
-					be64_to_cpu(msg_hi),
-					be64_to_cpu(cmd->rsp.tag));
+				rc = h_send_crq(vscsi->dma_dev->unit_address,
+						be64_to_cpu(msg_hi),
+						be64_to_cpu(cmd->rsp.tag));
 
-			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
-				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+				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);
+				/* 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;
+					ibmvscsis_free_cmd_resources(vscsi, cmd);
+				} else {
+					srp_snd_msg_failed(vscsi, rc);
+					break;
+				}
 			}
 		}
 
@@ -3581,9 +3590,15 @@ 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 ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
+		pr_err("write_pending failed since: %d\n", vscsi->flags);
+		return -EIO;
+	}
+
 	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma,
 			       1, 1);
 	if (rc) {
@@ -3674,7 +3689,10 @@ 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? */
+	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
+						 se_cmd);
+
+	cmd->flags |= CMD_ABORTED;
 	pr_debug("ibmvscsis_aborted_task %p\n", se_cmd);
 }
 
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
index 98b0ca7..24db7a9 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
@@ -171,6 +171,7 @@ struct ibmvscsis_cmd {
 	unsigned char sense_buf[TRANSPORT_SENSE_BUFFER];
 	u64 init_time;
 #define CMD_FAST_FAIL	BIT(0)
+#define CMD_ABORTED	BIT(1)
 	u32 flags;
 	char type;
 };
-- 
2.5.4 (Apple Git-61)

[PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

The driver is sending a response to the aborted task response
along with LIO sending the tmr response. 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
op is aborted.

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

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 4bb5635..8e2733f 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1169,6 +1169,7 @@ 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) {
+			cmd->flags &= ~(CMD_ABORTED);
 			list_del(&cmd->list);
 			cmd->iue = iue;
 			cmd->type = UNSET_TYPE;
@@ -1758,33 +1759,41 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
 
 	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
 		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
-			iue = cmd->iue;
+			/*
+			 * If an Abort flag is set then dont send response
+			 */
+			if (cmd->flags & CMD_ABORTED) {
+				list_del(&cmd->list);
+				ibmvscsis_free_cmd_resources(vscsi, cmd);
+			} else {
+				iue = cmd->iue;
 
-			crq->valid = VALID_CMD_RESP_EL;
-			crq->format = cmd->rsp.format;
+				crq->valid = VALID_CMD_RESP_EL;
+				crq->format = cmd->rsp.format;
 
-			if (cmd->flags & CMD_FAST_FAIL)
-				crq->status = VIOSRP_ADAPTER_FAIL;
+				if (cmd->flags & CMD_FAST_FAIL)
+					crq->status = VIOSRP_ADAPTER_FAIL;
 
-			crq->IU_length = cpu_to_be16(cmd->rsp.len);
+				crq->IU_length = cpu_to_be16(cmd->rsp.len);
 
-			rc = h_send_crq(vscsi->dma_dev->unit_address,
-					be64_to_cpu(msg_hi),
-					be64_to_cpu(cmd->rsp.tag));
+				rc = h_send_crq(vscsi->dma_dev->unit_address,
+						be64_to_cpu(msg_hi),
+						be64_to_cpu(cmd->rsp.tag));
 
-			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
-				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+				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);
+				/* 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;
+					ibmvscsis_free_cmd_resources(vscsi, cmd);
+				} else {
+					srp_snd_msg_failed(vscsi, rc);
+					break;
+				}
 			}
 		}
 
@@ -3581,9 +3590,15 @@ 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 ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
+		pr_err("write_pending failed since: %d\n", vscsi->flags);
+		return -EIO;
+	}
+
 	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma,
 			       1, 1);
 	if (rc) {
@@ -3674,7 +3689,10 @@ 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? */
+	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
+						 se_cmd);
+
+	cmd->flags |= CMD_ABORTED;
 	pr_debug("ibmvscsis_aborted_task %p\n", se_cmd);
 }
 
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
index 98b0ca7..24db7a9 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
@@ -171,6 +171,7 @@ struct ibmvscsis_cmd {
 	unsigned char sense_buf[TRANSPORT_SENSE_BUFFER];
 	u64 init_time;
 #define CMD_FAST_FAIL	BIT(0)
+#define CMD_ABORTED	BIT(1)
 	u32 flags;
 	char type;
 };
-- 
2.5.4 (Apple Git-61)

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

On 4/7/17 10:49 AM, Bryant G. Ly wrote:
> The driver is sending a response to the aborted task response
> along with LIO sending the tmr response. 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
> op is aborted.
>
> Cc: <stable@vger.kernel.org> # v4.8+
> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> ---
>   drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 +++++++++++++++++++++-----------
>   drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |  1 +
>   2 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 4bb5635..8e2733f 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1169,6 +1169,7 @@ 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) {
> +			cmd->flags &= ~(CMD_ABORTED);
>   			list_del(&cmd->list);
>   			cmd->iue = iue;
>   			cmd->type = UNSET_TYPE;
> @@ -1758,33 +1759,41 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
>
>   	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
>   		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
> -			iue = cmd->iue;
> +			/*
> +			 * If an Abort flag is set then dont send response
> +			 */
> +			if (cmd->flags & CMD_ABORTED) {
> +				list_del(&cmd->list);
> +				ibmvscsis_free_cmd_resources(vscsi, cmd);
> +			} else {
> +				iue = cmd->iue;
>
> -			crq->valid = VALID_CMD_RESP_EL;
> -			crq->format = cmd->rsp.format;
> +				crq->valid = VALID_CMD_RESP_EL;
> +				crq->format = cmd->rsp.format;
>
> -			if (cmd->flags & CMD_FAST_FAIL)
> -				crq->status = VIOSRP_ADAPTER_FAIL;
> +				if (cmd->flags & CMD_FAST_FAIL)
> +					crq->status = VIOSRP_ADAPTER_FAIL;
>
> -			crq->IU_length = cpu_to_be16(cmd->rsp.len);
> +				crq->IU_length = cpu_to_be16(cmd->rsp.len);
>
> -			rc = h_send_crq(vscsi->dma_dev->unit_address,
> -					be64_to_cpu(msg_hi),
> -					be64_to_cpu(cmd->rsp.tag));
> +				rc = h_send_crq(vscsi->dma_dev->unit_address,
> +						be64_to_cpu(msg_hi),
> +						be64_to_cpu(cmd->rsp.tag));
>
> -			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
> -				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
> +				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);
> +				/* 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;
> +					ibmvscsis_free_cmd_resources(vscsi, cmd);
> +				} else {
> +					srp_snd_msg_failed(vscsi, rc);
> +					break;
> +				}
>   			}
>   		}
>
> @@ -3581,9 +3590,15 @@ 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 ((vscsi->flags & (CLIENT_FAILED | RESPONSE_Q_DOWN))) {
> +		pr_err("write_pending failed since: %d\n", vscsi->flags);
> +		return -EIO;
> +	}
> +
>   	rc = srp_transfer_data(cmd, &vio_iu(iue)->srp.cmd, ibmvscsis_rdma,
>   			       1, 1);
>   	if (rc) {

One question I had for you Nick was whether I should just return success if CLIENT_FAILED or RSP Q DOWN.
I know LIO wants EAGAIN or ENONEM on write pending return codes, but in this case LIO prob doesn't care.
It would produce less noise/warning if I were to just do success, what do you think?

-Bryant

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Bart Van Assche]

On 04/07/2017 08:49 AM, Bryant G. Ly wrote:
> The driver is sending a response to the aborted task response
                                                       ^^^^^^^^
That occurrence of "response" looks extraneous?

> along with LIO sending the tmr response. 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
> op is aborted.

Hello Bryan,

Are you sure that a new flag needed to prevent that a command response
is sent to the initiator that submitted the ABORT?
__target_check_io_state() only sets CMD_T_TAS if the TMF was received
from another I_T nexus than the I_T nexus through which the aborted
command was received. core_tmr_handle_tas_abort() only triggers a call
to .queue_status() if CMD_T_TAS is set. Do you agree with this analysis?

Thanks,

Bart.

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Bart Van Assche]

On Fri, 2017-04-07 at 11:12 -0500, Bryant G. Ly wrote:
> So from this stack trace it looks like the ibmvscsis driver is sending an
> extra response through send_messages even though an abort was issued.  
> IBMi, reported this issue internally when they were testing the driver,
> because their client didn't like getting a response back for the aborted op.
> They are only expecting a TMR from the abort request, NOT the aborted op. 

Hello Bryant,

What is the root cause of this behavior? Why is it that the behavior of
the ibmvscsi_tgt contradicts what has been implemented in the LIO core?
Sorry but the patch at the start of this thread looks to me like an
attempt to paper over the problem instead of addressing the root cause.

Thanks,

Bart.

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

On 4/7/17 11:36 AM, Bart Van Assche wrote:
> On Fri, 2017-04-07 at 11:12 -0500, Bryant G. Ly wrote:
>> So from this stack trace it looks like the ibmvscsis driver is sending an
>> extra response through send_messages even though an abort was issued.
>> IBMi, reported this issue internally when they were testing the driver,
>> because their client didn't like getting a response back for the aborted op.
>> They are only expecting a TMR from the abort request, NOT the aborted op.
> Hello Bryant,
>
> What is the root cause of this behavior? Why is it that the behavior of
> the ibmvscsi_tgt contradicts what has been implemented in the LIO core?
> Sorry but the patch at the start of this thread looks to me like an
> attempt to paper over the problem instead of addressing the root cause.
>
> Thanks,
>
IBMi clients received a response for an aborted operation, so they sent an abort tm request.
Afterwards they get a CRQ response to the op that they aborted. That should not happen, because they are only supposed to get a response for the tm request NOT the aborted operation.
Looking at the code it looks like it is due to send messages, processing a response without checking to see if it was an aborted op.
This patch addresses a bug within the ibmvscsis driver and the fact that it SENT a response to the aborted operation(which is wrong since) without looking at what LIO core had done.
The driver isn't supposed to send any response to the aborted operation, BUT only a response to the abort tm request, which LIO core currently does.

-Bryant

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Michael Cyr]

On 4/7/17 12:01 PM, Bryant G. Ly wrote:
>
>
> On 4/7/17 11:36 AM, Bart Van Assche wrote:
>> On Fri, 2017-04-07 at 11:12 -0500, Bryant G. Ly wrote:
>>> So from this stack trace it looks like the ibmvscsis driver is 
>>> sending an
>>> extra response through send_messages even though an abort was issued.
>>> IBMi, reported this issue internally when they were testing the driver,
>>> because their client didn't like getting a response back for the 
>>> aborted op.
>>> They are only expecting a TMR from the abort request, NOT the 
>>> aborted op.
>> Hello Bryant,
>>
>> What is the root cause of this behavior? Why is it that the behavior of
>> the ibmvscsi_tgt contradicts what has been implemented in the LIO core?
>> Sorry but the patch at the start of this thread looks to me like an
>> attempt to paper over the problem instead of addressing the root cause.
>>
>> Thanks,
>>
> IBMi clients received a response for an aborted operation, so they 
> sent an abort tm request.
> Afterwards they get a CRQ response to the op that they aborted. That 
> should not happen, because they are only supposed to get a response 
> for the tm request NOT the aborted operation.
> Looking at the code it looks like it is due to send messages, 
> processing a response without checking to see if it was an aborted op.
> This patch addresses a bug within the ibmvscsis driver and the fact 
> that it SENT a response to the aborted operation(which is wrong since) 
> without looking at what LIO core had done.
> The driver isn't supposed to send any response to the aborted 
> operation, BUT only a response to the abort tm request, which LIO core 
> currently does.
>
> -Bryant
>
I think I can clarify the issue here: 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.  Perhaps it would be cleaner to set some sort of 
status valid flag during queue_status instead of setting a flag in 
aborted_task?

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Bart Van Assche]

On Fri, 2017-04-07 at 16:14 -0500, Michael Cyr wrote:
> That then caused this issue, because release_cmd is always called, even 
> if queue_status is not.  Perhaps it would be cleaner to set some sort of 
> status valid flag during queue_status instead of setting a flag in 
> aborted_task?

Hello Michael,

Thanks for the clarification. Have you already checked whether a new flag
is really needed or whether checking CMD_T_TAS would be sufficient?

Thanks,

Bart.

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Nicholas A. Bellinger]

Hi Bryant & Co,

Apologies for the delayed follow up.

Comments below.

On Mon, 2017-04-10 at 13:52 -0500, Bryant G. Ly wrote:
> The driver is sending a response to the aborted task response
> along with LIO sending the tmr response.
> 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 TAS bit is set.
> 
> Cc: <stable@vger.kernel.org> # v4.8+
> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 +++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 4bb5635..f75948a 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1758,33 +1758,42 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
>  
>  	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
>  		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
> -			iue = cmd->iue;
> +			/*
> +			 * If Task Abort Status Bit is set, then dont send a
> +			 * response.
> +			 */
> +			if (cmd->se_cmd.transport_state & CMD_T_TAS) {
> +				list_del(&cmd->list);
> +				ibmvscsis_free_cmd_resources(vscsi, cmd);
> +			} else {
> +				iue = cmd->iue;

Unless I'm mistaken, this should be a check for CMD_T_ABORTED &&
!CMD_T_TAS to avoid generating an explicit response + immediately free,
and not a check for CMD_T_TAS when a command still needs a explicit
response..

That is, CMD_T_TAS is the only scenario where target-core is supposed to
generate an explicit response of SAM_STAT_TASK_ABORTED, which means
h_send_crq() should be called for those se_cmd descriptors.

However for CMD_T_ABORTED w/o CMD_T_TAS scenarios (like TMR TASK_ABORT
for example) where target-core does not call ->queue_status(), this is
the case where ibmvscsis_send_messages() should be ignoring calls to
h_send_crq(), because se_cmd descriptors must not be generating response
after they have been aborted.

> @@ -3581,9 +3590,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) {

AFAICT, returning '0' here is correct to avoid generating an explicit
CHECK_CONDITION for non -EAGAIN or -ENOMEM return values.

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Tyrel Datwyler]

On 04/10/2017 11:52 AM, Bryant G. Ly wrote:
> The driver is sending a response to the aborted task response
> along with LIO sending the tmr response.

I think this could be better worded. Something like 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.

The above portion of the commit message is little convoluted in my opinion, and a bit hard
to follow. Otherwise,

Reviewed-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

> 
> 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 TAS bit is set.
> 
> Cc: <stable@vger.kernel.org> # v4.8+
> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 +++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 4bb5635..f75948a 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1758,33 +1758,42 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
>  
>  	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
>  		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
> -			iue = cmd->iue;
> +			/*
> +			 * If Task Abort Status Bit is set, then dont send a
> +			 * response.
> +			 */
> +			if (cmd->se_cmd.transport_state & CMD_T_TAS) {
> +				list_del(&cmd->list);
> +				ibmvscsis_free_cmd_resources(vscsi, cmd);
> +			} else {
> +				iue = cmd->iue;
>  
> -			crq->valid = VALID_CMD_RESP_EL;
> -			crq->format = cmd->rsp.format;
> +				crq->valid = VALID_CMD_RESP_EL;
> +				crq->format = cmd->rsp.format;
>  
> -			if (cmd->flags & CMD_FAST_FAIL)
> -				crq->status = VIOSRP_ADAPTER_FAIL;
> +				if (cmd->flags & CMD_FAST_FAIL)
> +					crq->status = VIOSRP_ADAPTER_FAIL;
>  
> -			crq->IU_length = cpu_to_be16(cmd->rsp.len);
> +				crq->IU_length = cpu_to_be16(cmd->rsp.len);
>  
> -			rc = h_send_crq(vscsi->dma_dev->unit_address,
> -					be64_to_cpu(msg_hi),
> -					be64_to_cpu(cmd->rsp.tag));
> +				rc = h_send_crq(vscsi->dma_dev->unit_address,
> +						be64_to_cpu(msg_hi),
> +						be64_to_cpu(cmd->rsp.tag));
>  
> -			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
> -				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
> +				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);
> +				/* 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;
> +					ibmvscsis_free_cmd_resources(vscsi, cmd);
> +				} else {
> +					srp_snd_msg_failed(vscsi, rc);
> +					break;
> +				}
>  			}
>  		}
>  
> @@ -3581,9 +3590,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) {
> 

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Martin K. Petersen]

"Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:

> The driver is sending a response to the aborted task response along
> with LIO sending the tmr response.  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 TAS
> bit is set.

Somebody please review!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] ibmvscsis: Do not send aborted task response

[Martin K. Petersen]

"Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:

> The driver is sending a response to the aborted task response along
> with LIO sending the tmr response.  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 TAS
> bit is set.

Somebody please review!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

The driver is sending a response to the aborted task response
along with LIO sending the tmr response.
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 TAS bit is set.

Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 +++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 4bb5635..f75948a 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1758,33 +1758,42 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
 
 	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
 		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
-			iue = cmd->iue;
+			/*
+			 * If Task Abort Status Bit is set, then dont send a
+			 * response.
+			 */
+			if (cmd->se_cmd.transport_state & CMD_T_TAS) {
+				list_del(&cmd->list);
+				ibmvscsis_free_cmd_resources(vscsi, cmd);
+			} else {
+				iue = cmd->iue;
 
-			crq->valid = VALID_CMD_RESP_EL;
-			crq->format = cmd->rsp.format;
+				crq->valid = VALID_CMD_RESP_EL;
+				crq->format = cmd->rsp.format;
 
-			if (cmd->flags & CMD_FAST_FAIL)
-				crq->status = VIOSRP_ADAPTER_FAIL;
+				if (cmd->flags & CMD_FAST_FAIL)
+					crq->status = VIOSRP_ADAPTER_FAIL;
 
-			crq->IU_length = cpu_to_be16(cmd->rsp.len);
+				crq->IU_length = cpu_to_be16(cmd->rsp.len);
 
-			rc = h_send_crq(vscsi->dma_dev->unit_address,
-					be64_to_cpu(msg_hi),
-					be64_to_cpu(cmd->rsp.tag));
+				rc = h_send_crq(vscsi->dma_dev->unit_address,
+						be64_to_cpu(msg_hi),
+						be64_to_cpu(cmd->rsp.tag));
 
-			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
-				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+				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);
+				/* 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;
+					ibmvscsis_free_cmd_resources(vscsi, cmd);
+				} else {
+					srp_snd_msg_failed(vscsi, rc);
+					break;
+				}
 			}
 		}
 
@@ -3581,9 +3590,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) {
-- 
2.5.4 (Apple Git-61)

[PATCH] ibmvscsis: Do not send aborted task response

[Bryant G. Ly]

The driver is sending a response to the aborted task response
along with LIO sending the tmr response.
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 TAS bit is set.

Cc: <stable@vger.kernel.org> # v4.8+
Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 60 +++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 4bb5635..f75948a 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1758,33 +1758,42 @@ static void ibmvscsis_send_messages(struct scsi_info *vscsi)
 
 	if (!(vscsi->flags & RESPONSE_Q_DOWN)) {
 		list_for_each_entry_safe(cmd, nxt, &vscsi->waiting_rsp, list) {
-			iue = cmd->iue;
+			/*
+			 * If Task Abort Status Bit is set, then dont send a
+			 * response.
+			 */
+			if (cmd->se_cmd.transport_state & CMD_T_TAS) {
+				list_del(&cmd->list);
+				ibmvscsis_free_cmd_resources(vscsi, cmd);
+			} else {
+				iue = cmd->iue;
 
-			crq->valid = VALID_CMD_RESP_EL;
-			crq->format = cmd->rsp.format;
+				crq->valid = VALID_CMD_RESP_EL;
+				crq->format = cmd->rsp.format;
 
-			if (cmd->flags & CMD_FAST_FAIL)
-				crq->status = VIOSRP_ADAPTER_FAIL;
+				if (cmd->flags & CMD_FAST_FAIL)
+					crq->status = VIOSRP_ADAPTER_FAIL;
 
-			crq->IU_length = cpu_to_be16(cmd->rsp.len);
+				crq->IU_length = cpu_to_be16(cmd->rsp.len);
 
-			rc = h_send_crq(vscsi->dma_dev->unit_address,
-					be64_to_cpu(msg_hi),
-					be64_to_cpu(cmd->rsp.tag));
+				rc = h_send_crq(vscsi->dma_dev->unit_address,
+						be64_to_cpu(msg_hi),
+						be64_to_cpu(cmd->rsp.tag));
 
-			pr_debug("send_messages: cmd %p, tag 0x%llx, rc %ld\n",
-				 cmd, be64_to_cpu(cmd->rsp.tag), rc);
+				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);
+				/* 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;
+					ibmvscsis_free_cmd_resources(vscsi, cmd);
+				} else {
+					srp_snd_msg_failed(vscsi, rc);
+					break;
+				}
 			}
 		}
 
@@ -3581,9 +3590,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) {
-- 
2.5.4 (Apple Git-61)