On Wed, 2011-05-04 at 22:04 -0700, Mike Christie wrote: > On 05/04/2011 07:42 AM, Vikas Chaudhary wrote: > >> -----Original Message----- > >> From: Eddie Wai [mailto:eddie.wai@xxxxxxxxxxxx] > >> Sent: Monday, May 02, 2011 5:19 AM > >> To: open-iscsi@xxxxxxxxxxxxxxxx > >> Cc: James.Bottomley@xxxxxxx; michaelc@xxxxxxxxxxx; linux- > >> scsi@xxxxxxxxxxxxxxx; Vikas Chaudhary; Lalit Chandivade; Ravi Anand; Harish > >> Zunjarrao > >> Subject: Re: [RFC-V2 PATCH] iscsiadm: Add netconfig support in iscsiadm and > >> iscsid > >> > >> Hello Vikas, > >> > >> The patch looks good but I have a few questions/comments in relation to > >> the general scheme of setting these iface net parameters: > >> > >> 1. As far as I understand, each iface file of the same hw must now > >> incorporate an unique iface_num which will be used to key off the > >> corresponding iface settings in the transport. Iscsiadm will send a > >> UEVENT along with the net contents to the transport only when it is > >> explicitly asked to apply the update via the command line. Perhaps it > >> would be beneficial to have the initiator apply the changes implicitly > >> like how it is currently making the call to the > >> iscsi_host_set_net_params just prior to all the ep_connect calls. This > >> will allow the current iface parameters to be set for the corresponding > >> ep_connect call. > > > > Yes, it makes sense. We can add flag similar to "t->template->set_host_ip" > > and allow implicit network configuration for each ep_connect. > > > > Mike C, do you see any issues? > > No. > > And for the iface_num, I was also think working on just making the iface > number more of a string or something. Userspace could then just pass > down any value it wanted, so drivers like bnx2i/cxgbi/be2iscsi would not > have to worry about the value. It could just be the userpace iface name > since that is unique, and it would allow us to match the iface to the > kernel object. Still working on it though. I have not completely > convince myself. Do you guys have any issues with it or any comments? > I think it would work as long as the handle passed to the transport is unique. > > > > > > However, when we apply network settings for qla4xxx, it will do logout and > > login all devices, so to avoid all devices getting logout and login for each > > ep_connect call we do not want to apply network settings for each ep_connect. > > > >> > >> 2. In the qla4xxx code changes, it looks like each Scsi_Host is only > >> allowing 1 IPv4 and 2 IPv6 addresses to the applied to the HBA. This is > >> conceivable as each interface can only assume that number of addresses. > >> But in the case of VLANs, how do you handle the case when the VLANs > >> assume different IP addresses and subnets when they are based off of the > >> same HBA? > >> > >> This can be resolved easily if we have a direct reference to the iface > >> parameters upon the ep_connect call. Perhaps embed this info (or > >> iface_num) to the Scsi_Host struct? Or just do as proposed in (1). > >> > >> 3. Its probably a good idea to bump the ISCSI_UEVENT_MAX #define since > >> the netlink.c in the initiator uses it to error out undefined events. > >>> ISCSI_UEVENT_PATH_UPDATE = UEVENT_BASE + 20, > >>> + ISCSI_UEVENT_SET_NET_CONFIG = UEVENT_BASE + 21, > >>> > >>> ISCSI_UEVENT_MAX = ISCSI_UEVENT_PATH_UPDATE, > >> + ISCSI_UEVENT_MAX = ISCSI_UEVENT_SET_NET_CONFIG, > >>> > >> > > > > Yes, we will fix this. > > I will fix that up for you. > > > > >> 4. Although the newly defined "set_net_config" to the iscsi_transport > >> structure in the driver is a new augmentation, its interesting that the > >> iscsi_transport_template in the initiator already has a "set_net_config" > >> entry which is currently used for uip_broadcast_params. We probably > >> should change the name of one of them. > > > > We are not able to find duplication of "set_net_config" on master branch of > > open_iscsi. Can you please point out exact location?. > > > > Eddie, is referring to some code in a distro specific patch. Eddie, you > probably have a better picture than Vikas of what is going on since you > have seen the uip related patches. Any naming suggestions? > uIP is the userspace app that our offload solution uses for some of the TCP/IP assistance like DHCP, ICMP, etc. We have plans to upstream this code so other people can use it for the same purposes also. I think it would be more fitting to change the iscsi_transport_template's set_net_config in the initiator to something more specific to uIP like set_uip_config instead. Specifically for the uIP solution, I would imagine it's iscsid thread will only be used to build up the iface database within the app. The actual iface used will have to come from the unique iface handle passed from the transport via PATH_REQUEST. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
