* [PATCH] ibmvscsis: Do not send aborted task response
@ 2017-04-07 15:49 Bryant G. Ly
2017-04-07 15:56 ` Bryant G. Ly
0 siblings, 1 reply; 6+ messages in thread
From: Bryant G. Ly @ 2017-04-07 15:49 UTC (permalink / raw)
To: nab; +Cc: seroyer, linux-scsi, target-devel, Bryant G. Ly, stable
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvscsis: Do not send aborted task response
2017-04-07 15:49 Bryant G. Ly
@ 2017-04-07 15:56 ` Bryant G. Ly
0 siblings, 0 replies; 6+ messages in thread
From: Bryant G. Ly @ 2017-04-07 15:56 UTC (permalink / raw)
To: nab; +Cc: seroyer, linux-scsi, target-devel, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ibmvscsis: Do not send aborted task response
@ 2017-04-10 18:52 Bryant G. Ly
2017-04-20 2:29 ` Martin K. Petersen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bryant G. Ly @ 2017-04-10 18:52 UTC (permalink / raw)
To: nab, Bart.VanAssche
Cc: seroyer, linux-scsi, target-devel, Bryant G. Ly, stable
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvscsis: Do not send aborted task response
2017-04-10 18:52 [PATCH] ibmvscsis: Do not send aborted task response Bryant G. Ly
@ 2017-04-20 2:29 ` Martin K. Petersen
2017-04-25 20:18 ` Tyrel Datwyler
2017-05-02 4:19 ` Nicholas A. Bellinger
2 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2017-04-20 2:29 UTC (permalink / raw)
To: Bryant G. Ly
Cc: nab, Bart.VanAssche, seroyer, linux-scsi, target-devel, stable
"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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvscsis: Do not send aborted task response
2017-04-10 18:52 [PATCH] ibmvscsis: Do not send aborted task response Bryant G. Ly
2017-04-20 2:29 ` Martin K. Petersen
@ 2017-04-25 20:18 ` Tyrel Datwyler
2017-05-02 4:19 ` Nicholas A. Bellinger
2 siblings, 0 replies; 6+ messages in thread
From: Tyrel Datwyler @ 2017-04-25 20:18 UTC (permalink / raw)
To: Bryant G. Ly, nab, Bart.VanAssche
Cc: seroyer, linux-scsi, target-devel, stable
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) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvscsis: Do not send aborted task response
2017-04-10 18:52 [PATCH] ibmvscsis: Do not send aborted task response Bryant G. Ly
2017-04-20 2:29 ` Martin K. Petersen
2017-04-25 20:18 ` Tyrel Datwyler
@ 2017-05-02 4:19 ` Nicholas A. Bellinger
2 siblings, 0 replies; 6+ messages in thread
From: Nicholas A. Bellinger @ 2017-05-02 4:19 UTC (permalink / raw)
To: Bryant G. Ly; +Cc: Bart.VanAssche, seroyer, linux-scsi, target-devel, stable
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-02 4:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-10 18:52 [PATCH] ibmvscsis: Do not send aborted task response Bryant G. Ly
2017-04-20 2:29 ` Martin K. Petersen
2017-04-25 20:18 ` Tyrel Datwyler
2017-05-02 4:19 ` Nicholas A. Bellinger
-- strict thread matches above, loose matches on Subject: below --
2017-04-07 15:49 Bryant G. Ly
2017-04-07 15:56 ` Bryant G. Ly
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).