Re: [PATCH v2 3/6] qla2xxx: Add FC-NVMe F/W initialization and transport registration

[Johannes Thumshirn]

On Fri, Jun 23, 2017 at 03:16:09AM +, Madhani, Himanshu wrote:
> if this is not *must* i’ll like to post patch for this with other patches 
> that I am going to queue up during rc1 phase. 

Ok.

[...]

> I saw your response to James that this is okay for FC NVMe code.
> 
> > [...]
> > 
> >> +  vha = (struct scsi_qla_host *)lport->private;
> > 
> > No need to cast from void *
> >> +  ql_log(ql_log_info, vha, 0x2104,
> >> +  "%s: handle %p, idx =%d, qsize %d\n",
> >> +  __func__, handle, qidx, qsize);
> > 
> > Btw, sometime in the future you could change your ql_log() thingies to the
> > kernel's dyndebug facility.
> > 
> > […]
> 
> Thanks for the suggestions. I’ll bring it up to team and we can slowly 
> convert these to kernel’s
> dynamic debugging facility. 

Thanks a lot.
> 
> 
> >> +  rval = ha->isp_ops->abort_command(sp);
> >> +  if (rval != QLA_SUCCESS)
> >> +  ql_log(ql_log_warn, fcport->vha, 0x2125,
> >> +  "%s: failed to abort LS command for SP:%p rval=%x\n",
> >> +  __func__, sp, rval);
> >> +
> >> +  ql_dbg(ql_dbg_io, fcport->vha, 0x212b,
> >> +  "%s: aborted sp:%p on fcport:%p\n", __func__, sp, fcport);
> > 
> > If you insinst in having these two messages ("failed to abort" and 
> > "aborted")
> > can you at least fold it into one print statement.
> > 
> 
> I’ll send follow up patch for this cleanup, if its okay with you? 

OK

[...]
> > I've just seen this in qla2xxx_start_scsi_mq() and
> > qla2xxx_dif_start_scsi_mq() and was about to send you an RFC patch. But
> > here it is for completeness in the nvme version as well:
> > 
> > You save a pointer to the req_que from you qpair and _afterwards_ you grab
> > the qp_lock. What prevents someone from changing the request internals
> > underneath you?
> > 
> > Like this:
> > 
> > CPU0   CPU1
> > req = qpair->req;
> > qla2xxx_delete_qpair(vha, qpair);
> > `-> ret = qla25xx_delete_req_que(vha, 
> > qpair->req);
> > spin_lock_irqsave(>qp_lock, flags);
> > handle = req->current_outstanding_cmd;
> > 
> > Oh and btw, neither qla2xxx_delete_qpair() nor qla25xx_delete_req_que() grab
> > the qp_lock.
> > 
> > I think this is something work re-thinking. Maybe you can identify the 
> > blocks
> > accessing struct members which need to be touched under a lock and extract
> > them into a helper function wich calls lockdep_assert_held(). No must just 
> > and
> > idea.
> > 
> 
> This is very valid point you brought up and thanks for the detail review 
> comment. 
> from your patch submitted this morning, I’ll like to have our test team run 
> through 
> regression testing with these changes and we can incorporate that into NVMe 
> as well
> and send a follow up patch to correct this. Would you be okay with that? 

That patch has a bug and I'll need to respin it, but I'll be sending you a v2
today.

> 
> > [...]
> >> +
> >> +  /* Load data segments */
> >> +  for_each_sg(sgl, sg, tot_dsds, i) {
> > 
> > Do you really need the whole loop under a spin_lock_irqsave()? If the sglist
> > has a lot of entries (i.e. becasue we couldn't cluster it) we're in risk to
> > trigger a NMI watchdog soft-lockup WARN_ON(). You need to grab the lock when
> > accessing req's members but the rest of the loop? This applies to
> > qla24xx_build_scsi_iocbs() for SCSI as well.
> > 
> 
> Since these changes would need us to do regression testing, I would like to 
> send a follow up 
> patch to correct them as a separate patch.

Sure.

> 
> > [...]
> > 
> >> +  struct qla_qpair *qpair = (struct qla_qpair *)hw_queue_handle;
> > 
> > Void pointer cast. Someone really should write a coccinelle script to get 
> > rid
> > of em.
> > 
> 
> Will send a follow up patch for cleanup
> 
> > [...]
> > 
> >> +  /* Alloc SRB structure */
> >> +  sp = qla2xxx_get_qpair_sp(qpair, fcport, GFP_ATOMIC);
> >> +  if (!sp)
> >> +  return -EIO;
> > 
> > __blk_mq_run_hw_queue()
> > `-> blk_mq_sched_dispatch_requests()
> >`-> blk_mq_dispatch_rq_list()
> >`-> nvme_fc_queue_rq()
> >`-> nvme_fc_start_fcp_op() 
> >`-> qla_nvme_post_cmd()
> > isn't called from an IRQ context and qla2xxx_get_qpair_sp() internally
> > uses mempool_alloc(). From mempool_alloc()'s documentation:
> > 
> > "Note that due to preallocation, this function *never* fails when called 
> > from
> > process contexts. (it might fail if called from an IRQ context.)"
> > mm/mempool.c:306
> > 
> 
> 
> Will investigate and work on fixing this. 


I think I did a mistake here, qla2xxx_get_qpair_sp() can fail for other
reasons than OOM. My bad, sorry.

> Thanks for these details review of this series and valuable input. 
> 
> I’ll send follow up series shortly. Let me know if this series is okay as is 
> and
> a follow up patches to address concerns by you are okay.

Thanks a lot,
Johannes
-- 
Johannes Thumshirn  Storage

Re: [PATCH v2 3/6] qla2xxx: Add FC-NVMe F/W initialization and transport registration

[Madhani, Himanshu]

Hi Johannes, 

> On Jun 22, 2017, at 2:46 AM, Johannes Thumshirn  wrote:
> 
> On Wed, Jun 21, 2017 at 01:48:43PM -0700, Madhani, Himanshu wrote:
> [...]
>> +wait_queue_head_t nvme_ls_waitQ;
> 
> Can you please lower-case the 'Q' in waitQ IFF you have to re-send the series?

sure.

> 
> [...]
>> +wait_queue_head_t nvme_waitQ;
> 
> Ditto
> 
Ack

> [...]
>> +wait_queue_head_t nvme_waitQ;
> 
> And here as well.

Ack

> 
>> diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
>> index 6fbee11c1a18..c6af45f7d5d6 100644
>> --- a/drivers/scsi/qla2xxx/qla_gbl.h
>> +++ b/drivers/scsi/qla2xxx/qla_gbl.h
>> @@ -10,6 +10,16 @@
>> #include 
>> 
>> /*
>> + * Global functions prototype in qla_nvme.c source file.
>> + */
>> +extern void qla_nvme_register_hba(scsi_qla_host_t *);
>> +extern int  qla_nvme_register_remote(scsi_qla_host_t *, fc_port_t *);
>> +extern void qla_nvme_delete(scsi_qla_host_t *);
>> +extern void qla_nvme_abort(struct qla_hw_data *, srb_t *sp);
>> +extern void qla24xx_nvme_ls4_iocb(scsi_qla_host_t *, struct pt_ls4_request 
>> *,
>> +struct req_que *);
> 
> You're still not convinced of the idea of headers, heh ;-)
> Especially as you have a qla_nvme.h.
> 
> […]
> 

if this is not *must* i’ll like to post patch for this with other patches that 
I am going to queue up during rc1 phase. 

>> +INIT_WORK(>nvme_del_work, qla_nvme_unregister_remote_port);
>> +rport = kzalloc(sizeof(*rport), GFP_KERNEL);
>> +if (!rport) {
>> +ql_log(ql_log_warn, vha, 0x2101,
>> +"%s: unable to alloc memory\n", __func__);
> 
> kzalloc() will warn you about a failed allocation, no need to double it.
> See also:
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> 
> […]

Ack. 

>> +ret = nvme_fc_register_remoteport(vha->nvme_local_port, >req,
>> +>nvme_remote_port);
>> +if (ret) {
>> +ql_log(ql_log_warn, vha, 0x212e,
>> +"Failed to register remote port. Transport returned %d\n",
>> +ret);
>> +return ret;
>> +}
>> +
>> +fcport->nvme_remote_port->private = fcport;
> 
> I think I already said that in the last review, but can you please move the 
> fcport->nvme_remote_port->private = fcport;
> assingment _above_ the nvme_fc_register_remoteport() call.
> 

I saw your response to James that this is okay for FC NVMe code.

> [...]
> 
>> +vha = (struct scsi_qla_host *)lport->private;
> 
> No need to cast from void *
>> +ql_log(ql_log_info, vha, 0x2104,
>> +"%s: handle %p, idx =%d, qsize %d\n",
>> +__func__, handle, qidx, qsize);
> 
> Btw, sometime in the future you could change your ql_log() thingies to the
> kernel's dyndebug facility.
> 
> […]

Thanks for the suggestions. I’ll bring it up to team and we can slowly convert 
these to kernel’s
dynamic debugging facility. 


>> +rval = ha->isp_ops->abort_command(sp);
>> +if (rval != QLA_SUCCESS)
>> +ql_log(ql_log_warn, fcport->vha, 0x2125,
>> +"%s: failed to abort LS command for SP:%p rval=%x\n",
>> +__func__, sp, rval);
>> +
>> +ql_dbg(ql_dbg_io, fcport->vha, 0x212b,
>> +"%s: aborted sp:%p on fcport:%p\n", __func__, sp, fcport);
> 
> If you insinst in having these two messages ("failed to abort" and "aborted")
> can you at least fold it into one print statement.
> 

I’ll send follow up patch for this cleanup, if its okay with you? 

>> +rval = ha->isp_ops->abort_command(sp);
>> +if (!rval)
>> +ql_log(ql_log_warn, fcport->vha, 0x2127,
>> +"%s: failed to abort command for SP:%p rval=%x\n",
>> +__func__, sp, rval);
>> +
>> +ql_dbg(ql_dbg_io, fcport->vha, 0x2126,
>> +"%s: aborted sp:%p on fcport:%p\n", __func__, sp, fcport);
> 
> Ditto.
> 

Agree. Will fold this into cleanup patch. 

> [...]
> 
> 
>> +/* Setup qpair pointers */
>> +req = qpair->req;
>> +tot_dsds = fd->sg_cnt;
>> +
>> +/* Acquire qpair specific lock */
>> +spin_lock_irqsave(>qp_lock, flags);
>> +
>> +/* Check for room in outstanding command list. */
>> +handle = req->current_outstanding_cmd;
> 
> I've just seen this in qla2xxx_start_scsi_mq() and
> qla2xxx_dif_start_scsi_mq() and was about to send you an RFC patch. But
> here it is for completeness in the nvme version as well:
> 
> You save a pointer to the req_que from you qpair and _afterwards_ you grab
> the qp_lock. What prevents someone from changing the request internals
> underneath you?
> 
> Like this:
> 
> CPU0   CPU1
> req = qpair->req;
> qla2xxx_delete_qpair(vha, qpair);
> `-> ret = qla25xx_delete_req_que(vha, 
> qpair->req);
> spin_lock_irqsave(>qp_lock, flags);
> handle = req->current_outstanding_cmd;
> 
> Oh and btw, neither qla2xxx_delete_qpair() nor 

Re: [PATCH v2 3/6] qla2xxx: Add FC-NVMe F/W initialization and transport registration

[Johannes Thumshirn]

On Thu, Jun 22, 2017 at 10:48:46AM -0700, James Smart wrote:
> He can't move it. the fcport->nvme_remote_port pointer is set by the
> nvme_fc_register_remoteport() routine (if return status is 0).

Gah, that's kind of wired. Literly _all_ of the Kernel's register_xxx()
funtions have a semantic that after the registration is done the object can be
used and thus assigning private pointer afterwards is an error. Damn I didn't
realize this in the nmve-fc review.

-- 
Johannes Thumshirn  Storage
jthumsh...@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

Re: [PATCH v2 3/6] qla2xxx: Add FC-NVMe F/W initialization and transport registration

[Johannes Thumshirn]

On Wed, Jun 21, 2017 at 01:48:43PM -0700, Madhani, Himanshu wrote:
[...]
> + wait_queue_head_t nvme_ls_waitQ;

Can you please lower-case the 'Q' in waitQ IFF you have to re-send the series?

[...]
> + wait_queue_head_t nvme_waitQ;

Ditto

[...]
> + wait_queue_head_t nvme_waitQ;

And here as well.

> diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
> index 6fbee11c1a18..c6af45f7d5d6 100644
> --- a/drivers/scsi/qla2xxx/qla_gbl.h
> +++ b/drivers/scsi/qla2xxx/qla_gbl.h
> @@ -10,6 +10,16 @@
>  #include 
>  
>  /*
> + * Global functions prototype in qla_nvme.c source file.
> + */
> +extern void qla_nvme_register_hba(scsi_qla_host_t *);
> +extern int  qla_nvme_register_remote(scsi_qla_host_t *, fc_port_t *);
> +extern void qla_nvme_delete(scsi_qla_host_t *);
> +extern void qla_nvme_abort(struct qla_hw_data *, srb_t *sp);
> +extern void qla24xx_nvme_ls4_iocb(scsi_qla_host_t *, struct pt_ls4_request *,
> +struct req_que *);

You're still not convinced of the idea of headers, heh ;-)
Especially as you have a qla_nvme.h.

[...]

> + INIT_WORK(>nvme_del_work, qla_nvme_unregister_remote_port);
> + rport = kzalloc(sizeof(*rport), GFP_KERNEL);
> + if (!rport) {
> + ql_log(ql_log_warn, vha, 0x2101,
> + "%s: unable to alloc memory\n", __func__);

kzalloc() will warn you about a failed allocation, no need to double it.
See also:
http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

[...]

> + ret = nvme_fc_register_remoteport(vha->nvme_local_port, >req,
> + >nvme_remote_port);
> + if (ret) {
> + ql_log(ql_log_warn, vha, 0x212e,
> + "Failed to register remote port. Transport returned %d\n",
> + ret);
> + return ret;
> + }
> +
> + fcport->nvme_remote_port->private = fcport;

I think I already said that in the last review, but can you please move the 
fcport->nvme_remote_port->private = fcport;
assingment _above_ the nvme_fc_register_remoteport() call.

[...]

> + vha = (struct scsi_qla_host *)lport->private;

No need to cast from void *
> + ql_log(ql_log_info, vha, 0x2104,
> + "%s: handle %p, idx =%d, qsize %d\n",
> + __func__, handle, qidx, qsize);

Btw, sometime in the future you could change your ql_log() thingies to the
kernel's dyndebug facility.

[...]

> + rval = ha->isp_ops->abort_command(sp);
> + if (rval != QLA_SUCCESS)
> + ql_log(ql_log_warn, fcport->vha, 0x2125,
> + "%s: failed to abort LS command for SP:%p rval=%x\n",
> + __func__, sp, rval);
> +
> + ql_dbg(ql_dbg_io, fcport->vha, 0x212b,
> + "%s: aborted sp:%p on fcport:%p\n", __func__, sp, fcport);

If you insinst in having these two messages ("failed to abort" and "aborted")
can you at least fold it into one print statement.

> + rval = ha->isp_ops->abort_command(sp);
> + if (!rval)
> + ql_log(ql_log_warn, fcport->vha, 0x2127,
> + "%s: failed to abort command for SP:%p rval=%x\n",
> + __func__, sp, rval);
> +
> + ql_dbg(ql_dbg_io, fcport->vha, 0x2126,
> + "%s: aborted sp:%p on fcport:%p\n", __func__, sp, fcport);

Ditto.

[...]


> + /* Setup qpair pointers */
> + req = qpair->req;
> + tot_dsds = fd->sg_cnt;
> +
> + /* Acquire qpair specific lock */
> + spin_lock_irqsave(>qp_lock, flags);
> +
> + /* Check for room in outstanding command list. */
> + handle = req->current_outstanding_cmd;

I've just seen this in qla2xxx_start_scsi_mq() and
qla2xxx_dif_start_scsi_mq() and was about to send you an RFC patch. But
here it is for completeness in the nvme version as well:

You save a pointer to the req_que from you qpair and _afterwards_ you grab
the qp_lock. What prevents someone from changing the request internals
underneath you?

Like this:

CPU0   CPU1
req = qpair->req;
 qla2xxx_delete_qpair(vha, qpair);
 `-> ret = qla25xx_delete_req_que(vha, 
qpair->req);
spin_lock_irqsave(>qp_lock, flags);
handle = req->current_outstanding_cmd;

Oh and btw, neither qla2xxx_delete_qpair() nor qla25xx_delete_req_que() grab
the qp_lock.

I think this is something work re-thinking. Maybe you can identify the blocks
accessing struct members which need to be touched under a lock and extract
them into a helper function wich calls lockdep_assert_held(). No must just and
idea.

[...]
> +
> + /* Load data segments */
> + for_each_sg(sgl, sg, tot_dsds, i) {

Do you really need the whole loop under a spin_lock_irqsave()? If the sglist
has a lot of entries (i.e. becasue we couldn't cluster it) we're in risk to
trigger a NMI watchdog soft-lockup WARN_ON(). You need to grab the lock when
accessing req's members but the rest of the loop? This applies to
qla24xx_build_scsi_iocbs() for SCSI as well.

[...]

> + 

Re: [PATCH v2 3/6] qla2xxx: Add FC-NVMe F/W initialization and transport registration

[Hannes Reinecke]

On 06/21/2017 10:48 PM, Madhani, Himanshu wrote:
> From: Duane Grigsby 
> 
> This code provides the interfaces to register remote and local ports
> of FC4 type 0x28 with the FC-NVMe transport and transports the
> requests (FC-NVMe FC link services and FC-NVMe commands IUs) to the
> fabric. It also provides the support for allocating h/w queues and
> aborting FC-NVMe FC requests.
> 
> Signed-off-by: Darren Trapp 
> Signed-off-by: Duane Grigsby 
> Signed-off-by: Anil Gurumurthy 
> Signed-off-by: Giridhar Malavali 
> Signed-off-by: Himanshu Madhani 
> ---
>  drivers/scsi/qla2xxx/Makefile   |   2 +-
>  drivers/scsi/qla2xxx/qla_dbg.c  |   2 +-
>  drivers/scsi/qla2xxx/qla_def.h  |   6 +
>  drivers/scsi/qla2xxx/qla_gbl.h  |  11 +
>  drivers/scsi/qla2xxx/qla_init.c |   8 +
>  drivers/scsi/qla2xxx/qla_iocb.c |  36 ++
>  drivers/scsi/qla2xxx/qla_isr.c  |  19 +
>  drivers/scsi/qla2xxx/qla_mbx.c  |  21 ++
>  drivers/scsi/qla2xxx/qla_nvme.c | 756 
> 
>  drivers/scsi/qla2xxx/qla_nvme.h | 132 +++
>  drivers/scsi/qla2xxx/qla_os.c   |  40 ++-
>  11 files changed, 1024 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/scsi/qla2xxx/qla_nvme.c
>  create mode 100644 drivers/scsi/qla2xxx/qla_nvme.h
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)