| Message ID | 1494281262-56000-1-git-send-email-bryantly@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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; > };
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; };