ib_srpt status

ib_srpt status

[Christoph Hellwig]

Hi Roland, hi Nick,

what is the status of queueing up ib_srpt for mainline?  It's a very
clean looking driver that was submitted 8 month ago and after an initial
round of fixes only saw updates for changing core APIs.

Keeping it out of tree for so long is a very discuraging story for
contributors.

Re: ib_srpt status

[Christoph Hellwig]

[addressing to a working address for Roland]

On Wed, Oct 12, 2011 at 11:23:18AM +0200, Christoph Hellwig wrote:
> Hi Roland, hi Nick,
> 
> what is the status of queueing up ib_srpt for mainline?  It's a very
> clean looking driver that was submitted 8 month ago and after an initial
> round of fixes only saw updates for changing core APIs.
> 
> Keeping it out of tree for so long is a very discuraging story for
> contributors.
---end quoted text---

Re: ib_srpt status

[Roland Dreier]

On Wed, Oct 12, 2011 at 2:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> Hi Roland, hi Nick,
>
> what is the status of queueing up ib_srpt for mainline?  It's a very
> clean looking driver that was submitted 8 month ago and after an initial
> round of fixes only saw updates for changing core APIs.
>
> Keeping it out of tree for so long is a very discuraging story for
> contributors.

Where is the latest patch?  I would like to review it and at least
get it into linux-next...

I seem to recall having some concerns about the interface -- ie
some things were done through module params and I don't want
to be stuck supporting those module params forever.

Other than that it is likely to be fine to merge.

 - R.

Re: ib_srpt status

[Nicholas A. Bellinger]

On Wed, 2011-10-12 at 09:17 -0700, Roland Dreier wrote:
> On Wed, Oct 12, 2011 at 2:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> > Hi Roland, hi Nick,
> >
> > what is the status of queueing up ib_srpt for mainline?  It's a very
> > clean looking driver that was submitted 8 month ago and after an initial
> > round of fixes only saw updates for changing core APIs.
> >
> > Keeping it out of tree for so long is a very discuraging story for
> > contributors.
> 
> Where is the latest patch?  I would like to review it and at least
> get it into linux-next...
> 
> I seem to recall having some concerns about the interface -- ie
> some things were done through module params and I don't want
> to be stuck supporting those module params forever.
> 
> Other than that it is likely to be fine to merge.
> 

Hey guys,

I'll post the latest version of ib_srpt in a bit after merging
Christoph's remaining patches..  However, this will be against the v3.2
queued patches in target-pending.git/for-next, as the core cleanups in
the se_cmd release path also effect current ib_srpt code.

--nab

Re: ib_srpt status

[Bart Van Assche]

On Wed, Oct 12, 2011 at 11:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> what is the status of queueing up ib_srpt for mainline?  It's a very
> clean looking driver that was submitted 8 month ago and after an initial
> round of fixes only saw updates for changing core APIs.

The ib_srpt driver was working fine last time I tested it. But with
the current (3.1-rc9) LIO core performing I/O triggers grave memory
corruption and random kernel crashes. Something must have changed in
the LIO core that broke ib_srpt.

Bart.

Re: ib_srpt status

[Nicholas A. Bellinger]

On Tue, 2011-10-18 at 08:13 +0200, Bart Van Assche wrote:
> On Wed, Oct 12, 2011 at 11:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> > what is the status of queueing up ib_srpt for mainline?  It's a very
> > clean looking driver that was submitted 8 month ago and after an initial
> > round of fixes only saw updates for changing core APIs.
> 
> The ib_srpt driver was working fine last time I tested it. But with
> the current (3.1-rc9) LIO core performing I/O triggers grave memory
> corruption and random kernel crashes. Something must have changed in
> the LIO core that broke ib_srpt.
> 

Hi Bart,

I'm having no problems with the patch posted for v3.2 review on MT26428
HCAs

Can you verify you are actually using this patch, and not your private
tree from github..?  The reason is that your tree did not contain the
bugfix below to ib_srpt code that was originally causing problems:

ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Thanks,

--nab

Re: ib_srpt status

[Christoph Hellwig]

On Tue, Oct 18, 2011 at 09:41:15AM -0700, Nicholas A. Bellinger wrote:
> I'm having no problems with the patch posted for v3.2 review on MT26428
> HCAs
> 
> Can you verify you are actually using this patch, and not your private
> tree from github..?  The reason is that your tree did not contain the
> bugfix below to ib_srpt code that was originally causing problems:
> 
> ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
> http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Also it would be useful to know the workload that causes the corruption.

Re: ib_srpt status

[Nicholas A. Bellinger]

On Tue, 2011-10-18 at 18:43 +0200, Christoph Hellwig wrote:
> On Tue, Oct 18, 2011 at 09:41:15AM -0700, Nicholas A. Bellinger wrote:
> > I'm having no problems with the patch posted for v3.2 review on MT26428
> > HCAs
> > 
> > Can you verify you are actually using this patch, and not your private
> > tree from github..?  The reason is that your tree did not contain the
> > bugfix below to ib_srpt code that was originally causing problems:
> > 
> > ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
> > http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252
> 
> Also it would be useful to know the workload that causes the corruption.

>From the above commit:

This patch fixes a bug in srpt_map_sg_to_ib_sge() that assumed a single
contigious array of struct scatterlist.  This was causing a OOPs when
se_cmd->t_task->t_tasks_no > 1 was using a set of sg_chained struct
se_task->task_sg[] memory into srpt_map_sg_to_ib_sge() code.

Re: ib_srpt status

[Bart Van Assche]

On Tue, Oct 18, 2011 at 6:41 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> I'm having no problems with the patch posted for v3.2 review on MT26428
> HCAs
>
> Can you verify you are actually using this patch, and not your private
> tree from github..?  The reason is that your tree did not contain the
> bugfix below to ib_srpt code that was originally causing problems:
>
> ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
> http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252

Yes, that patch was included - sorry, but I had made another mistake.
3.1-rc10 + ib_srpt + patch below is now working fine here. I noticed
only one unusual issue so far (with kernel debugging enabled) and that
is that the watchdog thread CPU time was unusually high:

root         7  2.3  0.0      0     0 ?        S    18:39   0:22 [watchdog/0]
root        12  0.6  0.0      0     0 ?        S    18:39   0:06 [watchdog/1]


The patch I used to backport ib_srpt from LIO-4.1 to kernel 3.1-rc10 is:

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 109b412..b3947fe 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1340,7 +1340,7 @@ static void srpt_put_send_ioctx(struct
srpt_send_ioctx *ioctx)
        WARN_ON(srpt_get_cmd_state(ioctx) != SRPT_STATE_DONE);

        srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
-       transport_generic_free_cmd(&ioctx->cmd, 0);
+       transport_generic_free_cmd(&ioctx->cmd, 0, 0);

        if (ioctx->n_rbuf > 1) {
                kfree(ioctx->rbufs);
@@ -1899,7 +1899,7 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
                        TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
                goto process_tmr;
        }
-       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
+       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr);
        if (!cmd->se_tmr_req) {
                send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
                send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;

Bart.

Re: ib_srpt status

[Nicholas A. Bellinger]

On Wed, 2011-10-19 at 18:59 +0200, Bart Van Assche wrote:
> On Tue, Oct 18, 2011 at 6:41 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > I'm having no problems with the patch posted for v3.2 review on MT26428
> > HCAs
> >
> > Can you verify you are actually using this patch, and not your private
> > tree from github..?  The reason is that your tree did not contain the
> > bugfix below to ib_srpt code that was originally causing problems:
> >
> > ib_srpt: Fix bug with chainged SGLs in srpt_map_sg_to_ib_sge
> > http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=commitdiff;h=ea485147563b6555a97dbf811825fbb586519252
> 
> Yes, that patch was included - sorry, but I had made another mistake.

Thanks for the update here.

> 3.1-rc10 + ib_srpt + patch below is now working fine here. I noticed
> only one unusual issue so far (with kernel debugging enabled) and that
> is that the watchdog thread CPU time was unusually high:
> 
> root         7  2.3  0.0      0     0 ?        S    18:39   0:22 [watchdog/0]
> root        12  0.6  0.0      0     0 ?        S    18:39   0:06 [watchdog/1]
> 
> 

I'm not exactly sure what to make of this..

> The patch I used to backport ib_srpt from LIO-4.1 to kernel 3.1-rc10 is:
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
> b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 109b412..b3947fe 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1340,7 +1340,7 @@ static void srpt_put_send_ioctx(struct
> srpt_send_ioctx *ioctx)
>         WARN_ON(srpt_get_cmd_state(ioctx) != SRPT_STATE_DONE);
> 
>         srpt_unmap_sg_to_ib_sge(ioctx->ch, ioctx);
> -       transport_generic_free_cmd(&ioctx->cmd, 0);
> +       transport_generic_free_cmd(&ioctx->cmd, 0, 0);
> 
>         if (ioctx->n_rbuf > 1) {
>                 kfree(ioctx->rbufs);
> @@ -1899,7 +1899,7 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
>                         TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
>                 goto process_tmr;
>         }
> -       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
> +       cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr);
>         if (!cmd->se_tmr_req) {
>                 send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
>                 send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
> 

Yep, the ib_srpt patch I sent is against target-pending.git/for-next
along with the other target-core updates for v3.2 that will be merged
first.

Btw, Roland asked me to take a look at the current set of module
parameters for ib_srpt, and see which could be removed and/or converted
to per SPR target endpoint configfs attributes..  So on that note, a few
questions for you on a couple of the module parameters:

*) use_port_guid_in_session_name: This appears to be a legacy compat 
   item, can this be safetly removed for mainline..?

*) srpt_service_guid: Should this really have global scope..?

Aside from those, i'm currently having a look to see which of the module
parameters are not used in the initial srpt_add_one() setup path, and
that using per SRP target endpoint configfs attributes would make sense.

Any input here would be appreciated.

Thanks,

--nab