diff mbox

ibmvscsis: Do not send aborted task response

Message ID 1491580154-30489-1-git-send-email-bryantly@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Bryant G. Ly April 7, 2017, 3:49 p.m. UTC
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(-)

Comments

Bryant G. Ly April 7, 2017, 3:56 p.m. UTC | #1
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
Bart Van Assche April 7, 2017, 4:02 p.m. UTC | #2
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.
Bart Van Assche April 7, 2017, 4:36 p.m. UTC | #3
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.
Bryant G. Ly April 7, 2017, 5:01 p.m. UTC | #4
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
Michael Cyr April 7, 2017, 9:14 p.m. UTC | #5
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?
Bart Van Assche April 7, 2017, 9:23 p.m. UTC | #6
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.
diff mbox

Patch

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;
 };