[PATCH 3/4] nvme: implement 'discovery' sysfs entry and discovery uevents

[hare@suse.de (Hannes Reinecke)]

On 08/22/2018 09:23 AM, Hannes Reinecke wrote:
> On 08/21/2018 11:01 PM, James Smart wrote:
>>
>>
>> On 8/21/2018 6:43 AM, Hannes Reinecke wrote:
>>> When establishing a connection to the discovery controller with the
>>> 'async-connect' option the cli has no means of actually getting the
>>> discovery log page entries.
>>> This patch implements a 'discovery' sysfs attribute for the controller
>>> which holds the information of the discovery log page. Additionally
>>> an uevent is generated for each discovery log page entry.
>>>
>>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>>> ---
>>> ? drivers/nvme/host/core.c | 163
>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>> ? drivers/nvme/host/nvme.h |?? 1 +
>>> ? 2 files changed, 163 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index dd8ec1dd9219..358be6d217d9 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -2325,6 +2325,133 @@ static int nvme_get_effects_log(struct
>>> nvme_ctrl *ctrl)
>>> ????? return ret;
>>> ? }
>>> ? +static int nvme_get_discovery_log(struct nvme_ctrl *ctrl)
>>> +{
>>> +??? int ret, disc_log_len = 4096;
>>> +
>>> +retry:
>>> +??? if (!ctrl->discovery)
>>> +??????? ctrl->discovery = kzalloc(disc_log_len, GFP_KERNEL);
>>> +
>>> +??? if (!ctrl->discovery)
>>> +??????? return 0;
>>> +
>>> +??? ret = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_DISC, 0,
>>> +?????????????? ctrl->discovery, disc_log_len, 0);
>>> +??? if (ret) {
>>> +??????? kfree(ctrl->discovery);
>>> +??????? ctrl->discovery = NULL;
>>> +??????? return ret;
>>> +??? }
>>> +??? if (ctrl->discovery->numrec > 3 && disc_log_len == 4096) {
>>> +??????? disc_log_len = (ctrl->discovery->numrec + 1) * 1024;
>>> +??????? kfree(ctrl->discovery);
>>> +??????? ctrl->discovery = NULL;
>>> +??????? goto retry;
>>
>> This is missing logic to detect a change in GENCTR and restart the log
>> read.
>>
> Yeah, I'll be implementing this for the next round.
> 
>>> +??? }
>>> +??? return ret;
>>> +}
>>> +
>>> ...
>>> +static int nvme_configure_discovery(struct nvme_ctrl *ctrl)
>>> +{
>>> +??? int ret = 0;
>>> +
>>> +??? if (!ctrl->opts || !ctrl->opts->discovery_nqn)
>>> +??????? return 0;
>>> +
>>> +??? if (!ctrl->opts->async_connect)
>>> +??????? return 0;
>>
>> I don't understand why async_connect parameter means anything in this
>> context...? why is this looked at?
>>
>> is it strictly to avoid delay while reading the discovery log ?? I don't
>> know that waiting for yet another command or two is a big deal, so I
>> would think just proceeding with reading the discovery log is fine.
>>
> Originally I did that so as to restore the original functionality, and
> tie discovery events to the use of the 'async_connect' parameter.
> But you are right, both are not necessarily connected.
> I'll drop this for the next round.
> 
>>> +
>>> +??? ret = nvme_get_discovery_log(ctrl);
>>> +??? if (!ret && ctrl->discovery) {
>>> +??????? int i;
>>> +??????? struct nvmf_disc_rsp_page_entry *entry =
>>> +??????????? ctrl->discovery->entries;
>>> +
>>> +??????? for (i = 0; i < ctrl->discovery->numrec; i++) {
>>> +??????????? nvme_discovery_uevent(ctrl, entry);
>>> +??????????? entry++;
>>> +??????? }
>>> +??? }
>>> +??? return ret;
>>> +}
>>> +
>>> ? /*
>>> ?? * Initialize the cached copies of the Identify data and various
>>> controller
>>> ?? * register in our nvme_ctrl structure.? This should be called as
>>> soon as
>>> @@ -2491,6 +2618,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>>> ????? if (ret < 0)
>>> ????????? return ret;
>>> ? +??? ret = nvme_configure_discovery(ctrl);
>>> +??? if (ret < 0)
>>> +??????? return ret;
>>> +
>>> ????? ctrl->identified = true;
>>> ? ????? return 0;
>>> @@ -2825,6 +2956,32 @@ static ssize_t nvme_sysfs_show_address(struct
>>> device *dev,
>>> ? }
>>> ? static DEVICE_ATTR(address, S_IRUGO, nvme_sysfs_show_address, NULL);
>>> ? +static ssize_t nvme_discovery_show(struct device *dev,
>>> +?????????????????? struct device_attribute *attr,
>>> +?????????????????? char *buf)
>>> +{
>>> +??? struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>> +??? struct nvmf_disc_rsp_page_entry *entry;
>>> +??? int i;
>>> +??? ssize_t len = 0;
>>> +
>>> +??? if (!ctrl->discovery)
>>> +??????? return -EINVAL;
>>> +
>>> +??? entry = ctrl->discovery->entries;
>>> +??? for (i = 0; i < ctrl->discovery->numrec; i++) {
>>> +??????? len += snprintf(buf, PAGE_SIZE,
>>> +??????????????? "trtype %d adrfam %d traddr %s "
>>> +??????????????? "trsvcid %s portid %d subnqn %s\n",
>>> +??????????????? entry->trtype, entry->adrfam, entry->traddr,
>>> +??????????????? entry->trsvcid, le16_to_cpu(entry->portid),
>>> +??????????????? entry->subnqn);
>>> +??????? entry++;
>>> +??? }
>>> +??? return len;
>>> +}
>>> +static DEVICE_ATTR(discovery, S_IRUGO, nvme_discovery_show, NULL);
>>> +
>>
>> I really don't like that it's reporting a cached log. ? It may be
>> completely inconsistent with what's on the discovery controller now
>> (consider a nvmetcli done then a reconfig done). ?? There may not even
>> be connectivity to the discovery controller at the time the sysfs entry
>> is read - so validity to the content is in question.? I would much
>> rather have the attribute read the log then display contents - so it's
>> always current.??? Of course that does make it interesting if you read
>> it while the controller is paused doing a reconnect.
>>
> For the next round I'll split this patch in two, one for getting the
> discovery log and issueing uevents, and a next onw for the sysfs attribute.
> After all, I've implemented the sysfs attribute just for completeness,
> to show the contents of the internal discovery buffer. Re-reading it
> when reading kind of defeats that purpose IMO.
> 
Actually, I think it would be better to make 'discovery' a _bin_ attribute.
With that 'nvme cli' could parse it directly without having to implement
yet another parser, we don't have this awkward multiple entry sysfs
attribute, and would actually provide additional value.

I'll give it a go.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)