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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx> # v4.8+
> Signed-off-by: Bryant G. Ly <bryantly@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Michael Cyr <mikecyr@xxxxxxxxxxxxxxxxxx>
> ---
>  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;
>  };





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]