> From: Ishai Rabinovitz [mailto:[EMAIL PROTECTED] > 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. _______________________________________________ openib-general mailing list [email protected] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
