Re: [PATCH v3] nvme-tcp: Print actual source IP address through sysfs "address" attr

From: Sagi Grimberg <sagi@grimberg.me>
To: "Belanger, Martin" <Martin.Belanger@dell.com>,
	Martin Belanger <nitram_67@hotmail.com>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc: "kbusch@kernel.org" <kbusch@kernel.org>,
	"hare@suse.de" <hare@suse.de>, "axboe@fb.com" <axboe@fb.com>,
	"hch@lst.de" <hch@lst.de>
Subject: Re: [PATCH v3] nvme-tcp: Print actual source IP address through sysfs "address" attr
Date: Tue, 6 Sep 2022 16:08:11 +0300	[thread overview]
Message-ID: <c9399436-3d45-90b0-731a-6baa0605f918@grimberg.me> (raw)
In-Reply-To: <SJ0PR19MB45440F0F979FB14315AE6B4DF27E9@SJ0PR19MB4544.namprd19.prod.outlook.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 exposes the actual source address using a new field named
>>> "src_addr=" in the "address" attribute.
>>>
>>> This is needed to diagnose and identify connectivity issues. With the
>>> source address we can infer the interface associated with each
>>> connection.
>>>
>>> This was tested with nvme-cli 2.0 to verify it does not have any
>>> adverse effect. The new "src_addr=" field will simply be displayed in
>>> the output of the "list-subsys" or "list -v" commands as shown here.
>>>
>>> $ nvme list-subsys
>>> nvme-subsys0 - NQN=nqn.2014-08.org.nvmexpress.discovery
>>> \
>>>    +- nvme0 tcp
>>> traddr=192.168.56.1,trsvcid=8009,src_addr=192.168.56.101 live
>>
>> I wonder how this works with udev events etc for auto-connect? and also for
>> uniqueness checks...
> 
> Udev events are triggered by device properties (which are not the same as attributes).
> The properties are found in the attribute "uevent". For example:
> 
> $ cat /sys/class/nvme/nvme1/uevent
> MAJOR=238
> MINOR=1
> DEVNAME=nvme1
> NVME_TRTYPE=tcp
> NVME_TRADDR=::ffff:192.168.56.1
> NVME_TRSVCID=8009
> NVME_HOST_TRADDR=none
> NVME_HOST_IFACE=enp0s8
> 
> The change I'm proposing does not affect device properties and therefore no impact on udev rules.
> 
> With regards to uniqueness checks, there is only one place in libnvme where we read and check the "address" attribute (file: tree.c, function: nvme_ctrl_alloc). The code will simply skip "src_address=" and extract all the other known keys from "address".

I'm concerned that adding this unconditionally may trigger someone to
fail due to this addition... It is also confusing when host_traddr is
provided...

If this would be logged as pr_dbg instead, would this achieve the goal?