[PATCH-v1 20/22] Update ABORT processing for NVMET.

[jthumshirn@suse.de (Johannes Thumshirn)]

On Wed, Apr 19, 2017@09:46:39PM -0700, jsmart2021@gmail.com wrote:
> From: James Smart <jsmart2021 at gmail.com>
> 
> The driver with nvme had this routine stubbed.
> 
> Right now XRI_ABORTED_CQE is not handled and the FC NVMET
> Transport has a new API for the driver.
> 
> Missing code path, new NVME abort API
> Update ABORT processing for NVMET
> 
> There are 3 new FC NVMET Transport API/ template routines for NVMET:
> 
> lpfc_nvmet_xmt_fcp_release
> This NVMET template callback routine called to release context
> associated with an IO This routine is ALWAYS called last, even
> if the IO was aborted or completed in error.
> 
> lpfc_nvmet_xmt_fcp_abort
> This NVMET template callback routine called to abort an exchange that
> has an IO in progress
> 
> nvmet_fc_rcv_fcp_req
> When the lpfc driver receives an ABTS, this NVME FC transport layer
> callback routine is called. For this case there are 2 paths thru the
> driver: the driver either has an outstanding exchange / context for the
> XRI to be aborted or not.  If not, a BA_RJT is issued otherwise a BA_ACC
> 
> NVMET Driver abort paths:
> 
> There are 2 paths for aborting an IO. The first one is we receive an IO and
> decide not to process it because of lack of resources. An unsolicated ABTS
> is immediately sent back to the initiator as a response.
> lpfc_nvmet_unsol_fcp_buffer
>             lpfc_nvmet_unsol_issue_abort  (XMIT_SEQUENCE_WQE)
> 
> The second one is we sent the IO up to the NVMET transport layer to
> process, and for some reason the NVME Transport layer decided to abort the
> IO before it completes all its phases. For this case there are 2 paths
> thru the driver:
> the driver either has an outstanding TSEND/TRECEIVE/TRSP WQE or no
> outstanding WQEs are present for the exchange / context.
> lpfc_nvmet_xmt_fcp_abort
>     if (LPFC_NVMET_IO_INP)
>         lpfc_nvmet_sol_fcp_issue_abort  (ABORT_WQE)
>                 lpfc_nvmet_sol_fcp_abort_cmp
>     else
>         lpfc_nvmet_unsol_fcp_issue_abort
>                 lpfc_nvmet_unsol_issue_abort  (XMIT_SEQUENCE_WQE)
>                         lpfc_nvmet_unsol_fcp_abort_cmp
> 
> Context flags:
> LPFC_NVMET_IOP - his flag signifies an IO is in progress on the exchange.
> LPFC_NVMET_XBUSY  - this flag indicates the IO completed but the firmware
> is still busy with the corresponding exchange. The exchange should not be
> reused until after a XRI_ABORTED_CQE is received for that exchange.
> LPFC_NVMET_ABORT_OP - this flag signifies an ABORT_WQE was issued on the
> exchange.
> LPFC_NVMET_CTX_RLS  - this flag signifies a context free was requested,
> but we are deferring it due to an XBUSY or ABORT in progress.
> 
> A ctxlock is added to the context structure that is used whenever these
> flags are set/read  within the context of an IO.
> The LPFC_NVMET_CTX_RLS flag is only set in the defer_relase routine when
> the transport has resolved all IO associated with the buffer. The flag is
> cleared when the CTX is associated with a new IO.
> 
> An exchange can has both an LPFC_NVMET_XBUSY and a LPFC_NVMET_ABORT_OP
> condition active simultaneously. Both conditions must complete before the
> exchange is freed.
> When the abort callback (lpfc_nvmet_xmt_fcp_abort) is envoked:
> If there is an outstanding IO, the driver will issue an ABORT_WQE. This
> should result in 3 completions for the exchange:
> 1) IO cmpl with XB bit set
> 2) Abort WQE cmpl
> 3) XRI_ABORTED_CQE cmpl
> For this scenerio, after completion #1, the NVMET Transport IO rsp
> callback is called.  After completion #2, no action is taken with respect
> to the exchange / context.  After completion #3, the exchange context is
> free for re-use on another IO.
> 
> If there is no outstanding activity on the exchange, the driver will send a
> ABTS to the Initiator. Upon completion of this WQE, the exchange / context
> is freed for re-use on another IO.
> 
> Signed-off-by: Dick Kennedy <dick.kennedy at broadcom.com>
> Signed-off-by: James Smart <james.smart at broadcom.com>
> ---

[...]

> @@ -139,6 +159,11 @@ lpfc_nvmet_rq_post(struct lpfc_hba *phba, struct lpfc_nvmet_rcv_ctx *ctxp,
>  		   struct lpfc_dmabuf *mp)
>  {
>  	if (ctxp) {
> +		if (ctxp->flag)
> +			lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +			"6314 rq_post ctx xri x%x flag x%x\n",
> +			ctxp->oxid, ctxp->flag);
> +

Indendation looks a bit odd here. Not sure if GCC-6's -Wmisleading-indent will
flag this.

> @@ -801,7 +847,122 @@ void
>  lpfc_sli4_nvmet_xri_aborted(struct lpfc_hba *phba,
>  			    struct sli4_wcqe_xri_aborted *axri)
>  {
> -	/* TODO: work in progress */
> +	uint16_t xri = bf_get(lpfc_wcqe_xa_xri, axri);
> +	uint16_t rxid = bf_get(lpfc_wcqe_xa_remote_xid, axri);
> +	struct lpfc_nvmet_rcv_ctx *ctxp, *next_ctxp;
> +	struct lpfc_nodelist *ndlp;
> +	unsigned long iflag = 0;
> +	int rrq_empty = 0;
> +	bool released = false;
> +
> +	lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +			"6317 XB aborted xri x%x rxid x%x\n", xri, rxid);
> +
> +	if (!(phba->cfg_enable_fc4_type & LPFC_ENABLE_NVME))
> +		return;
> +	spin_lock_irqsave(&phba->hbalock, iflag);
> +	spin_lock(&phba->sli4_hba.abts_nvme_buf_list_lock);
> +	list_for_each_entry_safe(ctxp, next_ctxp,
> +				 &phba->sli4_hba.lpfc_abts_nvmet_ctx_list,
> +				 list) {
> +		if (ctxp->rqb_buffer->sglq->sli4_xritag == xri) {

		if (ctxp->rqb_buffer->sglq->sli4_xritag != xri)
			continue;
			????

This saves the indent so the block below doesn't look so squezed and you don't
have to have a linebreak in spin_unlock() for instance.

> +			/* Check if we already received a free context call
> +			 * and we have completed processing an abort situation.
> +			 */
> +			if ((ctxp->flag & LPFC_NVMET_CTX_RLS) &&
> +			    !(ctxp->flag & LPFC_NVMET_ABORT_OP)) {
> +				list_del(&ctxp->list);
> +				released = true;
> +			}
> +			ctxp->flag &= ~LPFC_NVMET_XBUSY;
> +			spin_unlock(
> +				&phba->sli4_hba.abts_nvme_buf_list_lock);
> +
> +			rrq_empty = list_empty(&phba->active_rrq_list);
> +			spin_unlock_irqrestore(&phba->hbalock, iflag);
> +			ndlp = lpfc_findnode_did(phba->pport, ctxp->sid);
> +			if (ndlp && NLP_CHK_NODE_ACT(ndlp) &&
> +			    ((ndlp->nlp_state == NLP_STE_UNMAPPED_NODE) ||
> +			    (ndlp->nlp_state == NLP_STE_MAPPED_NODE))) {
> +				lpfc_set_rrq_active(
> +					phba, ndlp,
> +					ctxp->rqb_buffer->sglq->sli4_lxritag,
> +					rxid, 1);
> +				lpfc_sli4_abts_err_handler(phba, ndlp, axri);
> +			}
> +
> +			lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +					"6318 XB aborted %x flg x%x (%x)\n",
> +					ctxp->oxid, ctxp->flag, released);
> +			if (released)
> +				lpfc_nvmet_rq_post(phba, ctxp,
> +						   &ctxp->rqb_buffer->hbuf);
> +			if (rrq_empty)
> +				lpfc_worker_wake_up(phba);
> +			return;
> +		}
> +	}
> +	spin_unlock(&phba->sli4_hba.abts_nvme_buf_list_lock);
> +	spin_unlock_irqrestore(&phba->hbalock, iflag);
> +}
> +
> +int
> +lpfc_nvmet_rcv_unsol_abort(struct lpfc_vport *vport,
> +			   struct fc_frame_header *fc_hdr)
> +
> +{
> +#if (IS_ENABLED(CONFIG_NVME_TARGET_FC))
> +	struct lpfc_hba *phba = vport->phba;
> +	struct lpfc_nvmet_rcv_ctx *ctxp, *next_ctxp;
> +	struct nvmefc_tgt_fcp_req *rsp;
> +	uint16_t xri;
> +	unsigned long iflag = 0;
> +
> +	xri = be16_to_cpu(fc_hdr->fh_ox_id);
> +
> +	spin_lock_irqsave(&phba->hbalock, iflag);
> +	spin_lock(&phba->sli4_hba.abts_nvme_buf_list_lock);
> +	list_for_each_entry_safe(ctxp, next_ctxp,
> +				 &phba->sli4_hba.lpfc_abts_nvmet_ctx_list,
> +				 list) {

Same here.

> +		if (ctxp->rqb_buffer->sglq->sli4_xritag == xri) {
> +			spin_unlock(&phba->sli4_hba.abts_nvme_buf_list_lock);
> +			spin_unlock_irqrestore(&phba->hbalock, iflag);
> +
> +			spin_lock_irqsave(&ctxp->ctxlock, iflag);
> +			ctxp->flag |= LPFC_NVMET_ABTS_RCV;
> +			spin_unlock_irqrestore(&ctxp->ctxlock, iflag);
> +
> +			lpfc_nvmeio_data(phba,
> +					 "NVMET ABTS RCV: "
> +					 "xri x%x CPU %02x rjt %d\n",
> +					 xri, smp_processor_id(), 0);
> +
> +			lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +					"6319 NVMET Rcv ABTS:acc xri x%x\n",
> +					xri);
> +
> +			rsp = &ctxp->ctx.fcp_req;
> +			nvmet_fc_rcv_fcp_abort(phba->targetport, rsp);
> +
> +			/* Respond with BA_ACC accordingly */
> +			lpfc_sli4_seq_abort_rsp(vport, fc_hdr, 1);
> +			return 0;
> +		}
> +	}
> +	spin_unlock(&phba->sli4_hba.abts_nvme_buf_list_lock);
> +	spin_unlock_irqrestore(&phba->hbalock, iflag);
> +
> +	lpfc_nvmeio_data(phba, "NVMET ABTS RCV: xri x%x CPU %02x rjt %d\n",
> +			 xri, smp_processor_id(), 1);
> +
> +	lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +			"6320 NVMET Rcv ABTS:rjt xri x%x\n", xri);
> +
> +	/* Respond with BA_RJT accordingly */
> +	lpfc_sli4_seq_abort_rsp(vport, fc_hdr, 0);
> +	return 0;
> +#endif
>  }

> @@ -1677,10 +1847,15 @@ lpfc_nvmet_sol_fcp_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
>  	cmdwqe->context2 = NULL;
>  	cmdwqe->context3 = NULL;
>  	lpfc_sli_release_iocbq(phba, cmdwqe);
> +
> +	/* Since iaab/iaar are NOT set, there is no work left.
> +	 * For LPFC_NVMET_XBUSY, lpfc_sli4_nvmet_xri_aborted
> +	 * should have been called already.
> +	 */
>  }
>  
>  /**
> - * lpfc_nvmet_xmt_fcp_abort_cmp - Completion handler for ABTS
> + * lpfc_nvmet_unsol_fcp_abort_cmp - Completion handler for ABTS
>   * @phba: Pointer to HBA context object.
>   * @cmdwqe: Pointer to driver command WQE object.
>   * @wcqe: Pointer to driver response CQE object.
> @@ -1690,8 +1865,8 @@ lpfc_nvmet_sol_fcp_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
>   * The function frees memory resources used for the NVME commands.
>   **/
>  static void
> -lpfc_nvmet_xmt_fcp_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
> -			     struct lpfc_wcqe_complete *wcqe)
> +lpfc_nvmet_unsol_fcp_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
> +			       struct lpfc_wcqe_complete *wcqe)
>  {
>  	struct lpfc_nvmet_rcv_ctx *ctxp;
>  	struct lpfc_nvmet_tgtport *tgtp;
> @@ -1706,35 +1881,54 @@ lpfc_nvmet_xmt_fcp_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
>  	tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
>  	atomic_inc(&tgtp->xmt_abort_cmpl);
>  
> +	if (!ctxp) {
> +		lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
> +				"6070 ABTS cmpl: WCQE: %08x %08x %08x %08x\n",
> +				wcqe->word0, wcqe->total_data_placed,
> +				result, wcqe->word3);
> +		return;
> +	}
> +
> +	/* Sanity check */
> +	if (ctxp->state != LPFC_NVMET_STE_ABORT) {
> +		lpfc_printf_log(phba, KERN_ERR, LOG_NVME_ABTS,
> +				"6112 ABTS Wrong state:%d oxid x%x\n",
> +				ctxp->state, ctxp->oxid);

OK, this is kinda odd as well. We do a sanity check on the state but don't
take any measures other than writing a log message? Maybe that's OK, but it
surely looks a bit odd to me when reviewing. If it's OK please amend the
comment above and explain this is intented behaviour.


Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850