[PATCH v2] nvme-tcp: Print actual source IP address in address attribute

[Belanger, Martin]

> -----Original Message-----
> From: Sagi Grimberg <sagi at grimberg.me>
> Sent: Thursday, September 1, 2022 7:03 AM
> To: Martin Belanger; linux-nvme at lists.infradead.org
> Cc: kbusch at kernel.org; hare at suse.de; axboe at fb.com; hch at lst.de; Belanger,
> Martin
> Subject: Re: [PATCH v2] nvme-tcp: Print actual source IP address in address
> attribute
> 
> 
> [EXTERNAL EMAIL]
> 
> 
> 
> On 8/31/22 22:38, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger at dell.com>
> >
> > TCP transport relies on the routing table to determine which source
> > address and interface to use when making a connection.
> > Currently, there is no way to tell from userspace where a connection
> > was made. This patch allows exposing the actual source address using
> > the host_traddr field of the address attribute.
> >
> > This is needed to diagnose and identify connectivity issues since it
> > will allow us to tell which interface and source address is associated
> > with each connection.
> >
> > Signed-off-by: Martin Belanger <martin.belanger at dell.com>
> > ---
> >   drivers/nvme/host/tcp.c | 34 +++++++++++++++++++++++++++++++++-
> >   1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index
> > 044da18c06f5..adbb76483a74 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -165,6 +165,7 @@ struct nvme_tcp_ctrl {
> >   	struct blk_mq_tag_set	admin_tag_set;
> >   	struct sockaddr_storage addr;
> >   	struct sockaddr_storage src_addr;
> > +	struct sockaddr_storage sock_addr;
> >   	struct nvme_ctrl	ctrl;
> >
> >   	struct work_struct	err_work;
> > @@ -1597,6 +1598,12 @@ static int nvme_tcp_alloc_queue(struct
> nvme_ctrl *nctrl,
> >   		goto err_rcv_pdu;
> >   	}
> >
> > +	/* Retrieve the actual source address from the socket */
> > +	ret = kernel_getsockname(queue->sock,
> > +		(struct sockaddr *)&ctrl->sock_addr);
> > +	if (ret < 0)
> > +		memset(&ctrl->sock_addr, 0, sizeof(ctrl->sock_addr));
> > +
> 
> For every queue?

Hi Sagi. Thanks for reviewing this. 

I see the error of my ways. I will remove this and instead invoke kernel_getsockname() from nvme_tcp_get_address() below. And it will only apply to queue[0].

> 
> >   	ret = nvme_tcp_init_connection(queue);
> >   	if (ret)
> >   		goto err_init_connect;
> > @@ -2532,6 +2539,31 @@ static int nvme_tcp_poll(struct blk_mq_hw_ctx
> *hctx, struct io_comp_batch *iob)
> >   	return queue->nr_cqe;
> >   }
> >
> > +static int nvme_tcp_get_address(struct nvme_ctrl *ctrl, char *buf,
> > +int size) {
> > +	int len = 0;
> > +	struct sockaddr_storage *sa = &to_tcp_ctrl(ctrl)->sock_addr;
> 
> You can just retrieve the sock_addr from the admin queue here locally. I don't
> think we need a struct member for it.

Got it. Will do.

> 
> > +
> > +	if (ctrl->opts->mask & NVMF_OPT_TRADDR)
> > +		len += scnprintf(buf, size, "traddr=%s", ctrl->opts->traddr);
> > +	if (ctrl->opts->mask & NVMF_OPT_TRSVCID)
> > +		len += scnprintf(buf + len, size - len, "%strsvcid=%s",
> > +				(len) ? "," : "", ctrl->opts->trsvcid);
> > +	if ((sa->ss_family == AF_INET) || (sa->ss_family == AF_INET6)) {
> > +		len += scnprintf(buf + len, size - len, "%shost_traddr=%pISc",
> > +				(len) ? "," : "", sa);
> 
> It is confusing to expose that if it was not specified, that is creating a different
> behavior from the existing behavior and from other transports.

I asked Hannes first. He recommended using the host_traddr key because it is already recognized by nvme-cli. If you think we should define a new key, e.g. "src_addr", I can do that too.

On the other hand, if you think that exposing the source address through the "address" attribute is not the best approach, we could introduce a new attribute instead (e.g. "src_address"). However, this is a TCP-only attribute, and there is currently no framework to expose attributes directly from the tcp.c module. This will require a lot of new code and I was looking for a solution that requires the least amount of changes.

Let me know what you prefer.

Thanks,
Martin
> 
> > +	} else if (ctrl->opts->mask & NVMF_OPT_HOST_TRADDR) {
> > +		len += scnprintf(buf + len, size - len, "%shost_traddr=%s",
> > +				(len) ? "," : "", ctrl->opts->host_traddr);
> > +	}
> > +	if (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)
> > +		len += scnprintf(buf + len, size - len, "%shost_iface=%s",
> > +				(len) ? "," : "", ctrl->opts->host_iface);
> > +	len += scnprintf(buf + len, size - len, "\n");
> > +
> > +	return len;
> > +}
> > +
> >   static const struct blk_mq_ops nvme_tcp_mq_ops = {
> >   	.queue_rq	= nvme_tcp_queue_rq,
> >   	.commit_rqs	= nvme_tcp_commit_rqs,
> > @@ -2563,7 +2595,7 @@ static const struct nvme_ctrl_ops
> nvme_tcp_ctrl_ops = {
> >   	.free_ctrl		= nvme_tcp_free_ctrl,
> >   	.submit_async_event	= nvme_tcp_submit_async_event,
> >   	.delete_ctrl		= nvme_tcp_delete_ctrl,
> > -	.get_address		= nvmf_get_address,
> > +	.get_address		= nvme_tcp_get_address,
> >   	.stop_ctrl		= nvme_tcp_stop_ctrl,
> >   };
> >