[PATCH rfc 0/6] support traddr as dns names for ip based transports

[PATCH rfc 0/6] support traddr as dns names for ip based transports

[Sagi Grimberg]

Currently nvme-cli is able to resolve an ip address from a dns name if
traddr argument was given as a dns name, so it does that and passes down
to the kernel the resolved ip address.

However, if the dns resolution changes over time, the kernel will not
be able to reconnect because it has a stale ip address. While this is
not a real use case for nvme subsystems, it could be a valid use-case
for discovery controllers, where the resolution is made from a dns
name that may load-balance/failover over multiple endpoints.

The last patch (7/6) is for libnvme to pass down the traddr as a dns
name if the kernel supports dns queries.

The inspiration was taken from the nfs client that does this for
redirects or locations discovery.

It is an rfc, so feedback is welcome.

Sagi Grimberg (6):
  nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name
  nvme-tcp: use nvmf_resolve_address helper
  nvme-tcp: re-resolve traddr upon reconnect in case it is a dns name
  nvme-rdma: use nvmf_resolve_address helper
  nvme-rdma: re-resolve traddr upon reconnect in case it is a dns name
  nvme-fabrics: expose support for traddr as dns names to userspace

 drivers/nvme/host/fabrics.c | 29 +++++++++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |  2 ++
 drivers/nvme/host/rdma.c    | 21 +++++++++++++--------
 drivers/nvme/host/tcp.c     | 21 +++++++++++++--------
 4 files changed, 57 insertions(+), 16 deletions(-)

-- 
2.41.0

[PATCH rfc 1/6] nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name

[Sagi Grimberg]

Allow userspace to pass in traddr as a dns name as well. This may be
useful for a discovery service that may have a dns name, that may also
change in time (with an updated dns record).

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/fabrics.c | 28 ++++++++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 8175d49f2909..fadd25538d1b 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -10,6 +10,7 @@
 #include <linux/mutex.h>
 #include <linux/parser.h>
 #include <linux/seq_file.h>
+#include <linux/dns_resolver.h>
 #include "nvme.h"
 #include "fabrics.h"
 
@@ -1133,6 +1134,33 @@ bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvmf_ip_options_match);
 
+int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
+		struct sockaddr_storage *traddr)
+{
+	char *traddr_str = NULL;
+	int traddr_len;
+	int ret;
+
+	/* first check if traddr is ipv4/ipv6 address */
+	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
+			opts->traddr, opts->trsvcid, traddr);
+	if (!ret)
+		return ret;
+
+	/* second check if traddr is dns name */
+	traddr_len = dns_query(&init_net, NULL, opts->traddr,
+			strlen(opts->traddr), NULL, &traddr_str,
+			NULL, false);
+	if (traddr_len < 0)
+		return traddr_len;
+
+	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
+			traddr_str, opts->trsvcid, traddr);
+	kfree(traddr_str);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nvmf_resolve_ip_address);
+
 static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
 		unsigned int allowed_opts)
 {
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 82e7a27ffbde..603d8d176168 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -222,6 +222,8 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
+int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
+		struct sockaddr_storage *traddr);
 void nvmf_set_io_queues(struct nvmf_ctrl_options *opts, u32 nr_io_queues,
 			u32 io_queues[HCTX_MAX_TYPES]);
 void nvmf_map_queues(struct blk_mq_tag_set *set, struct nvme_ctrl *ctrl,
-- 
2.41.0

[PATCH rfc 2/6] nvme-tcp: use nvmf_resolve_address helper

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/tcp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 3e7dd6f91832..a47b597ace3e 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2537,8 +2537,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 		opts->mask |= NVMF_OPT_TRSVCID;
 	}
 
-	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
-			opts->traddr, opts->trsvcid, &ctrl->addr);
+	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
 	if (ret) {
 		pr_err("malformed address passed: %s:%s\n",
 			opts->traddr, opts->trsvcid);
-- 
2.41.0

[PATCH rfc 3/6] nvme-tcp: re-resolve traddr upon reconnect in case it is a dns name

[Sagi Grimberg]

In case traddr is a dns name, it may describe a different address
during the controller lifetime. The classic example would be
a discovery-service dns name that may be load-balanced via dns over
multiple addresses.

So try to re-resolve the controller address. There is no dns query
in case the traddr provided is an ip address.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/tcp.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a47b597ace3e..d44d88d12ea0 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1917,8 +1917,21 @@ static void nvme_tcp_destroy_admin_queue(struct nvme_ctrl *ctrl, bool remove)
 
 static int nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
 {
+	struct nvmf_ctrl_options *opts = ctrl->opts;
 	int error;
 
+	/*
+	 * it is possible that the controller traddr may be a dns name which
+	 * may have changed its ip address since it was last resolved in prior
+	 * connect. Try to re-resolve the address in this case.
+	 */
+	error = nvmf_resolve_ip_address(opts, &to_tcp_ctrl(ctrl)->addr);
+	if (error) {
+		dev_warn(ctrl->device, "unable to resolve address: %s:%s\n",
+			opts->traddr, opts->trsvcid);
+		return error;
+	}
+
 	error = nvme_tcp_alloc_admin_queue(ctrl);
 	if (error)
 		return error;
@@ -2537,13 +2550,6 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 		opts->mask |= NVMF_OPT_TRSVCID;
 	}
 
-	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
-	if (ret) {
-		pr_err("malformed address passed: %s:%s\n",
-			opts->traddr, opts->trsvcid);
-		goto out_free_ctrl;
-	}
-
 	if (opts->mask & NVMF_OPT_HOST_TRADDR) {
 		ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
 			opts->host_traddr, NULL, &ctrl->src_addr);
-- 
2.41.0

[PATCH rfc 4/6] nvme-rdma: use nvmf_resolve_address helper

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/rdma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d433b2ec07a6..797bd192a413 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2230,8 +2230,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		opts->mask |= NVMF_OPT_TRSVCID;
 	}
 
-	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
-			opts->traddr, opts->trsvcid, &ctrl->addr);
+	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
 	if (ret) {
 		pr_err("malformed address passed: %s:%s\n",
 			opts->traddr, opts->trsvcid);
-- 
2.41.0

[PATCH rfc 5/6] nvme-rdma: re-resolve traddr upon reconnect in case it is a dns name

[Sagi Grimberg]

In case traddr is a dns name, it may describe a different address
during the controller lifetime. The classic example would be
a discovery-service dns name that may be load-balanced via dns over
multiple addresses.

So try to re-resolve the controller address. There is no dns query
in case the traddr provided is an ip address.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/rdma.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 797bd192a413..2a4109bbd4a2 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -777,9 +777,22 @@ static void nvme_rdma_destroy_admin_queue(struct nvme_rdma_ctrl *ctrl)
 static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
 		bool new)
 {
+	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
 	bool pi_capable = false;
 	int error;
 
+	/*
+	 * it is possible that the controller traddr may be a dns name which
+	 * may have changed its ip address since it was last resolved in prior
+	 * connect. Try to re-resolve the address in this case.
+	 */
+	error = nvmf_resolve_ip_address(opts, &ctrl->addr);
+	if (error) {
+		dev_warn(ctrl->ctrl.device, "unable to resolve address: %s:%s\n",
+			opts->traddr, opts->trsvcid);
+		return error;
+	}
+
 	error = nvme_rdma_alloc_queue(ctrl, 0, NVME_AQ_DEPTH);
 	if (error)
 		return error;
@@ -2230,13 +2243,6 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		opts->mask |= NVMF_OPT_TRSVCID;
 	}
 
-	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
-	if (ret) {
-		pr_err("malformed address passed: %s:%s\n",
-			opts->traddr, opts->trsvcid);
-		goto out_free_ctrl;
-	}
-
 	if (opts->mask & NVMF_OPT_HOST_TRADDR) {
 		ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
 			opts->host_traddr, NULL, &ctrl->src_addr);
-- 
2.41.0

[PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

userspace needs to know that the kernel is capable of doing a dns
query, so expose a custom opt called "dns_ip_traddr" which is not
an argument, but rather indication of support for dns names for
ip based traddr.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/fabrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index fadd25538d1b..6a7e8cbdaf69 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -1331,6 +1331,7 @@ static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)
 		seq_puts(seq_file, ",");
 		seq_puts(seq_file, tok->pattern);
 	}
+	seq_puts(seq_file, ",dns_ip_traddr");
 	seq_puts(seq_file, "\n");
 }
 
-- 
2.41.0

[PATCH rfc libnvme 7/6] fabrics: pass traddr dns name if the kernel supports it

[Sagi Grimberg]

If the kernel supports dns names as traddr (i.e. able to query the dns to
resolve the ip address), pass the dns name down to the kernel.

The advantage is that if the dns resolution changes to a different ip
address, the kernel will be able to reconnect.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 src/nvme/fabrics.c | 10 ++++++----
 src/nvme/private.h |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/nvme/fabrics.c b/src/nvme/fabrics.c
index 2a9d8dc7f23a..0e9b4a15858b 100644
--- a/src/nvme/fabrics.c
+++ b/src/nvme/fabrics.c
@@ -736,6 +736,7 @@ static  int __nvmf_supported_options(nvme_root_t r)
 		parse_option(r, v, traddr);
 		parse_option(r, v, transport);
 		parse_option(r, v, trsvcid);
+		parse_option(r, v, dns_ip_traddr);
 	}
 	nvme_msg(r, LOG_DEBUG, "\n");
 	ret = 0;
@@ -900,8 +901,12 @@ int nvmf_add_ctrl(nvme_host_t h, nvme_ctrl_t c,
 		}
 	}
 
+	ret = __nvmf_supported_options(h->r);
+	if (ret)
+		return ret;
+
 	nvme_ctrl_set_discovered(c, true);
-	if (traddr_is_hostname(h->r, c)) {
+	if (traddr_is_hostname(h->r, c) && !h->r->options->dns_ip_traddr) {
 		char *traddr = c->traddr;
 
 		c->traddr = hostname2traddr(h->r, traddr);
@@ -913,9 +918,6 @@ int nvmf_add_ctrl(nvme_host_t h, nvme_ctrl_t c,
 		free(traddr);
 	}
 
-	ret = __nvmf_supported_options(h->r);
-	if (ret)
-		return ret;
 	ret = build_options(h, c, &argstr);
 	if (ret)
 		return ret;
diff --git a/src/nvme/private.h b/src/nvme/private.h
index f4992a48a41e..e58736b1e18d 100644
--- a/src/nvme/private.h
+++ b/src/nvme/private.h
@@ -152,6 +152,7 @@ struct nvme_fabric_options {
 	bool traddr;
 	bool transport;
 	bool trsvcid;
+	bool dns_ip_traddr;
 };
 
 struct nvme_root {
-- 
2.41.0

Re: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Sagi Grimberg]

> Currently nvme-cli is able to resolve an ip address from a dns name if
> traddr argument was given as a dns name, so it does that and passes down
> to the kernel the resolved ip address.
> 
> However, if the dns resolution changes over time, the kernel will not
> be able to reconnect because it has a stale ip address. While this is
> not a real use case for nvme subsystems, it could be a valid use-case
> for discovery controllers, where the resolution is made from a dns
> name that may load-balance/failover over multiple endpoints.
> 
> The last patch (7/6) is for libnvme to pass down the traddr as a dns
> name if the kernel supports dns queries.
> 
> The inspiration was taken from the nfs client that does this for
> redirects or locations discovery.
> 
> It is an rfc, so feedback is welcome.

So I get that there is zero appetite to support this?

Re: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Daniel Wagner]

On Thu, Aug 10, 2023 at 03:03:46PM +0300, Sagi Grimberg wrote:
> 
> > Currently nvme-cli is able to resolve an ip address from a dns name if
> > traddr argument was given as a dns name, so it does that and passes down
> > to the kernel the resolved ip address.
> > 
> > However, if the dns resolution changes over time, the kernel will not
> > be able to reconnect because it has a stale ip address. While this is
> > not a real use case for nvme subsystems, it could be a valid use-case
> > for discovery controllers, where the resolution is made from a dns
> > name that may load-balance/failover over multiple endpoints.
> > 
> > The last patch (7/6) is for libnvme to pass down the traddr as a dns
> > name if the kernel supports dns queries.
> > 
> > The inspiration was taken from the nfs client that does this for
> > redirects or locations discovery.
> > 
> > It is an rfc, so feedback is welcome.
> 
> So I get that there is zero appetite to support this?

Sorry, just too many things at the same time.

I think it makes totaly sense to move the resolve step down into the
kernel. So I am all good with this.

Re: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Hannes Reinecke]

On 8/10/23 14:03, Sagi Grimberg wrote:
> 
>> Currently nvme-cli is able to resolve an ip address from a dns name if
>> traddr argument was given as a dns name, so it does that and passes down
>> to the kernel the resolved ip address.
>>
>> However, if the dns resolution changes over time, the kernel will not
>> be able to reconnect because it has a stale ip address. While this is
>> not a real use case for nvme subsystems, it could be a valid use-case
>> for discovery controllers, where the resolution is made from a dns
>> name that may load-balance/failover over multiple endpoints.
>>
>> The last patch (7/6) is for libnvme to pass down the traddr as a dns
>> name if the kernel supports dns queries.
>>
>> The inspiration was taken from the nfs client that does this for
>> redirects or locations discovery.
>>
>> It is an rfc, so feedback is welcome.
> 
> So I get that there is zero appetite to support this?
No, actually not.

I'm pretty much in favour of it.

Hmm. Probably should send a review ... wait ...

Cheers,

Hannes

RE: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Belanger, Martin]

> So I get that there is zero appetite to support this?

I do have appetite for this. Yum!

Martin


Internal Use - Confidential

Re: [PATCH rfc 1/6] nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> Allow userspace to pass in traddr as a dns name as well. This may be
> useful for a discovery service that may have a dns name, that may also
> change in time (with an updated dns record).
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/fabrics.c | 28 ++++++++++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |  2 ++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 8175d49f2909..fadd25538d1b 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -10,6 +10,7 @@
>   #include <linux/mutex.h>
>   #include <linux/parser.h>
>   #include <linux/seq_file.h>
> +#include <linux/dns_resolver.h>
>   #include "nvme.h"
>   #include "fabrics.h"
>   
> @@ -1133,6 +1134,33 @@ bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   }
>   EXPORT_SYMBOL_GPL(nvmf_ip_options_match);
>   
> +int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
> +		struct sockaddr_storage *traddr)
> +{
> +	char *traddr_str = NULL;
> +	int traddr_len;
> +	int ret;
> +
> +	/* first check if traddr is ipv4/ipv6 address */
> +	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
> +			opts->traddr, opts->trsvcid, traddr);
> +	if (!ret)
> +		return ret;
> +
> +	/* second check if traddr is dns name */
> +	traddr_len = dns_query(&init_net, NULL, opts->traddr,
> +			strlen(opts->traddr), NULL, &traddr_str,
> +			NULL, false);
> +	if (traddr_len < 0)
> +		return traddr_len;
> +
> +	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
> +			traddr_str, opts->trsvcid, traddr);
> +	kfree(traddr_str);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nvmf_resolve_ip_address);
> +
>   static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
>   		unsigned int allowed_opts)
>   {
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 82e7a27ffbde..603d8d176168 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -222,6 +222,8 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   		struct nvmf_ctrl_options *opts);
> +int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
> +		struct sockaddr_storage *traddr);
>   void nvmf_set_io_queues(struct nvmf_ctrl_options *opts, u32 nr_io_queues,IS
>   			u32 io_queues[HCTX_MAX_TYPES]);
>   void nvmf_map_queues(struct blk_mq_tag_set *set, struct nvme_ctrl *ctrl,

dns resolver is a config option, so we would need either a '#ifdef 
DNS_RESOLVER' or if (IS_ENABLED(CONFIG_DNS_RESOLVER))' somewhere ...

Cheers,

Hannes

Re: [PATCH rfc 2/6] nvme-tcp: use nvmf_resolve_address helper

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/tcp.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 3e7dd6f91832..a47b597ace3e 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2537,8 +2537,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
>   		opts->mask |= NVMF_OPT_TRSVCID;
>   	}
>   
> -	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
> -			opts->traddr, opts->trsvcid, &ctrl->addr);
> +	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
>   	if (ret) {
>   		pr_err("malformed address passed: %s:%s\n",
>   			opts->traddr, opts->trsvcid);

Merge with the first patch?
Otherwise okay.

Cheers,

Hannes

RE: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Belanger, Martin]

> > So I get that there is zero appetite to support this?
> 
> I do have appetite for this. Yum!
> 
> Martin
> 
> 
> Internal Use - Confidential

Oops! Sorry about the automatic "Internal Use" label.

Re: [PATCH rfc 3/6] nvme-tcp: re-resolve traddr upon reconnect in case it is a dns name

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> In case traddr is a dns name, it may describe a different address
> during the controller lifetime. The classic example would be
> a discovery-service dns name that may be load-balanced via dns over
> multiple addresses.
> 
> So try to re-resolve the controller address. There is no dns query
> in case the traddr provided is an ip address.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/tcp.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Re: [PATCH rfc 4/6] nvme-rdma: use nvmf_resolve_address helper

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/rdma.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index d433b2ec07a6..797bd192a413 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2230,8 +2230,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   		opts->mask |= NVMF_OPT_TRSVCID;
>   	}
>   
> -	ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
> -			opts->traddr, opts->trsvcid, &ctrl->addr);
> +	ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
>   	if (ret) {
>   		pr_err("malformed address passed: %s:%s\n",
>   			opts->traddr, opts->trsvcid);

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Re: [PATCH rfc 5/6] nvme-rdma: re-resolve traddr upon reconnect in case it is a dns name

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> In case traddr is a dns name, it may describe a different address
> during the controller lifetime. The classic example would be
> a discovery-service dns name that may be load-balanced via dns over
> multiple addresses.
> 
> So try to re-resolve the controller address. There is no dns query
> in case the traddr provided is an ip address.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/rdma.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> userspace needs to know that the kernel is capable of doing a dns
> query, so expose a custom opt called "dns_ip_traddr" which is not
> an argument, but rather indication of support for dns names for
> ip based traddr.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/fabrics.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index fadd25538d1b..6a7e8cbdaf69 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -1331,6 +1331,7 @@ static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)
>   		seq_puts(seq_file, ",");
>   		seq_puts(seq_file, tok->pattern);
>   	}
> +	seq_puts(seq_file, ",dns_ip_traddr");
>   	seq_puts(seq_file, "\n");
>   }
>   

Don't we need to update the parser to accept this option, too?
After all, the implication is that all tokens in the output can be used 
as valid tokens for the connect string ...

Cheers,

Hannes

Re: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Hannes Reinecke]

On 7/24/23 11:20, Sagi Grimberg wrote:
> Currently nvme-cli is able to resolve an ip address from a dns name if
> traddr argument was given as a dns name, so it does that and passes down
> to the kernel the resolved ip address.
> 
> However, if the dns resolution changes over time, the kernel will not
> be able to reconnect because it has a stale ip address. While this is
> not a real use case for nvme subsystems, it could be a valid use-case
> for discovery controllers, where the resolution is made from a dns
> name that may load-balance/failover over multiple endpoints.
> 
> The last patch (7/6) is for libnvme to pass down the traddr as a dns
> name if the kernel supports dns queries.
> 
> The inspiration was taken from the nfs client that does this for
> redirects or locations discovery.
> 
> It is an rfc, so feedback is welcome.
> 
> Sagi Grimberg (6):
>    nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name
>    nvme-tcp: use nvmf_resolve_address helper
>    nvme-tcp: re-resolve traddr upon reconnect in case it is a dns name
>    nvme-rdma: use nvmf_resolve_address helper
>    nvme-rdma: re-resolve traddr upon reconnect in case it is a dns name
>    nvme-fabrics: expose support for traddr as dns names to userspace
> 
>   drivers/nvme/host/fabrics.c | 29 +++++++++++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |  2 ++
>   drivers/nvme/host/rdma.c    | 21 +++++++++++++--------
>   drivers/nvme/host/tcp.c     | 21 +++++++++++++--------
>   4 files changed, 57 insertions(+), 16 deletions(-)
> 
In principle looks good, but what about target support?

Cheers,

Hannes

Re: [PATCH rfc 1/6] nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name

[Sagi Grimberg]

> On 7/24/23 11:20, Sagi Grimberg wrote:
>> Allow userspace to pass in traddr as a dns name as well. This may be
>> useful for a discovery service that may have a dns name, that may also
>> change in time (with an updated dns record).
>>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/host/fabrics.c | 28 ++++++++++++++++++++++++++++
>>   drivers/nvme/host/fabrics.h |  2 ++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 8175d49f2909..fadd25538d1b 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/mutex.h>
>>   #include <linux/parser.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/dns_resolver.h>
>>   #include "nvme.h"
>>   #include "fabrics.h"
>> @@ -1133,6 +1134,33 @@ bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>>   }
>>   EXPORT_SYMBOL_GPL(nvmf_ip_options_match);
>> +int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
>> +        struct sockaddr_storage *traddr)
>> +{
>> +    char *traddr_str = NULL;
>> +    int traddr_len;
>> +    int ret;
>> +
>> +    /* first check if traddr is ipv4/ipv6 address */
>> +    ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
>> +            opts->traddr, opts->trsvcid, traddr);
>> +    if (!ret)
>> +        return ret;
>> +
>> +    /* second check if traddr is dns name */
>> +    traddr_len = dns_query(&init_net, NULL, opts->traddr,
>> +            strlen(opts->traddr), NULL, &traddr_str,
>> +            NULL, false);
>> +    if (traddr_len < 0)
>> +        return traddr_len;
>> +
>> +    ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
>> +            traddr_str, opts->trsvcid, traddr);
>> +    kfree(traddr_str);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(nvmf_resolve_ip_address);
>> +
>>   static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
>>           unsigned int allowed_opts)
>>   {
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 82e7a27ffbde..603d8d176168 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -222,6 +222,8 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char 
>> *buf, int size);
>>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>>           struct nvmf_ctrl_options *opts);
>> +int nvmf_resolve_ip_address(struct nvmf_ctrl_options *opts,
>> +        struct sockaddr_storage *traddr);
>>   void nvmf_set_io_queues(struct nvmf_ctrl_options *opts, u32 
>> nr_io_queues,IS
>>               u32 io_queues[HCTX_MAX_TYPES]);
>>   void nvmf_map_queues(struct blk_mq_tag_set *set, struct nvme_ctrl 
>> *ctrl,
> 
> dns resolver is a config option, so we would need either a '#ifdef 
> DNS_RESOLVER' or if (IS_ENABLED(CONFIG_DNS_RESOLVER))' somewhere ...

Probably will be hidden in nvmf_resolve_ip_address, which will else
just perform inet_pton_with_scope...

Re: [PATCH rfc 2/6] nvme-tcp: use nvmf_resolve_address helper

[Sagi Grimberg]

>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/host/tcp.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 3e7dd6f91832..a47b597ace3e 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2537,8 +2537,7 @@ static struct nvme_ctrl 
>> *nvme_tcp_create_ctrl(struct device *dev,
>>           opts->mask |= NVMF_OPT_TRSVCID;
>>       }
>> -    ret = inet_pton_with_scope(&init_net, AF_UNSPEC,
>> -            opts->traddr, opts->trsvcid, &ctrl->addr);
>> +    ret = nvmf_resolve_ip_address(opts, &ctrl->addr);
>>       if (ret) {
>>           pr_err("malformed address passed: %s:%s\n",
>>               opts->traddr, opts->trsvcid);
> 
> Merge with the first patch?
> Otherwise okay.

I try not to spray changes in both core/fabrics and in
the drivers... We can do either way...

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index fadd25538d1b..6a7e8cbdaf69 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -1331,6 +1331,7 @@ static void __nvmf_concat_opt_tokens(struct 
>> seq_file *seq_file)
>>           seq_puts(seq_file, ",");
>>           seq_puts(seq_file, tok->pattern);
>>       }
>> +    seq_puts(seq_file, ",dns_ip_traddr");
>>       seq_puts(seq_file, "\n");
>>   }
> 
> Don't we need to update the parser to accept this option, too?
> After all, the implication is that all tokens in the output can be used 
> as valid tokens for the connect string ...

I struggled with this bit here...

Today, we simply expose all valid tokens, but in this case
what we really need is a capability indicator (in this case
traddr can be a fqdn and not only an ip address).

So we could make this a parsed token, and make userspace
actually pass it in case traddr was passed as a fqdn, but
it is absolutely redundant to do so.

We could come up with a format that explicitly says this
is a capability instead of a valid token.
Like: "cap=dns_ip_traddr" or "format=traddr_as_dns_name" or
something...

What we just need, is for userspace to know if it is possible
to pass traddr in a specific format.

I tend to think that this is something that we will want for
other options that can have multiple formats.

Another option would be to make this a separate token and
call it ctrl_dns_name that would be separate from traddr, but:
- that will cause the changes a spray of changes that would accept
   an alternative to traddr, and validate that at least one exist, but
   fail if both are given
- bubble up the usage to userspace, leaving the admin with the
   exact format of the traddr
- keep track of this when doing reconnects and stuff.

So I figured that the current approach would be preferable if
we accept how to expose that for a given kernel/driver, we can
accept dns names, or we cannot.

What do others think? Christoph? Keith? Chaitanya?

Re: [PATCH rfc 0/6] support traddr as dns names for ip based transports

[Sagi Grimberg]

>> Currently nvme-cli is able to resolve an ip address from a dns name if
>> traddr argument was given as a dns name, so it does that and passes down
>> to the kernel the resolved ip address.
>>
>> However, if the dns resolution changes over time, the kernel will not
>> be able to reconnect because it has a stale ip address. While this is
>> not a real use case for nvme subsystems, it could be a valid use-case
>> for discovery controllers, where the resolution is made from a dns
>> name that may load-balance/failover over multiple endpoints.
>>
>> The last patch (7/6) is for libnvme to pass down the traddr as a dns
>> name if the kernel supports dns queries.
>>
>> The inspiration was taken from the nfs client that does this for
>> redirects or locations discovery.
>>
>> It is an rfc, so feedback is welcome.
>>
>> Sagi Grimberg (6):
>>    nvme-fabrics: add helper to resolve ipv4/ipv6 or dns name
>>    nvme-tcp: use nvmf_resolve_address helper
>>    nvme-tcp: re-resolve traddr upon reconnect in case it is a dns name
>>    nvme-rdma: use nvmf_resolve_address helper
>>    nvme-rdma: re-resolve traddr upon reconnect in case it is a dns name
>>    nvme-fabrics: expose support for traddr as dns names to userspace
>>
>>   drivers/nvme/host/fabrics.c | 29 +++++++++++++++++++++++++++++
>>   drivers/nvme/host/fabrics.h |  2 ++
>>   drivers/nvme/host/rdma.c    | 21 +++++++++++++--------
>>   drivers/nvme/host/tcp.c     | 21 +++++++++++++--------
>>   4 files changed, 57 insertions(+), 16 deletions(-)
>>
> In principle looks good, but what about target support?

I don't see what the target needs to support here?

Just place nvmet ports behind say nginx with a load-balancer, and
voila...

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

On 8/13/23 16:07, Sagi Grimberg wrote:
> 
>>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>>> index fadd25538d1b..6a7e8cbdaf69 100644
>>> --- a/drivers/nvme/host/fabrics.c
>>> +++ b/drivers/nvme/host/fabrics.c
>>> @@ -1331,6 +1331,7 @@ static void __nvmf_concat_opt_tokens(struct 
>>> seq_file *seq_file)
>>>           seq_puts(seq_file, ",");
>>>           seq_puts(seq_file, tok->pattern);
>>>       }
>>> +    seq_puts(seq_file, ",dns_ip_traddr");
>>>       seq_puts(seq_file, "\n");
>>>   }
>>
>> Don't we need to update the parser to accept this option, too?
>> After all, the implication is that all tokens in the output can be 
>> used as valid tokens for the connect string ...
> 
> I struggled with this bit here...
> 
> Today, we simply expose all valid tokens, but in this case
> what we really need is a capability indicator (in this case
> traddr can be a fqdn and not only an ip address).
> 
> So we could make this a parsed token, and make userspace
> actually pass it in case traddr was passed as a fqdn, but
> it is absolutely redundant to do so.
> 
> We could come up with a format that explicitly says this
> is a capability instead of a valid token.
> Like: "cap=dns_ip_traddr" or "format=traddr_as_dns_name" or
> something...
> 
> What we just need, is for userspace to know if it is possible
> to pass traddr in a specific format.
> 
> I tend to think that this is something that we will want for
> other options that can have multiple formats.
> 
> Another option would be to make this a separate token and
> call it ctrl_dns_name that would be separate from traddr, but:
> - that will cause the changes a spray of changes that would accept
>    an alternative to traddr, and validate that at least one exist, but
>    fail if both are given
> - bubble up the usage to userspace, leaving the admin with the
>    exact format of the traddr
> - keep track of this when doing reconnects and stuff.
> 
> So I figured that the current approach would be preferable if
> we accept how to expose that for a given kernel/driver, we can
> accept dns names, or we cannot.
> 
> What do others think? Christoph? Keith? Chaitanya?

Anyone has feedback here?

Seems that we want this, but the question is how to expose
a new format of traddr (now can be a dns name) to userspace?

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Hannes Reinecke]

On 8/17/23 11:11, Sagi Grimberg wrote:
> 
> 
> On 8/13/23 16:07, Sagi Grimberg wrote:
>>
[ .. ]
>> So I figured that the current approach would be preferable if
>> we accept how to expose that for a given kernel/driver, we can
>> accept dns names, or we cannot.
>>
>> What do others think? Christoph? Keith? Chaitanya?
> 
> Anyone has feedback here?
> 
> Seems that we want this, but the question is how to expose
> a new format of traddr (now can be a dns name) to userspace?

Or have a new token 'traddr_dns' (instaed of 'traddr'), which would
hold the dnsname instead of the ip address.
And reject inputs where both are set.
Hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

> [ .. ]
>>> So I figured that the current approach would be preferable if
>>> we accept how to expose that for a given kernel/driver, we can
>>> accept dns names, or we cannot.
>>>
>>> What do others think? Christoph? Keith? Chaitanya?
>>
>> Anyone has feedback here?
>>
>> Seems that we want this, but the question is how to expose
>> a new format of traddr (now can be a dns name) to userspace?
> 
> Or have a new token 'traddr_dns' (instaed of 'traddr'), which would
> hold the dnsname instead of the ip address.
> And reject inputs where both are set.
> Hmm?

I listed that as an option, you should read it.
There are some disadvantages in doing that, it needs to co-exist
with the mandatory (today) traddr, which is now will be mandatory
only for fc. And also now the user will need to decide a conflicting
existing functionality in nvme-cli that today accept dns names as
a format to traddr, but now will need a different argument for
passing it to the kernel.

Anyway, its possible, but more complicated IMO.

Plus, I also think that it can be useful for the kernel to signal
capabilities and support for things that are not only valid tokens.

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Hannes Reinecke]

On 8/17/23 12:37, Sagi Grimberg wrote:
> 
>> [ .. ]
>>>> So I figured that the current approach would be preferable if
>>>> we accept how to expose that for a given kernel/driver, we can
>>>> accept dns names, or we cannot.
>>>>
>>>> What do others think? Christoph? Keith? Chaitanya?
>>>
>>> Anyone has feedback here?
>>>
>>> Seems that we want this, but the question is how to expose
>>> a new format of traddr (now can be a dns name) to userspace?
>>
>> Or have a new token 'traddr_dns' (instaed of 'traddr'), which would
>> hold the dnsname instead of the ip address.
>> And reject inputs where both are set.
>> Hmm?
> 
> I listed that as an option, you should read it.
> There are some disadvantages in doing that, it needs to co-exist
> with the mandatory (today) traddr, which is now will be mandatory
> only for fc. And also now the user will need to decide a conflicting
> existing functionality in nvme-cli that today accept dns names as
> a format to traddr, but now will need a different argument for
> passing it to the kernel.
> 
> Anyway, its possible, but more complicated IMO.
> 
> Plus, I also think that it can be useful for the kernel to signal
> capabilities and support for things that are not only valid tokens.

I cross-checked with nvme-cli; there we read the list of available 
tokens upon failure to give the user a hint which arguments are 
supported by any given kernel.
So by that regard it should be fine to just add 'dns' as a parameter
here.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

>> I listed that as an option, you should read it.
>> There are some disadvantages in doing that, it needs to co-exist
>> with the mandatory (today) traddr, which is now will be mandatory
>> only for fc. And also now the user will need to decide a conflicting
>> existing functionality in nvme-cli that today accept dns names as
>> a format to traddr, but now will need a different argument for
>> passing it to the kernel.
>>
>> Anyway, its possible, but more complicated IMO.
>>
>> Plus, I also think that it can be useful for the kernel to signal
>> capabilities and support for things that are not only valid tokens.
> 
> I cross-checked with nvme-cli; there we read the list of available 
> tokens upon failure to give the user a hint which arguments are 
> supported by any given kernel.
> So by that regard it should be fine to just add 'dns' as a parameter
> here.
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Thanks,

I'd also want to hear from others (Keith, Christoph, Chaitanya, Daniel,
Martin...)

Where do you stand on the kernel exposing capabilities that do not
manifest in "valid tokens" when writing to /dev/nvme-fabrics ?

today when we read /dev/nvme-fabrics we get a string with all
the valid tokens. that is useful for userspace to understand what
it can or cannot pass in a fabrics connection string.

In the example here, we are trying to introduce a new format for
traddr argument that is now supported by the fabrics driver (tcp/rdma)
where traddr can now be ipv4, ipv6 or a dns name (where before this
patchset it could only be ipv4 or ipv6).

The question is:
Do we enforce that every new capability must come with a matching valid
token? Or do we want to expose capabilities that are not necessarily
come with dedicated tokens (like in this example)?

I think the latter is a be better approach. But could be convinced
otherwise...

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Daniel Wagner]

On Thu, Aug 17, 2023 at 02:09:39PM +0300, Sagi Grimberg wrote:
> Where do you stand on the kernel exposing capabilities that do not
> manifest in "valid tokens" when writing to /dev/nvme-fabrics ?
>
> today when we read /dev/nvme-fabrics we get a string with all
> the valid tokens. that is useful for userspace to understand what
> it can or cannot pass in a fabrics connection string.
> 
> In the example here, we are trying to introduce a new format for
> traddr argument that is now supported by the fabrics driver (tcp/rdma)
> where traddr can now be ipv4, ipv6 or a dns name (where before this
> patchset it could only be ipv4 or ipv6).
> 
> The question is:
> Do we enforce that every new capability must come with a matching valid
> token? Or do we want to expose capabilities that are not necessarily
> come with dedicated tokens (like in this example)?
> 
> I think the latter is a be better approach. But could be convinced
> otherwise...

My gut feeling tells me in the long run it's good to have some generic
way to inform userspace of options format or feature changes. I am
pondering if sysfs wouldn't be the better place for this. But then we
would need to gather this info from two places. Not nice.

Just to through in another idea, we could also introduce a version
scheme, e.g.

...,traddr=%s@v2,...

If I got this right, the current libnvme token parser would just ignore
the versioning string.

Obviously, the simplest way is to introduce just a new token and be done
with it. I somewhat don't like it. Maybe I am trying to over engineer
this.

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

>> Where do you stand on the kernel exposing capabilities that do not
>> manifest in "valid tokens" when writing to /dev/nvme-fabrics ?
>>
>> today when we read /dev/nvme-fabrics we get a string with all
>> the valid tokens. that is useful for userspace to understand what
>> it can or cannot pass in a fabrics connection string.
>>
>> In the example here, we are trying to introduce a new format for
>> traddr argument that is now supported by the fabrics driver (tcp/rdma)
>> where traddr can now be ipv4, ipv6 or a dns name (where before this
>> patchset it could only be ipv4 or ipv6).
>>
>> The question is:
>> Do we enforce that every new capability must come with a matching valid
>> token? Or do we want to expose capabilities that are not necessarily
>> come with dedicated tokens (like in this example)?
>>
>> I think the latter is a be better approach. But could be convinced
>> otherwise...
> 
> My gut feeling tells me in the long run it's good to have some generic
> way to inform userspace of options format or feature changes. I am
> pondering if sysfs wouldn't be the better place for this.

You would need a global entry for that, its not controller/subsystem
specific. For that we have the misc device...

> But then we
> would need to gather this info from two places. Not nice.
> 
> Just to through in another idea, we could also introduce a version
> scheme, e.g.
> 
> ...,traddr=%s@v2,...
> 
> If I got this right, the current libnvme token parser would just ignore
> the versioning string.

I don't think that this is a elegant option, there would need for some
documentation entry for what is the versioning stuff. Plus, a
feature/capability may not even relate directly to an existing token
necessarily (if we generalize this).

> 
> Obviously, the simplest way is to introduce just a new token and be done
> with it. I somewhat don't like it. Maybe I am trying to over engineer
> this.

It could be a form of token that can emphasize its not a token like
",feat=<feat_name>" or "cap=<cap_name>"...

But this would definitely need to be done in a way that is compatible
with userspace moving forward.

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Daniel Wagner]

On Sun, Aug 20, 2023 at 01:55:47PM +0300, Sagi Grimberg wrote:
> > My gut feeling tells me in the long run it's good to have some generic
> > way to inform userspace of options format or feature changes. I am
> > pondering if sysfs wouldn't be the better place for this.
> 
> You would need a global entry for that, its not controller/subsystem
> specific. For that we have the misc device...

Ah, that explains why we have this interface after all.

> > ...,traddr=%s@v2,...
> > 
> > If I got this right, the current libnvme token parser would just ignore
> > the versioning string.
> 
> I don't think that this is a elegant option, there would need for some
> documentation entry for what is the versioning stuff. Plus, a
> feature/capability may not even relate directly to an existing token
> necessarily (if we generalize this).

I agree. It is clumsy.

> > Obviously, the simplest way is to introduce just a new token and be done
> > with it. I somewhat don't like it. Maybe I am trying to over engineer
> > this.
> 
> It could be a form of token that can emphasize its not a token like
> ",feat=<feat_name>" or "cap=<cap_name>"...
> 
> But this would definitely need to be done in a way that is compatible
> with userspace moving forward.

The libnvme parser is ignore anything it doesn't know yet. Introducing
'feat' or 'cap' is not a problem at all. Also it should be fairly simple
to add. The resulting code is likely to be very similar too; testing if
feat/cap or dns_ip_addr token is available and then use the dns name.

Given this, I am in favor of the feat/cap approach.

Re: [PATCH rfc 6/6] nvme-fabrics: expose support for traddr as dns names to userspace

[Sagi Grimberg]

> The libnvme parser is ignore anything it doesn't know yet. Introducing
> 'feat' or 'cap' is not a problem at all. Also it should be fairly simple
> to add. The resulting code is likely to be very similar too; testing if
> feat/cap or dns_ip_addr token is available and then use the dns name.
> 
> Given this, I am in favor of the feat/cap approach.

I do still want to hear others. We have thumbs up from Daniel and
Hannes.