[GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Nicholas A. Bellinger]

Hi Linus,

This is the PULL request for an initial merge of the ib_srpt driver
using mainline target infrastructure into v3.2-rc1.  Please go ahead and
pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next-merge

Note that 'target: Updates for v3.2-rc1 (round two)' should be pulled
first via target-pending.git/for-next, and has gone out in a separate
PULL request. 

At this point should we should have a stable userspace API for the
layout of /sys/kernel/config/target/srpt/, and a number of legacy module
parameters have been removed or converted into per endpoint attributes
as per Roland's feedback.  This code has been getting build testing in
next the past week, and does not touch any external code.

There is an known issue with active I/O shutdown is currently being
addressed using the new generic active I/O shutdown bits in for-next,
and will be sent out post merge after more testing and feedback.  I also
see a few patches from DanC this morning to address a handful of non
critical issues that will be included post merge as well.

Here are full set of review changes from Roland and Bart that have been
made:

v1 -> v2 review changes:

ib_srpt: Fix potential out-of-bounds array access
ib_srpt: Avoid failed multipart RDMA transfers
ib_srpt: Fix srpt_alloc_fabric_acl failure case return value
ib_srpt: Update comments to reference $driver/$port layout
ib_srpt: Fix sport->port_guid formatting code
ib_srpt: Remove legacy use_port_guid_in_session_name module parameter
ib_srpt: Convert srp_max_rdma_size into per port configfs attribute
ib_srpt: Convert srp_max_rsp_size into per port configfs attribute
ib_srpt: Convert srpt_sq_size into per port configfs attribute

v2 -> v3 review changes:

ib_srpt: Fix possible race with srp_sq_size in srpt_create_ch_ib
ib_srpt: Fix possible race with srp_max_rsp_size in srpt_release_channel_work
ib_srpt: Fix up MAX_SRPT_RDMA_SIZE define
ib_srpt: Make srpt_map_sg_to_ib_sge() failure case return -EAGAIN
ib_srpt: Convert port_guid to use subnet_prefix + interface_id formatting
ib_srpt: Make srpt_check_stop_free return kref_put status

Thanks!

--nab

Bart Van Assche (1):
  ib_srpt: Initial SRP Target merge for v3.2-rc1

 drivers/infiniband/Kconfig              |    1 +
 drivers/infiniband/Makefile             |    1 +
 drivers/infiniband/ulp/srpt/Kconfig     |   12 +
 drivers/infiniband/ulp/srpt/Makefile    |    2 +
 drivers/infiniband/ulp/srpt/ib_dm_mad.h |  139 ++
 drivers/infiniband/ulp/srpt/ib_srpt.c   | 4081 +++++++++++++++++++++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.h   |  444 ++++
 7 files changed, 4680 insertions(+), 0 deletions(-)
 create mode 100644 drivers/infiniband/ulp/srpt/Kconfig
 create mode 100644 drivers/infiniband/ulp/srpt/Makefile
 create mode 100644 drivers/infiniband/ulp/srpt/ib_dm_mad.h
 create mode 100644 drivers/infiniband/ulp/srpt/ib_srpt.c
 create mode 100644 drivers/infiniband/ulp/srpt/ib_srpt.h

Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Roland Dreier]

On Fri, Nov 4, 2011 at 1:10 PM, Nicholas A. Bellinger
<nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> At this point should we should have a stable userspace API for the
> layout of /sys/kernel/config/target/srpt/, and a number of legacy module
> parameters have been removed or converted into per endpoint attributes
> as per Roland's feedback.  This code has been getting build testing in
> next the past week, and does not touch any external code.

Given the confidence that the userspace ABI is stable,

Acked-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

 - 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: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Bart Van Assche]

On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> This is the PULL request for an initial merge of the ib_srpt driver
> using mainline target infrastructure into v3.2-rc1.

In case anyone is interested, the most important unaddressed comments
for this version of ib_srpt are:
- There are still too many module parameters. This makes ib_srpt
harder to use than necessary because several of these parameters can
only be set at module load time.
- The last WQE event can arrive before the queue pair is reset,
resulting in a hanging session and blocking future logins
(http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09678.html).
- The "ib_srpt: Convert srp_max_rdma_size into per port configfs
attribute" contradicts the I/O controller concept. This is a bug.
(http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09677.html).

Bart.

Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Nicholas A. Bellinger]

On Sat, 2011-11-05 at 08:37 +0100, Bart Van Assche wrote:
> On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> > This is the PULL request for an initial merge of the ib_srpt driver
> > using mainline target infrastructure into v3.2-rc1.
> 
> In case anyone is interested, the most important unaddressed comments
> for this version of ib_srpt are:
> - There are still too many module parameters. This makes ib_srpt
> harder to use than necessary because several of these parameters can
> only be set at module load time.

Can you be more specific here..?  I went through the module parameters
and made the ones that where used in a per-port specific context into
configfs attributes as Roland recommended, but it seems like you are
saying that more should be made into attributes.  Which ones..?

> - The last WQE event can arrive before the queue pair is reset,
> resulting in a hanging session and blocking future logins
> (http://www.mail-archive.com/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg09678.html).

Yep, was going to get this resolved post merge along with my patch now
in lio-core.git to address active I/O shutdown with ib_srpt.

> - The "ib_srpt: Convert srp_max_rdma_size into per port configfs
> attribute" contradicts the I/O controller concept. This is a bug.
> (http://www.mail-archive.com/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg09677.html).
> 

As mentioned in the thread, I deferred to Roland's input on that one.
So you're saying this should be made back into a module parameter now,
or what..?

--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: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Bart Van Assche]

On Sat, Nov 5, 2011 at 7:04 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> On Sat, 2011-11-05 at 08:37 +0100, Bart Van Assche wrote:
>> On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
>> <nab@linux-iscsi.org> wrote:
>> > This is the PULL request for an initial merge of the ib_srpt driver
>> > using mainline target infrastructure into v3.2-rc1.
>>
>> In case anyone is interested, the most important unaddressed comments
>> for this version of ib_srpt are:
>> - There are still too many module parameters. This makes ib_srpt
>> harder to use than necessary because several of these parameters can
>> only be set at module load time.
>
> Can you be more specific here..?  I went through the module parameters
> and made the ones that where used in a per-port specific context into
> configfs attributes as Roland recommended, but it seems like you are
> saying that more should be made into attributes.  Which ones..?

Several read-only kernel module parameters remain. That's fine in
out-of-tree code since it will be compiled as a kernel module only.
But anyone who tries to build with CONFIG_INFINIBAND_SRPT=y will
notice that it's impossible to change the value of these read-only
kernel module parameters.

>> - The last WQE event can arrive before the queue pair is reset,
>> resulting in a hanging session and blocking future logins
>> (http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09678.html).
>
> Yep, was going to get this resolved post merge along with my patch now
> in lio-core.git to address active I/O shutdown with ib_srpt.

Your "active I/O shutdown" patch does not address the "last WQE" issue.

>> - The "ib_srpt: Convert srp_max_rdma_size into per port configfs
>> attribute" contradicts the I/O controller concept. This is a bug.
>> (http://www.mail-archive.com/linux-rdma@vger.kernel.org/msg09677.html).
>
> As mentioned in the thread, I deferred to Roland's input on that one.
> So you're saying this should be made back into a module parameter now,
> or what..?

I see two possible ways forward:
- Convert the per-port max_rdma_size configfs variable into a
per-module configfs variable.
- Remove the max_rdma_size variable from configfs entirely and compute
the max_rdma_size from the HCA properties in struct ib_device_attr.

Bart.

Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Nicholas A. Bellinger]

On Sun, 2011-11-06 at 08:34 +0100, Bart Van Assche wrote:
> On Sat, Nov 5, 2011 at 7:04 PM, Nicholas A. Bellinger
> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> > On Sat, 2011-11-05 at 08:37 +0100, Bart Van Assche wrote:
> >> On Fri, Nov 4, 2011 at 9:10 PM, Nicholas A. Bellinger
> >> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> >> > This is the PULL request for an initial merge of the ib_srpt driver
> >> > using mainline target infrastructure into v3.2-rc1.
> >>
> >> In case anyone is interested, the most important unaddressed comments
> >> for this version of ib_srpt are:
> >> - There are still too many module parameters. This makes ib_srpt
> >> harder to use than necessary because several of these parameters can
> >> only be set at module load time.
> >
> > Can you be more specific here..?  I went through the module parameters
> > and made the ones that where used in a per-port specific context into
> > configfs attributes as Roland recommended, but it seems like you are
> > saying that more should be made into attributes.  Which ones..?
> 
> Several read-only kernel module parameters remain. That's fine in
> out-of-tree code since it will be compiled as a kernel module only.
> But anyone who tries to build with CONFIG_INFINIBAND_SRPT=y will
> notice that it's impossible to change the value of these read-only
> kernel module parameters.
> 

So after the cleanups, srp_max_req_size, srpt_srq_size and
srpt_service_guid appear as the remaining module parameters.  The first
two are required at srpt_add_one() -> srpt_device allocation time
(usually at initial load time), and should remain as global scope
parameters correct..?  To address the point with
CONFIG_INFINIBAND_SRPT=y, how about assigning these two values for each
srpt_device during srpt_add_one(), and reference them directly in
srpt_device so their respective module parameter can become S_IRUGO|
S_IWUSR...?

We have also discussed srpt_service_guid a bit before, which you
indicated needed to stay in current code as global scope, and presumably
should not change value after loading.  Looking at the actual usage, one
post merge improvement we can consider is seeing if it's possible to
move ib_cm_listen() out of srpt_add_one() and have it driven instead by
configfs context in order to optionally set srpt_service_guid on a per
target endpoint basis to get us some more flexibility. 

> >> - The last WQE event can arrive before the queue pair is reset,
> >> resulting in a hanging session and blocking future logins
> >> (http://www.mail-archive.com/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg09678.html).
> >
> > Yep, was going to get this resolved post merge along with my patch now
> > in lio-core.git to address active I/O shutdown with ib_srpt.
> 
> Your "active I/O shutdown" patch does not address the "last WQE" issue.

The current patch in lio-core allows ib_srpt to push active I/O and
perform shutdown + unload without hanging (or crashing) at this point.
Active I/O tests with physical link layer failures also seems to work,
but mixing to two together still obviously needs more testing.

For the "last WQE" you've mentioned, I'm happy to accept a patch to
address this as you know the code better than me.  Otherwise, I'll be
digging this out of your out-of-tree code as a seperate item and
figuring out how to reproduce and test this special case.

> 
> >> - The "ib_srpt: Convert srp_max_rdma_size into per port configfs
> >> attribute" contradicts the I/O controller concept. This is a bug.
> >> (http://www.mail-archive.com/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg09677.html).
> >
> > As mentioned in the thread, I deferred to Roland's input on that one.
> > So you're saying this should be made back into a module parameter now,
> > or what..?
> 
> I see two possible ways forward:
> - Convert the per-port max_rdma_size configfs variable into a
> per-module configfs variable.

We don't need to add any per-module configfs target variables, we
already have /sys/module/$NAME/parameters/ for that.  ;)

> - Remove the max_rdma_size variable from configfs entirely and compute
> the max_rdma_size from the HCA properties in struct ib_device_attr.
> 

What did you have in mind..?  This should be based on ->max_sge *
PAGE_SIZE, or something else from ib_device_attr..?  

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: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Bart Van Assche]

On Sun, Nov 6, 2011 at 11:09 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> We have also discussed srpt_service_guid a bit before, which you
> indicated needed to stay in current code as global scope, and presumably
> should not change value after loading.  Looking at the actual usage, one
> post merge improvement we can consider is seeing if it's possible to
> move ib_cm_listen() out of srpt_add_one() and have it driven instead by
> configfs context in order to optionally set srpt_service_guid on a per
> target endpoint basis to get us some more flexibility.

ib_sprt defines a single I/O controller profile and hence there should
be exactly one GUID associated with it.

> For the "last WQE" you've mentioned, I'm happy to accept a patch to
> address this as you know the code better than me.  Otherwise, I'll be
> digging this out of your out-of-tree code as a seperate item and
> figuring out how to reproduce and test this special case.

I'll post a patch to address last WQE handling.

Bart.

Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Bart Van Assche]

On Sun, Nov 6, 2011 at 2:20 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Sun, Nov 6, 2011 at 11:09 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
>> We have also discussed srpt_service_guid a bit before, which you
>> indicated needed to stay in current code as global scope, and presumably
>> should not change value after loading.  Looking at the actual usage, one
>> post merge improvement we can consider is seeing if it's possible to
>> move ib_cm_listen() out of srpt_add_one() and have it driven instead by
>> configfs context in order to optionally set srpt_service_guid on a per
>> target endpoint basis to get us some more flexibility.
>
> ib_sprt defines a single I/O controller profile and hence there should
> be exactly one GUID associated with it.

(replying to my own e-mail)

Note: I'm not sure anyone is using the srpt_service_guid kernel module
parameter. One possible approach is to leave out that kernel module
parameter for the initial merge. If later on anyone asks for restoring
that functionality we can evaluate at that time what would be the best
way to restore it.

Bart.

Re: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Nicholas A. Bellinger]

On Sun, 2011-11-06 at 14:20 +0100, Bart Van Assche wrote:
> On Sun, Nov 6, 2011 at 11:09 AM, Nicholas A. Bellinger
> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> > We have also discussed srpt_service_guid a bit before, which you
> > indicated needed to stay in current code as global scope, and presumably
> > should not change value after loading.  Looking at the actual usage, one
> > post merge improvement we can consider is seeing if it's possible to
> > move ib_cm_listen() out of srpt_add_one() and have it driven instead by
> > configfs context in order to optionally set srpt_service_guid on a per
> > target endpoint basis to get us some more flexibility.
> 
> ib_sprt defines a single I/O controller profile and hence there should
> be exactly one GUID associated with it.

Ok, so if it exists, it needs to stay global scope.

> 
> > For the "last WQE" you've mentioned, I'm happy to accept a patch to
> > address this as you know the code better than me.  Otherwise, I'll be
> > digging this out of your out-of-tree code as a seperate item and
> > figuring out how to reproduce and test this special case.
> 
> I'll post a patch to address last WQE handling.
> 

Thank you for handling this one.  I've merged your patch into lio-core
and will get this included into target-pending with next set of
post-merge rc-fixes.

Thanks Bart!

--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: [GIT PULL] ib_srpt: Initial SRP Target merge for v3.2-rc1

[Nicholas A. Bellinger]

On Sun, 2011-11-06 at 18:42 +0100, Bart Van Assche wrote:
> On Sun, Nov 6, 2011 at 2:20 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> > On Sun, Nov 6, 2011 at 11:09 AM, Nicholas A. Bellinger
> > <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> >> We have also discussed srpt_service_guid a bit before, which you
> >> indicated needed to stay in current code as global scope, and presumably
> >> should not change value after loading.  Looking at the actual usage, one
> >> post merge improvement we can consider is seeing if it's possible to
> >> move ib_cm_listen() out of srpt_add_one() and have it driven instead by
> >> configfs context in order to optionally set srpt_service_guid on a per
> >> target endpoint basis to get us some more flexibility.
> >
> > ib_sprt defines a single I/O controller profile and hence there should
> > be exactly one GUID associated with it.
> 
> (replying to my own e-mail)
> 
> Note: I'm not sure anyone is using the srpt_service_guid kernel module
> parameter. One possible approach is to leave out that kernel module
> parameter for the initial merge. If later on anyone asks for restoring
> that functionality we can evaluate at that time what would be the best
> way to restore it.
> 

Well, when this was discussed this before we concluded (via your input)
that this should stay in place for the initial merge.

So it's a bit late to drop this now, as the PULL request has gone out to
Linus, and has been acked by Roland.  But if you think it should
removed, I'm happy to drop it in the first set of post-merge
target-pending rc-fixes.

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