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: <[email protected]> # v4.8+ > Signed-off-by: Bryant G. Ly <[email protected]> > Signed-off-by: Michael Cyr <[email protected]> > --- > 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; > };
