| Message ID | 33dfc646-1804-363e-4d83-1ba618303dbb@grimberg.me (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sun, Jul 31, 2016 at 3:58 AM, Sagi Grimberg <sagi@grimberg.me> wrote: > How did you test it btw? I have a userspace target :). Which is also how I caught the uninitialized private data. > Currently the target driver doesn't support ipv6 addresses. > > These complementary patches [1,2] makes the it work e2e! That's cool! Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> How did you test it btw? > > I have a userspace target :). Interesting. Is the target public? > Which is also how I caught the uninitialized private data. Nice catch! -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> How did you test it btw? >> >> I have a userspace target :). Is it the spdk one? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Currently the target driver doesn't support ipv6 addresses. > > These complementary patches [1,2] makes the it work e2e! So, I think we should use the same ipv6 addr parsing that Roland used in the host, and preferably have another module export that as a helper. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Currently the target driver doesn't support ipv6 addresses. >> >> These complementary patches [1,2] makes the it work e2e! So, > > I think we should use the same ipv6 addr parsing that Roland used > in the host, and preferably have another module export that as a helper. I can try to unify them, but unlike the host that first tries ipv4 and then fall to ipv6, the target needs to enforce the address family so it's somewhat different. I kinda think that these are different enough to stay separate don't you think? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 01, 2016 at 02:24:10PM +0300, Sagi Grimberg wrote: > >>> Currently the target driver doesn't support ipv6 addresses. >>> >>> These complementary patches [1,2] makes the it work e2e! So, >> >> I think we should use the same ipv6 addr parsing that Roland used >> in the host, and preferably have another module export that as a helper. > > I can try to unify them, but unlike the host that first tries ipv4 and > then fall to ipv6, the target needs to enforce the address family so > it's somewhat different. I kinda think that these are different enough > to stay separate don't you think? It'd still need all the scope ID handling similar to what Roland did, and that's a fair chunk of code. We have a few options to handle the different allowed addresses: (1) v4/v6 only flags (2) having low-level v4/v6 handlers and one that tries these both (3) using the try both handler and rejecting the wrong one after parsing. (3) seems easiest, but (2) sounds fine to me. But I'd really like to hear from folks on the netdev list what they think of that idea first. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 1, 2016 at 8:50 AM, Christoph Hellwig <hch@lst.de> wrote: > It'd still need all the scope ID handling similar to what Roland did, > and that's a fair chunk of code. We have a few options to handle the > different allowed addresses: > > (1) v4/v6 only flags > (2) having low-level v4/v6 handlers and one that tries these both > (3) using the try both handler and rejecting the wrong one after > parsing. > > (3) seems easiest, but (2) sounds fine to me. But I'd really like to > hear from folks on the netdev list what they think of that idea first. I think adding a new helper that parses both v4 and v6 addresses + scope ID seems like the best thing for now. I did a grep for in6_pton and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c could use the helper. What do you think of adding inet_pton_with_scope() to net/core/utils.c? I'm open to better ideas on the name. But I can code that up and use it in nvme, as well as convert over the two places I mentioned above. The first parameter of the function can be an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to restrict the parsing to one type of address (or not). - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Currently the target driver doesn't support ipv6 addresses. >>>> >>>> These complementary patches [1,2] makes the it work e2e! So, >>> >>> I think we should use the same ipv6 addr parsing that Roland used >>> in the host, and preferably have another module export that as a helper. >> >> I can try to unify them, but unlike the host that first tries ipv4 and >> then fall to ipv6, the target needs to enforce the address family so >> it's somewhat different. I kinda think that these are different enough >> to stay separate don't you think? > > It'd still need all the scope ID handling similar to what Roland did, > and that's a fair chunk of code. We have a few options to handle the > different allowed addresses: > > (1) v4/v6 only flags > (2) having low-level v4/v6 handlers and one that tries these both > (3) using the try both handler and rejecting the wrong one after > parsing. > > (3) seems easiest, but (2) sounds fine to me. But I'd really like to > hear from folks on the netdev list what they think of that idea first. (2) sounds fine to me too. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> It'd still need all the scope ID handling similar to what Roland did, >> and that's a fair chunk of code. We have a few options to handle the >> different allowed addresses: >> >> (1) v4/v6 only flags >> (2) having low-level v4/v6 handlers and one that tries these both >> (3) using the try both handler and rejecting the wrong one after >> parsing. >> >> (3) seems easiest, but (2) sounds fine to me. But I'd really like to >> hear from folks on the netdev list what they think of that idea first. > > I think adding a new helper that parses both v4 and v6 addresses + > scope ID seems like the best thing for now. I did a grep for in6_pton > and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c > could use the helper. The iscsi target code can use it too. > What do you think of adding inet_pton_with_scope() to > net/core/utils.c? I'm open to better ideas on the name. But I can > code that up and use it in nvme, as well as convert over the two > places I mentioned above. The first parameter of the function can be > an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to > restrict the parsing to one type of address (or not). Sounds good Roland. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 01, 2016 at 09:06:07AM -0700, Roland Dreier wrote: > On Mon, Aug 1, 2016 at 8:50 AM, Christoph Hellwig <hch@lst.de> wrote: > > It'd still need all the scope ID handling similar to what Roland did, > > and that's a fair chunk of code. We have a few options to handle the > > different allowed addresses: > > > > (1) v4/v6 only flags > > (2) having low-level v4/v6 handlers and one that tries these both > > (3) using the try both handler and rejecting the wrong one after > > parsing. > > > > (3) seems easiest, but (2) sounds fine to me. But I'd really like to > > hear from folks on the netdev list what they think of that idea first. > > I think adding a new helper that parses both v4 and v6 addresses + > scope ID seems like the best thing for now. I did a grep for in6_pton > and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c > could use the helper. > > What do you think of adding inet_pton_with_scope() to > net/core/utils.c? I'm open to better ideas on the name. But I can > code that up and use it in nvme, as well as convert over the two > places I mentioned above. The first parameter of the function can be > an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to > restrict the parsing to one type of address (or not). Sounds fine to me, and I hope the netdev folks (Cc'ed) are fine with that as well. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I think adding a new helper that parses both v4 and v6 addresses + > scope ID seems like the best thing for now. I did a grep for in6_pton > and it looks like at least fs/cifs/netmisc.c and net/sunrpc/addr.c > could use the helper. > > What do you think of adding inet_pton_with_scope() to > net/core/utils.c? I'm open to better ideas on the name. But I can > code that up and use it in nvme, as well as convert over the two > places I mentioned above. The first parameter of the function can be > an af, and the caller can pass in AF_UNSPEC, AF_INET, or AF_INET6 to > restrict the parsing to one type of address (or not). Hey Roland, Are you looking into this? I'd love to get it into 4.9... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 18, 2016 at 12:44 AM, Sagi Grimberg <sagi@grimberg.me> wrote: > Are you looking into this? > > I'd love to get it into 4.9... Yeah, me too. I got distracted about halfway through writing the patch but I will finish it this week. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Are you looking into this? >> >> I'd love to get it into 4.9... > > Yeah, me too. I got distracted about halfway through writing the > patch but I will finish it this week. Awesome! thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/nvme/target/rdma.c b/drivers/nvme/target/rdma.c index 4e83d92d6bdd..347cc6d37dad 100644 --- a/drivers/nvme/target/rdma.c +++ b/drivers/nvme/target/rdma.c @@ -1336,29 +1336,65 @@ restart: mutex_unlock(&nvmet_rdma_queue_mutex); } -static int nvmet_rdma_add_port(struct nvmet_port *port) +static int nvmet_rdma_set_addr_port(int family, char *ipaddr, + char *ipport, struct sockaddr_storage *addr) { - struct rdma_cm_id *cm_id; - struct sockaddr_in addr_in; - u16 port_in; - int ret; + size_t buflen = strlen(ipaddr); + struct sockaddr_in *addr4; + struct sockaddr_in6 *addr6; + u16 port; + + if (kstrtou16(ipport, 0, &port)) + return -EINVAL; - switch (port->disc_addr.adrfam) { + switch (family) { case NVMF_ADDR_FAMILY_IP4: + if (buflen > INET_ADDRSTRLEN) + return -EINVAL; + + addr4 = (struct sockaddr_in *)addr; + + if (in4_pton(ipaddr, buflen, (u8 *)&addr4->sin_addr.s_addr, + '\n', NULL) == 0) + return -EINVAL; + + addr4->sin_family = AF_INET; + addr4->sin_port = htons(port); + break; + case NVMF_ADDR_FAMILY_IP6: + if (buflen > INET6_ADDRSTRLEN) + return -EINVAL; + + addr6 = (struct sockaddr_in6 *) addr; + + if (in6_pton(ipaddr, buflen, (u8 *)&addr6->sin6_addr.s6_addr, + '\n', NULL) == 0) + return -EINVAL; + + addr6->sin6_family = AF_INET6; + addr6->sin6_port = htons(port); break; default: - pr_err("address family %d not supported\n", - port->disc_addr.adrfam); + pr_err("address family %d not supported\n", family); return -EINVAL; } - ret = kstrtou16(port->disc_addr.trsvcid, 0, &port_in); - if (ret) - return ret; + return 0; +} - addr_in.sin_family = AF_INET; - addr_in.sin_addr.s_addr = in_aton(port->disc_addr.traddr); - addr_in.sin_port = htons(port_in); +static int nvmet_rdma_add_port(struct nvmet_port *port) +{ + struct rdma_cm_id *cm_id; + struct sockaddr_storage addr = { }; + int ret; + + ret = nvmet_rdma_set_addr_port(port->disc_addr.adrfam, + port->disc_addr.traddr, port->disc_addr.trsvcid, &addr); + if (ret) { + pr_err("failed setting addr %s:%s\n", + port->disc_addr.traddr, port->disc_addr.trsvcid); + return ret; + } cm_id = rdma_create_id(&init_net, nvmet_rdma_cm_handler, port, RDMA_PS_TCP, IB_QPT_RC); @@ -1367,20 +1403,33 @@ static int nvmet_rdma_add_port(struct nvmet_port *port) return PTR_ERR(cm_id); } - ret = rdma_bind_addr(cm_id, (struct sockaddr *)&addr_in); + /* + * Allow both IPv4 and IPv6 sockets to bind a single port + * at the same time. + */ + ret = rdma_set_afonly(cm_id, 1); + if (ret) { + pr_err("rdma_set_afonly failed (%d)\n", ret); + goto out_destroy_id; + } + + ret = rdma_bind_addr(cm_id, (struct sockaddr *)&addr); if (ret) { - pr_err("binding CM ID to %pISpc failed (%d)\n", &addr_in, ret); + pr_err("binding CM ID to %pISpcs failed (%d)\n", + (struct sockaddr *)&addr, ret); goto out_destroy_id; } ret = rdma_listen(cm_id, 128); if (ret) { - pr_err("listening to %pISpc failed (%d)\n", &addr_in, ret); + pr_err("listening to %pISpcs failed (%d)\n", + (struct sockaddr *)&addr, ret); goto out_destroy_id; } - pr_info("enabling port %d (%pISpc)\n", - le16_to_cpu(port->disc_addr.portid), &addr_in); + pr_info("enabling port %d (%pISpcs)\n", + le16_to_cpu(port->disc_addr.portid), + (struct sockaddr *)&addr); port->priv = cm_id; return 0; -- [2]: -- commit 6d962c76580680e9cd0f5a56f47bcd30b5a39dd7 Author: Sagi Grimberg <sagi@grimberg.me> Date: Sun Jul 31 13:56:58 2016 +0300 fabrics: Allow ipv6 address resolution Signed-off-by: Sagi Grimberg <sagi@grimberg.me> diff --git a/fabrics.c b/fabrics.c index 4bf557b5e672..221e34e5e39b 100644 --- a/fabrics.c +++ b/fabrics.c @@ -390,7 +390,8 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e) /* we can safely ignore the rest of the entries */ break; case NVMF_TRTYPE_RDMA: - if (e->adrfam != NVMF_ADDR_FAMILY_IP4) { + if (e->adrfam != NVMF_ADDR_FAMILY_IP4 && + e->adrfam != NVMF_ADDR_FAMILY_IP6) { fprintf(stderr, "skipping unsupported adrfam\n"); return -EINVAL;