[openib-general] [PATCH] IB/SRP Userspace: srptools/srp_daemon - Fix connect bug and add support for user specified initiator extension

[Lakshmanan, Madhu]

> From: Ishai Rabinovitz [mailto:ishai at dev.mellanox.co.il] 
> Subject: Re: [openib-general] [PATCH] IB/SRP Userspace: 
> srptools/srp_daemon - Fix connect bug and add support for 
> user specified initiator extension
> 
> Thanks for your patch.
> 
> I agree with some of the changes you suggest and disagree 
> with others. It
> will be useful to post a different patch for each logical change.
> 

Thanks for the comments. I will make sure to separate out the logical
changes into discrete patches the next time I submit a patch.

> > 1. Fixes bug in srp_daemon for the case where if it is 
> > invoked with the '-e' option, it fails to connect to the 
> > SRP targets because of a newline character in the parameter string.
> 
> You are right that the '\n' is redundant, but I have not seen 
> the bug it
> creates. The last parameter in the string is considered to be 
> a string by
> ib_srp and therefore ib_srp will ignore the newline.
>

I saw the following error message in /var/log/messages before I fixed
the newline:

messages:Oct 17 06:14:25 aspen kernel: ib_srp: unknown parameter or
missing valu
e 'io_class=ff00
messages:Oct 17 06:14:25 aspen kernel: ib_srp: unknown parameter or
missing valu
e 'io_class=ff00
messages:Oct 18 05:43:42 aspen kernel: ib_srp: unknown parameter or
missing valu
e 'io_class=ff00

Do you suspect the problem to be elsewhere? I was testing against
Silverstorm SRP targets, but considering the error message, that should
not have been relevant. The connection, of course, never got created,
which is what prompted me to make the above fix.
 
> 
> > 2. Changes the name of the constant 
> 'MAX_TRAGET_CONFIG_STR_STRING' to
> 'MAX_TARGET_CONFIG_STR_STRING'.
> 
> Thanks, I will apply this change.
> 

Didn't really mean to nitpick on this typo. I decided to fix it only
when I grep'ed for TARGET and found no matches.

> 
> > 3. Changes the behavior of the '-n' option to srp_daemon. 
> > The earlier behavior printed the initiator extension. The 
> > new behavior allows the user to specify an initiator extension 
> > as an argument to the '-n' option.
> 
> I do not think we want to change the -n behavior to this one.
> First of all your approach induces the same initiator_ext to all the
> targets discovered by this srp_daemon.

The point you raise actually prompts another related question about
srp_daemon behavior. Currently, srp_daemon connects to all the targets
it finds, when given the '-e' option. I think adding a flag that would
allow a user to specify the target IOC guid to connect to would help,
and that flag when used with the initiator extension would be more
useful. What do you think?

> Secondly If someone uses random values for the initiator_ext 
> it may cause a waste of resources in the target: the target can never 
> tell when a connection had failed (or an initiator performed a boot)
> and will keep the connection alive. When there is an attempt to
reconnect 
> to this target with the same initiator_ext, the target knows he can 
> close the old connection.
> This is the reason we decided to have a convention. The convention is
to
> use the target port guid. The advantage of this convention is that it
> allows us to have two connection on the same time from an initiator to
> both ports of the target.
> 
> I understand that some targets may need a different 
> initiator_ext, 

Yes, Silverstorm SRP targets support multiple connections from a single
HCA port to a single SRP target, for the purposes of establishing unique
connections to specific storage devices that are *behind* an FC switch
but all being accessible through the same target port. In summary, to
fully support such SRP targets and the increased functionality that
becomes possible because of them, supporting multiple initiator
extension becomes a necessity.

> but you should add a new flag for actually setting the 
> initiator_ext and leave -n untouched.
> You are welcome to send such a patch.

I agree, I will leave '-n' untouched and add a new flag, say '-x', that
will allow a user to specify the initiator extension. 

> 
> Ishai

There is another issue that we need to tackle as well. Currently, the
check in srp_daemon, to see whether a target is already connected, does
not take into account the initiator extension that a user could specify.
That would need to be added to the check as well. From a quick
examination of the code, it appears that the changes are both to the
kernel module as well as to srp_daemon. I hope to have a separate patch
for that ready in a few days.

Thanks, appreciate your comments and feedback, Madhu.