lpfc: nodelist pointer cleanup
Subsystems (2)
cover lpfc: nodelist pointer cleanup
Hi James,
trying to figure this annoying lpfc_set_rrq_active() bug I've found the nodelist pointer handling in the lpfc io buffers a bit strange; there's a 'ndlp' pointer, but for scsi the nodelist is primarily accessed via the 'rdata' pointer (although not everywhere). For NVMe it's primarily the 'ndlp' pointer, apparently, but the usage is quite confusing.
So here's a patchset to straighten things up; it primarily moves the anonymous protocol-specific structure in the io buffer to a named one, and always accesses the nodelist through the protocol-specific rport data structure.
It also has the nice side-effect that the protocol-specific areas are aligned now, so clearing the 'rdata' pointer on the scsi side will be equivalent to clearing the 'rport' pointer on the nvme side. And it reduces the size of the io buffer.
Let me know what you think.
Hannes Reinecke (3):
lpfc: use named structure for combined I/O buffer
lpfc: access nodelist through scsi-specific rdata pointer
lpfc: access nvme nodelist through nvme rport structure
drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 56 ++++++------ drivers/scsi/lpfc/lpfc_scsi.c | 196 +++++++++++++++++++++--------------------- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 6 +- 5 files changed, 143 insertions(+), 143 deletions(-)
Signature · 1 line
2.16.4
PATCH 2/3 lpfc: access nodelist through scsi-specific rdata pointer
Access the nodelist through the scsi-specific rdata pointer when handling SCSI requests.
--- drivers/scsi/lpfc/lpfc_scsi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/scsi/lpfc/lpfc_scsi.c +2 -4
@@ -808,8 +808,8 @@ lpfc_release_scsi_buf_s4(struct lpfc_hba *phba, struct lpfc_io_buf *psb)static void lpfc_release_scsi_buf (struct lpfc_hba *phba,struct lpfc_io_buf *psb) { -if ((psb->flags &LPFC_SBUF_BUMP_QDEPTH ) && psb->ndlp) -atomic_dec (&psb->ndlp->cmd_pending); +if ((psb->flags &LPFC_SBUF_BUMP_QDEPTH ) && psb->scsi.rdata->pnode) +atomic_dec (&psb->scsi.rdata->pnode->cmd_pending); psb->flags &= ~LPFC_SBUF_BUMP_QDEPTH ; phba->lpfc_release_scsi_buf (phba, psb);
@@ -4580,7 +4580,6 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)*/ lpfc_cmd->scsi.pCmd = cmnd; lpfc_cmd->scsi.rdata = rdata; - lpfc_cmd->ndlp = ndlp; cmnd->host_scribble = (unsigned char *)lpfc_cmd;if (scsi_get_prot_op (cmnd) !=SCSI_PROT_NORMAL ) {
@@ -5035,7 +5034,6 @@ lpfc_send_taskmgmt(struct lpfc_vport *vport, struct scsi_cmnd *cmnd, lpfc_cmd->timeout = phba->cfg_task_mgmt_tmo; lpfc_cmd->scsi.rdata = rdata; lpfc_cmd->scsi.pCmd = cmnd; - lpfc_cmd->ndlp = pnode; status =lpfc_scsi_prep_task_mgmt_cmd (vport, lpfc_cmd, lun_id, task_mgmt_cmd);
Signature · 1 line
2.16.4
PATCH 3/3 lpfc: access nvme nodelist through nvme rport structure
In reply to Hannes ReineckeInstead of referencing the nodelist directly via a point in the I/O buffer we should be storing the nvme rport pointer in the protocol-specific section and access the nodelist through that. With this we reduce the size of the I/O buffer, eliminate a possible race and align the protoco-specific sections.
--- drivers/scsi/lpfc/lpfc_nvme.c | 14 ++++++++------ drivers/scsi/lpfc/lpfc_sli.h | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-)
drivers/scsi/lpfc/lpfc_nvme.c +8 -6
@@ -1056,7 +1056,7 @@ lpfc_nvme_io_cmd_wqe_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pwqeIn,* Catch race where our node has transitioned, but the * transport is still transitioning. */ - ndlp = lpfc_ncmd->ndlp; + ndlp = lpfc_ncmd->nvme.rport->ndlp;if (!ndlp || !NLP_CHK_NODE_ACT (ndlp)) {lpfc_printf_vlog (vport,KERN_ERR ,LOG_NVME_IOERR ,"6062 Ignoring NVME cmpl. No ndlp\n" );
@@ -1695,7 +1695,7 @@ lpfc_nvme_fcp_io_submit(struct nvme_fc_local_port *pnvme_lport,*/ freqpriv->nvme_buf = lpfc_ncmd; lpfc_ncmd->nvme.nvmeCmd = pnvme_fcreq; - lpfc_ncmd->ndlp = ndlp; + lpfc_ncmd->nvme.rport = rport; lpfc_ncmd->nvme.qidx = lpfc_queue_info->qidx;/*
@@ -2085,11 +2085,13 @@ lpfc_release_nvme_buf(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_ncmd) {struct lpfc_sli4_hdw_queue *qp;unsigned long iflag =0 ; +struct lpfc_nodelist *ndlp = + lpfc_ncmd->nvme.rport ? lpfc_ncmd->nvme.rport->ndlp :NULL ; -if ((lpfc_ncmd->flags &LPFC_SBUF_BUMP_QDEPTH ) && lpfc_ncmd->ndlp) -atomic_dec (&lpfc_ncmd->ndlp->cmd_pending); +if ((lpfc_ncmd->flags &LPFC_SBUF_BUMP_QDEPTH ) && ndlp) +atomic_dec (&ndlp->cmd_pending); - lpfc_ncmd->ndlp =NULL ; + lpfc_ncmd->nvme.rport =NULL ; lpfc_ncmd->flags &= ~LPFC_SBUF_BUMP_QDEPTH ; qp = lpfc_ncmd->hdwq;
@@ -2646,7 +2648,7 @@ lpfc_sli4_nvme_xri_aborted(struct lpfc_hba *phba, {uint16_t xri =bf_get (lpfc_wcqe_xa_xri, axri);struct nvmefc_fcp_req *nvme_cmd =NULL ; -struct lpfc_nodelist *ndlp = lpfc_ncmd->ndlp; +struct lpfc_nodelist *ndlp = lpfc_ncmd->nvme.rport->ndlp;if (ndlp)
drivers/scsi/lpfc/lpfc_sli.h +1 -1
@@ -382,7 +382,6 @@ struct lpfc_io_buf {uint16_t hdwq_no;uint16_t cpu; -struct lpfc_nodelist *ndlp;uint32_t timeout;uint16_t flags;/* TBD convert exch_busy to flags */ #define LPFC_SBUF_XBUSY 0x1/* SLI4 hba reported XB on WCQE cmpl */
@@ -439,6 +438,7 @@ struct lpfc_io_buf {/* NVME specific fields */ struct {struct nvmefc_fcp_req *nvmeCmd; +struct lpfc_nvme_rport *rport;uint16_t qidx; } nvme; };
Signature · 1 line
2.16.4
PATCH 1/3 lpfc: use named structure for combined I/O buffer
In reply to Hannes ReineckeUse named structures in the combined I/O buffer to annotate the fields with the intended protocol usage.
--- drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 42 +++++----- drivers/scsi/lpfc/lpfc_scsi.c | 190 +++++++++++++++++++++--------------------- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 4 +- 5 files changed, 132 insertions(+), 132 deletions(-)
drivers/scsi/lpfc/lpfc_init.c +1 -1
@@ -1081,7 +1081,7 @@ lpfc_hba_down_post_s4(struct lpfc_hba *phba) &aborts);list_for_each_entry_safe (psb, psb_next, &aborts, list) { - psb->pCmd =NULL ; + psb->scsi.pCmd =NULL ; psb->status =IOSTAT_SUCCESS ; cnt++; }
drivers/scsi/lpfc/lpfc_nvme.c +21 -21
140 lines not on this page · raw message
drivers/scsi/lpfc/lpfc_scsi.c +95 -95
547 lines not on this page · raw message
drivers/scsi/lpfc/lpfc_sli.c +13 -13
@@ -1157,7 +1157,7 @@ __lpfc_sli_get_els_sglq(struct lpfc_hba *phba, struct lpfc_iocbq *piocbq)if (piocbq->iocb_flag &LPFC_IO_FCP ) { lpfc_cmd = (struct lpfc_io_buf *) piocbq->context1; - ndlp = lpfc_cmd->rdata->pnode; + ndlp = lpfc_cmd->scsi.rdata->pnode; }else if ((piocbq->iocb.ulpCommand ==CMD_GEN_REQUEST64_CR ) && !(piocbq->iocb_flag &LPFC_IO_LIBDFC )) { ndlp = piocbq->context_un.ndlp;
@@ -9531,7 +9531,7 @@ lpfc_sli4_iocb2wqe(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq, lpfc_cmd = iocbq->context1; sgl = (struct sli4_sge *)lpfc_cmd->dma_sgl; - fcp_cmnd = lpfc_cmd->fcp_cmnd; + fcp_cmnd = lpfc_cmd->scsi.fcp_cmnd;/* Word 0-2 - FCP_CMND */ wqe->generic.bde.tus.f.bdeFlags =
@@ -9595,7 +9595,7 @@ lpfc_sli4_iocb2wqe(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq, lpfc_cmd = iocbq->context1; sgl = (struct sli4_sge *)lpfc_cmd->dma_sgl; - fcp_cmnd = lpfc_cmd->fcp_cmnd; + fcp_cmnd = lpfc_cmd->scsi.fcp_cmnd;/* Word 0-2 - FCP_CMND */ wqe->generic.bde.tus.f.bdeFlags =
@@ -9652,7 +9652,7 @@ lpfc_sli4_iocb2wqe(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq, lpfc_cmd = iocbq->context1; sgl = (struct sli4_sge *)lpfc_cmd->dma_sgl; - fcp_cmnd = lpfc_cmd->fcp_cmnd; + fcp_cmnd = lpfc_cmd->scsi.fcp_cmnd;/* Word 0-2 - FCP_CMND */ wqe->generic.bde.tus.f.bdeFlags =
@@ -11371,19 +11371,19 @@ lpfc_sli_validate_fcp_iocb(struct lpfc_iocbq *iocbq, struct lpfc_vport *vport, lpfc_cmd = container_of(iocbq, struct lpfc_io_buf, cur_iocbq); -if (lpfc_cmd->pCmd ==NULL ) +if (lpfc_cmd->scsi.pCmd ==NULL )return rc;switch (ctx_cmd) {case LPFC_CTX_LUN : -if ((lpfc_cmd->rdata) && (lpfc_cmd->rdata->pnode) && - (lpfc_cmd->rdata->pnode->nlp_sid == tgt_id) && - (scsilun_to_int (&lpfc_cmd->fcp_cmnd->fcp_lun) == lun_id)) +if ((lpfc_cmd->scsi.rdata) && (lpfc_cmd->scsi.rdata->pnode) && + (lpfc_cmd->scsi.rdata->pnode->nlp_sid == tgt_id) && + (scsilun_to_int (&lpfc_cmd->scsi.fcp_cmnd->fcp_lun) == lun_id)) rc =0 ;break ;case LPFC_CTX_TGT : -if ((lpfc_cmd->rdata) && (lpfc_cmd->rdata->pnode) && - (lpfc_cmd->rdata->pnode->nlp_sid == tgt_id)) +if ((lpfc_cmd->scsi.rdata) && (lpfc_cmd->scsi.rdata->pnode) && + (lpfc_cmd->scsi.rdata->pnode->nlp_sid == tgt_id)) rc =0 ;break ;case LPFC_CTX_HOST :
@@ -11625,7 +11625,7 @@ lpfc_sli_abort_taskmgmt(struct lpfc_vport *vport, struct lpfc_sli_ring *pring, lpfc_cmd = container_of(iocbq, struct lpfc_io_buf, cur_iocbq);spin_lock (&lpfc_cmd->buf_lock); -if (!lpfc_cmd->pCmd) { +if (!lpfc_cmd->scsi.pCmd) {spin_unlock (&lpfc_cmd->buf_lock);continue ; }
@@ -11681,7 +11681,7 @@ lpfc_sli_abort_taskmgmt(struct lpfc_vport *vport, struct lpfc_sli_ring *pring,if (iocbq->iocb_flag &LPFC_IO_FOF ) abtsiocbq->iocb_flag |=LPFC_IO_FOF ; - ndlp = lpfc_cmd->rdata->pnode; + ndlp = lpfc_cmd->scsi.rdata->pnode;if (lpfc_is_link_up (phba) && (ndlp && ndlp->nlp_state ==NLP_STE_MAPPED_NODE ))
@@ -20132,7 +20132,7 @@ void lpfc_release_io_buf(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_ncmd,u32 abts_io_bufs;/* MUST zero fields if buffer is reused by another protocol */ - lpfc_ncmd->nvmeCmd =NULL ; + lpfc_ncmd->nvme.nvmeCmd =NULL ; lpfc_ncmd->cur_iocbq.wqe_cmpl =NULL ; lpfc_ncmd->cur_iocbq.iocb_cmpl =NULL ;
drivers/scsi/lpfc/lpfc_sli.h +2 -2
@@ -434,13 +434,13 @@ struct lpfc_io_buf {#define LPFC_INJERR_APPTAG 2#define LPFC_INJERR_GUARD 3#endif - }; + } scsi;/* NVME specific fields */ struct {struct nvmefc_fcp_req *nvmeCmd;uint16_t qidx; - }; + } nvme; };#ifdef CONFIG_SCSI_LPFC_DEBUG_FS uint64_t ts_cmd_start;
Signature · 1 line
2.16.4
Quoted 25 lines · Hannes Reinecke
Hi James,
trying to figure this annoying
lpfc_set_rrq_active()bug I've found the nodelist pointer handling in the lpfc io buffers a bit strange; there's a 'ndlp' pointer, but for scsi the nodelist is primarily accessed via the 'rdata' pointer (although not everywhere). For NVMe it's primarily the 'ndlp' pointer, apparently, but the usage is quite confusing.
So here's a patchset to straighten things up; it primarily moves the anonymous protocol-specific structure in the io buffer to a named one, and always accesses the nodelist through the protocol-specific rport data structure.It also has the nice side-effect that the protocol-specific areas are aligned now, so clearing the 'rdata' pointer on the scsi side will be equivalent to clearing the 'rport' pointer on the nvme side. And it reduces the size of the io buffer.
Let me know what you think.
Hannes Reinecke (3):
lpfc: use named structure for combined I/O buffer
lpfc: access nodelist through scsi-specific rdata pointer
lpfc: access nvme nodelist through nvme rport structure
Quoted 7 lines
drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 56 ++++++------ drivers/scsi/lpfc/lpfc_scsi.c | 196 +++++++++++++++++++++--------------------- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 6 +- 5 files changed, 143 insertions(+), 143 deletions(-)
Well, the problem I think you are trying to solve is ultimately the root issue that is solved by this patch in the just-posted 12.6.0.0 patch set:
[PATCH 05/16] lpfc: Fix bad ndlp ptr in xri aborted handling
As such, I'd like to see if the 12.6.0.0 patch resolves the issue before going through all the shifting in your patches.
Note: the failing routine can change as it's totally dependent on where the bogus pointer value takes you. The key is the lpfc_sli4_sp_handle_abort_xri_wcqe() routine on the stack.
-- james
Quoted 35 lines · James Smart
Quoted 25 lines · Hannes Reinecke
Hi James,
trying to figure this annoying
lpfc_set_rrq_active()bug I've found the nodelist pointer handling in the lpfc io buffers a bit strange; there's a 'ndlp' pointer, but for scsi the nodelist is primarily accessed via the 'rdata' pointer (although not everywhere). For NVMe it's primarily the 'ndlp' pointer, apparently, but the usage is quite confusing.
So here's a patchset to straighten things up; it primarily moves the anonymous protocol-specific structure in the io buffer to a named one, and always accesses the nodelist through the protocol-specific rport data structure.It also has the nice side-effect that the protocol-specific areas are aligned now, so clearing the 'rdata' pointer on the scsi side will be equivalent to clearing the 'rport' pointer on the nvme side. And it reduces the size of the io buffer.
Let me know what you think.
Hannes Reinecke (3):
lpfc: use named structure for combined I/O buffer
lpfc: access nodelist through scsi-specific rdata pointer
lpfc: access nvme nodelist through nvme rport structureQuoted 8 lines
drivers/scsi/lpfc/lpfc_init.c | 2 +- drivers/scsi/lpfc/lpfc_nvme.c | 56 ++++++------ drivers/scsi/lpfc/lpfc_scsi.c | 196 +++++++++++++++++++++--------------------- drivers/scsi/lpfc/lpfc_sli.c | 26 +++--- drivers/scsi/lpfc/lpfc_sli.h | 6 +- 5 files changed, 143 insertions(+), 143 deletions(-)
Quoted 6 lines
Well, the problem I think you are trying to solve is ultimately the root issue that is solved by this patch in the just-posted 12.6.0.0 patch set:
[PATCH 05/16] lpfc: Fix bad ndlp ptr in xri aborted handlingAs such, I'd like to see if the 12.6.0.0 patch resolves the issue before going through all the shifting in your patches.
Note: the failing routine can change as it's totally dependent on where the bogus pointer value takes you. The key is thelpfc_sli4_sp_handle_abort_xri_wcqe()routine on the stack.
Fair enough.
I'll be giving your patchset a spin once submitted.
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)