| Message ID | 1360364708-26104-1-git-send-email-agrover@redhat.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote: > RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, > belongs to exactly one portal group within that node." therefore > iscsit_add_np should not check for existing matching portals, it should > just go ahead and try to make the portal, and then kernel_bind() will > return the proper error. > > Signed-off-by: Andy Grover <agrover@redhat.com> > --- NACK. Your interpretation of RFC-3720 is incorrect. There is nothing that says that a single IP address cannot be shared across multiple TargetName+TargetPortalGroupTag endpoints. --nab > drivers/target/iscsi/iscsi_target.c | 64 ----------------------------------- > 1 files changed, 0 insertions(+), 64 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c > index 339f97f..73be05c 100644 > --- a/drivers/target/iscsi/iscsi_target.c > +++ b/drivers/target/iscsi/iscsi_target.c > @@ -264,64 +264,6 @@ int iscsit_deaccess_np(struct iscsi_np *np, struct iscsi_portal_group *tpg) > return 0; > } > > -static struct iscsi_np *iscsit_get_np( > - struct __kernel_sockaddr_storage *sockaddr, > - int network_transport) > -{ > - struct sockaddr_in *sock_in, *sock_in_e; > - struct sockaddr_in6 *sock_in6, *sock_in6_e; > - struct iscsi_np *np; > - int ip_match = 0; > - u16 port; > - > - spin_lock_bh(&np_lock); > - list_for_each_entry(np, &g_np_list, np_list) { > - spin_lock(&np->np_thread_lock); > - if (np->np_thread_state != ISCSI_NP_THREAD_ACTIVE) { > - spin_unlock(&np->np_thread_lock); > - continue; > - } > - > - if (sockaddr->ss_family == AF_INET6) { > - sock_in6 = (struct sockaddr_in6 *)sockaddr; > - sock_in6_e = (struct sockaddr_in6 *)&np->np_sockaddr; > - > - if (!memcmp(&sock_in6->sin6_addr.in6_u, > - &sock_in6_e->sin6_addr.in6_u, > - sizeof(struct in6_addr))) > - ip_match = 1; > - > - port = ntohs(sock_in6->sin6_port); > - } else { > - sock_in = (struct sockaddr_in *)sockaddr; > - sock_in_e = (struct sockaddr_in *)&np->np_sockaddr; > - > - if (sock_in->sin_addr.s_addr == > - sock_in_e->sin_addr.s_addr) > - ip_match = 1; > - > - port = ntohs(sock_in->sin_port); > - } > - > - if ((ip_match == 1) && (np->np_port == port) && > - (np->np_network_transport == network_transport)) { > - /* > - * Increment the np_exports reference count now to > - * prevent iscsit_del_np() below from being called > - * while iscsi_tpg_add_network_portal() is called. > - */ > - np->np_exports++; > - spin_unlock(&np->np_thread_lock); > - spin_unlock_bh(&np_lock); > - return np; > - } > - spin_unlock(&np->np_thread_lock); > - } > - spin_unlock_bh(&np_lock); > - > - return NULL; > -} > - > struct iscsi_np *iscsit_add_np( > struct __kernel_sockaddr_storage *sockaddr, > char *ip_str, > @@ -331,12 +273,6 @@ struct iscsi_np *iscsit_add_np( > struct sockaddr_in6 *sock_in6; > struct iscsi_np *np; > int ret; > - /* > - * Locate the existing struct iscsi_np if already active.. > - */ > - np = iscsit_get_np(sockaddr, network_transport); > - if (np) > - return np; > > np = kzalloc(sizeof(struct iscsi_np), GFP_KERNEL); > if (!np) { -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/13/2013 12:31 PM, Nicholas A. Bellinger wrote: > On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote: >> RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, >> belongs to exactly one portal group within that node." therefore >> iscsit_add_np should not check for existing matching portals, it should >> just go ahead and try to make the portal, and then kernel_bind() will >> return the proper error. >> >> Signed-off-by: Andy Grover <agrover@redhat.com> >> --- > > NACK. Your interpretation of RFC-3720 is incorrect. There is nothing > that says that a single IP address cannot be shared across multiple > TargetName+TargetPortalGroupTag endpoints. A Network Portal is ip:port, not just IP. I'd agree two TPGs can use the same IP as long as they listen on different ports. But that bit I quoted seems pretty clear. How should it be alternatively interpreted? Thanks -- Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-02-13 at 14:09 -0800, Andy Grover wrote: > On 02/13/2013 12:31 PM, Nicholas A. Bellinger wrote: > > On Fri, 2013-02-08 at 15:05 -0800, Andy Grover wrote: > >> RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, > >> belongs to exactly one portal group within that node." therefore > >> iscsit_add_np should not check for existing matching portals, it should > >> just go ahead and try to make the portal, and then kernel_bind() will > >> return the proper error. > >> > >> Signed-off-by: Andy Grover <agrover@redhat.com> > >> --- > > > > NACK. Your interpretation of RFC-3720 is incorrect. There is nothing > > that says that a single IP address cannot be shared across multiple > > TargetName+TargetPortalGroupTag endpoints. > > A Network Portal is ip:port, not just IP. I'd agree two TPGs can use the > same IP as long as they listen on different ports. > No. The whole point of having IQNs is to decouple the network portal access from the target node, so that network portals can be shared across the network entity. > But that bit I quoted seems pretty clear. How should it be alternatively > interpreted? > Your completely ignoring all the previous context to reach this conclusion. Consider: 3.4. SCSI to iSCSI Concepts Mapping Model The following diagram shows an example of how multiple iSCSI Nodes (targets in this case) can coexist within the same Network Entity and can share Network Portals (IP addresses and TCP ports). .... and, 3.4.1 iSCSI Architecture Model a) Network Entity - represents a device or gateway that is accessible from the IP network. A Network Entity must have one or more Network Portals (see item d), each of which can be used by some iSCSI Nodes (see item (b)) contained in that Network Entity to gain access to the IP network. and, b) iSCSI Node - ..... The separation of the iSCSI Name from the addresses used by and for the iSCSI node allows multiple iSCSI nodes to use the same addresses, and the same iSCSI node to use multiple addresses. and, Appendix D. SendTargets Operation The next example has two internal iSCSI targets, each accessible via two different ports with different IP addresses. The following is the text response: TargetName=iqn.1993-11.com.example:diskarray.sn.8675309 TargetAddress=10.1.0.45:3000,1 TargetAddress=10.1.1.45:3000,2 TargetName=iqn.1993-11.com.example:diskarray.sn.1234567 TargetAddress=10.1.0.45:3000,1 TargetAddress=10.1.1.45:3000,2 Both targets share both addresses; the multiple addresses are likely used to provide multi-path support. The initiator may connect to either target name on either address. The wording in section Section 3.4.1, e) that your referring to: "Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node." does not mean that individual network portals are limited to a single network entity, but that network portals are linked to a single TPG within an individual TargetName. Eg, 'that node' does not mean the entire physical machine (network entity), that may contain multiple nodes (TargetName+TargetPortalGroupTag endpoints). However, in practice I've not yet seen a target implementation that supports multiple TPGs actually enforce this, considering this is not accompanied by a "SHOULD not" or "MUST not" anywhere in the spec. So unless you have a specific problem case where this is causing an issue with an initiator, I'm likely not going to accept a kernel patch to change existing behavior. --nab -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/15/2013 07:46 AM, Nicholas A. Bellinger wrote: > The wording in section Section 3.4.1, e) that your referring to: > > "Each Network Portal, as utilized by a given iSCSI Node, belongs to > exactly one portal group within that node." > > does not mean that individual network portals are limited to a single > network entity, but that network portals are linked to a single TPG > within an individual TargetName. Eg, 'that node' does not mean the > entire physical machine (network entity), that may contain multiple > nodes (TargetName+TargetPortalGroupTag endpoints). > > However, in practice I've not yet seen a target implementation that > supports multiple TPGs actually enforce this, considering this is not > accompanied by a "SHOULD not" or "MUST not" anywhere in the spec. So > unless you have a specific problem case where this is causing an issue > with an initiator, I'm likely not going to accept a kernel patch to > change existing behavior. OK, so I'm clear now that a NetworkPortal can be shared among TargetNames, but not among TPGs within a TargetName. But LIO currently allows it. See https://bugzilla.redhat.com/show_bug.cgi?id=908368 . The tester's actual issue may not be related to this area, but if you look at the attachment in comment 2, this configuration was allowed. I don't think this is an issue where we need to worry about existing behavior. This *can't* work because the initiator passes the desired TargetName during iSCSI login, but not TargetPortalGroupTag. There's no way a target can tell which TPG the initiator wants if the TargetName for two are the same. We could add a check for this to the rtslib userspace library, but this would mean the kernel could still be configured this way, if rtslib was not used to wrap configfs accesses. Therefore I'd push for the kernel to check for this. Would a patch for that fly? Thanks -- Regards -- Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-02-18 at 14:41 -0800, Andy Grover wrote: > On 02/15/2013 07:46 AM, Nicholas A. Bellinger wrote: > > The wording in section Section 3.4.1, e) that your referring to: > > > > "Each Network Portal, as utilized by a given iSCSI Node, belongs to > > exactly one portal group within that node." > > > > does not mean that individual network portals are limited to a single > > network entity, but that network portals are linked to a single TPG > > within an individual TargetName. Eg, 'that node' does not mean the > > entire physical machine (network entity), that may contain multiple > > nodes (TargetName+TargetPortalGroupTag endpoints). > > > > However, in practice I've not yet seen a target implementation that > > supports multiple TPGs actually enforce this, considering this is not > > accompanied by a "SHOULD not" or "MUST not" anywhere in the spec. So > > unless you have a specific problem case where this is causing an issue > > with an initiator, I'm likely not going to accept a kernel patch to > > change existing behavior. > > OK, so I'm clear now that a NetworkPortal can be shared among > TargetNames, but not among TPGs within a TargetName. > > But LIO currently allows it. > See https://bugzilla.redhat.com/show_bug.cgi?id=908368 . > > The tester's actual issue may not be related to this area, but if you > look at the attachment in comment 2, this configuration was allowed. > Yes, it's related. He will want to be using multiple IQNs for this type of setup. > I don't think this is an issue where we need to worry about existing > behavior. This *can't* work because the initiator passes the desired > TargetName during iSCSI login, but not TargetPortalGroupTag. There's no > way a target can tell which TPG the initiator wants if the TargetName > for two are the same. > > We could add a check for this to the rtslib userspace library, but this > would mean the kernel could still be configured this way, if rtslib was > not used to wrap configfs accesses. Therefore I'd push for the kernel to > check for this. Would a patch for that fly? > So considering in this special case that an target cannot distinguish between TargetPortalGroup for an incoming Login Request, enforcing from the kernel that individual network portals only be mapped to a single TargetPortalGroup within TargetName context is going to be the proper resolution here. I'm working on a patch for this, and will post shortly.. Thanks, --nab -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 339f97f..73be05c 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -264,64 +264,6 @@ int iscsit_deaccess_np(struct iscsi_np *np, struct iscsi_portal_group *tpg) return 0; } -static struct iscsi_np *iscsit_get_np( - struct __kernel_sockaddr_storage *sockaddr, - int network_transport) -{ - struct sockaddr_in *sock_in, *sock_in_e; - struct sockaddr_in6 *sock_in6, *sock_in6_e; - struct iscsi_np *np; - int ip_match = 0; - u16 port; - - spin_lock_bh(&np_lock); - list_for_each_entry(np, &g_np_list, np_list) { - spin_lock(&np->np_thread_lock); - if (np->np_thread_state != ISCSI_NP_THREAD_ACTIVE) { - spin_unlock(&np->np_thread_lock); - continue; - } - - if (sockaddr->ss_family == AF_INET6) { - sock_in6 = (struct sockaddr_in6 *)sockaddr; - sock_in6_e = (struct sockaddr_in6 *)&np->np_sockaddr; - - if (!memcmp(&sock_in6->sin6_addr.in6_u, - &sock_in6_e->sin6_addr.in6_u, - sizeof(struct in6_addr))) - ip_match = 1; - - port = ntohs(sock_in6->sin6_port); - } else { - sock_in = (struct sockaddr_in *)sockaddr; - sock_in_e = (struct sockaddr_in *)&np->np_sockaddr; - - if (sock_in->sin_addr.s_addr == - sock_in_e->sin_addr.s_addr) - ip_match = 1; - - port = ntohs(sock_in->sin_port); - } - - if ((ip_match == 1) && (np->np_port == port) && - (np->np_network_transport == network_transport)) { - /* - * Increment the np_exports reference count now to - * prevent iscsit_del_np() below from being called - * while iscsi_tpg_add_network_portal() is called. - */ - np->np_exports++; - spin_unlock(&np->np_thread_lock); - spin_unlock_bh(&np_lock); - return np; - } - spin_unlock(&np->np_thread_lock); - } - spin_unlock_bh(&np_lock); - - return NULL; -} - struct iscsi_np *iscsit_add_np( struct __kernel_sockaddr_storage *sockaddr, char *ip_str, @@ -331,12 +273,6 @@ struct iscsi_np *iscsit_add_np( struct sockaddr_in6 *sock_in6; struct iscsi_np *np; int ret; - /* - * Locate the existing struct iscsi_np if already active.. - */ - np = iscsit_get_np(sockaddr, network_transport); - if (np) - return np; np = kzalloc(sizeof(struct iscsi_np), GFP_KERNEL); if (!np) {
RFC 3720 says "Each Network Portal, as utilized by a given iSCSI Node, belongs to exactly one portal group within that node." therefore iscsit_add_np should not check for existing matching portals, it should just go ahead and try to make the portal, and then kernel_bind() will return the proper error. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/iscsi/iscsi_target.c | 64 ----------------------------------- 1 files changed, 0 insertions(+), 64 deletions(-)