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

Re: ib_srpt status

[Bart Van Assche]

On Wed, Oct 19, 2011 at 7:38 PM, Nicholas A. Bellinger
<nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> 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..?

As far as I know some people are still using this parameter.

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

I'm afraid that having different values of this parameter for
different HCAs would break failing over between HCAs.

> 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.

My opinion is that the parameters used in srpt_add_one() also should
be converted from module parameters into something that is
configurable dynamically. ib_srpt users have already been asking to
make e.g. srp_max_req_size configurable dynamically. It should be
doable to reallocate the receive ring when there are no active
sessions.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ib_srpt status

[Nicholas A. Bellinger]

On Wed, 2011-10-19 at 20:09 +0200, Bart Van Assche wrote:
> On Wed, Oct 19, 2011 at 7:38 PM, Nicholas A. Bellinger
> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> > 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..?
> 
> As far as I know some people are still using this parameter.

Sure, but I think the question is if it's worth getting rid of for
mainline if it's something that the mainline code will not be using.

AFAICT it's still something we will never end up using with mainline
target infrastructure, right..?

> 
> > *) srpt_service_guid: Should this really have global scope..?
> 
> I'm afraid that having different values of this parameter for
> different HCAs would break failing over between HCAs.
> 

Ok, leaving srpt_service_guid as a single global module parameter.

> > 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.
> 
> My opinion is that the parameters used in srpt_add_one() also should
> be converted from module parameters into something that is
> configurable dynamically.

Sure, but the question is: Where can these parameters be set dynamically
if srpt_add_one() is called directly from modprobe..?

> ib_srpt users have already been asking to
> make e.g. srp_max_req_size configurable dynamically. It should be
> doable to reallocate the receive ring when there are no active
> sessions.
> 

Mmmm, I think I understand what you mean here, but will need to dig into
the code some more to make sure.  I'll try moving as much of these as
possible into per SRP target endpoint attributes, and will let you know
if I get stuck on any specific parameter.

Thanks,

--nab 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ib_srpt status

[Roland Dreier]

On Wed, Oct 19, 2011 at 11:09 AM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
>> *) use_port_guid_in_session_name: This appears to be a legacy compat
>>   item, can this be safetly removed for mainline..?
>
> As far as I know some people are still using this parameter.

So what exactly does this parameter do?  How do I as a user
know what to set this to?

For mainline can we just decide what the right value of this is
and get rid of the parameter?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ib_srpt status

[Roland Dreier]

On Wed, Oct 19, 2011 at 11:42 AM, Nicholas A. Bellinger
<nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
>> > *) srpt_service_guid: Should this really have global scope..?
>>
>> I'm afraid that having different values of this parameter for
>> different HCAs would break failing over between HCAs.

If I have the possibility to set this per-whatever, I can of course
still choose to set it to the same value everywhere, if I care about
failover (vs. maybe providing SRP service to two different contexts).

How do I choose what value to set this to anyway?

Can we at least put this in configfs with the rest of the target
config options?  It seems like a hassle fo rme as a user to have
to set a module parameter to control this type of config.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ib_srpt status

[Bart Van Assche]

On Thu, Oct 20, 2011 at 6:51 PM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
> On Wed, Oct 19, 2011 at 11:09 AM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
>>> *) use_port_guid_in_session_name: This appears to be a legacy compat
>>>   item, can this be safetly removed for mainline..?
>>
>> As far as I know some people are still using this parameter.
>
> So what exactly does this parameter do?  How do I as a user
> know what to set this to?
>
> For mainline can we just decide what the right value of this is
> and get rid of the parameter?

If this parameter would be removed I would prefer to use the value 0
(false) - this is the only value that is compliant with the SRP r16a
draft.

If anyone is still using this parameter, I think it is a good time to speak up.

The most recent discussion I have been able to find about this subject
is: http://sourceforge.net/mailarchive/message.php?msg_id=23481074

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html