Re: [SPDK] Allow attachment to an already probed controller

[Walker, Benjamin <benjamin.walker at intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4837 bytes --]

On Fri, 2016-02-26 at 21:18 +0300, Andrey Kuzmin wrote:
> On Fri, Feb 26, 2016 at 9:08 PM, Verkamp, Daniel
> <daniel.verkamp(a)intel.com> wrote:
> > Hi Andrey,
> > 
> > I am not sure what behavior you would like to see from probe when
> > calling it again after attaching some controllers.  We can't re-
> > probe/re-attach the same controller twice without detaching it in
> > between - if it is already attached, that means the driver is
> > already running, queues are initialized, etc. and we should not
> > call the user's attach callback again.
> > 
> > The spdk_nvme_probe() interface operates at the controller (PCIe
> > device) level; namespace enumeration should be handled in the
> > application layer in or after the attach callback.
> > 
> > Can you clarify your desired behavior/use case in your app?
> > 
> 
> To my understanding, the goal of the probe(), from the user's
> standpoint, is to get hold of the controller reference (and it's
> basically the only way to accomplish this under the current API). If
> I'm parsing multiple PCI inputs, the probe() semantics forces me into
> the probed controller housekeeping on my side. As probe() (one can
> safely assume) occurs in the initialization phase, it should be safe
> to return the controller references back to the user even if the
> controller has already been started, at user's discretion.
> 
> > We can possibly add another interface to enumerate all of the NVMe
> > controllers that are currently attached to the userspace driver
> > (the ones in the attached_ctrlrs list) if that would help.
> > 
> 
> That's pretty much what I have suggested.

If we add an API to give you a list of all of the currently attached
controllers, does that solve the problem? Are there any other issues
beyond that one?

> 
> Regards,
> Andrey
> 
> > Thanks,
> > -- Daniel
> > 
> > P.S. I believe the return value from PCI enumeration is already
> > handled; the intent is that even if nvme_pci_enumerate() returns a
> > failure code, it may have found some controllers, and we want to
> > initialize those even if other PCI functions failed to probe.  The
> > return code is returned intact at the end of spdk_nvme_probe() so
> > that the application can determine that an error occurred and take
> > action if necessary.
> > 
> > -----Original Message-----
> > From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Andrey
> > Kuzmin
> > Sent: Friday, February 26, 2016 10:52 AM
> > To: spdk(a)lists.01.org
> > Subject: [SPDK] Allow attachment to an already probed controller
> > 
> > While I appreciatethe reasoning behind the current spdk_nvme_probe
> > attach-once semantics, it still imposes an extra burden of the
> > already probed controller tracking on the application that, for
> > instance, seeks to attach to multiple namespaces under the same
> > controller. The below patch provides an optional fix for the issue.
> > Notice that it also fixes the bug of the error returned by the
> > enumeration being ignored.
> > 
> > Regards,
> > Andrey
> > 
> > diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index
> > 63a316b..2a3ac64 100644
> > --- a/lib/nvme/nvme.c
> > +++ b/lib/nvme/nvme.c
> > @@ -296,10 +296,32 @@ spdk_nvme_probe(void *cb_ctx,
> > spdk_nvme_probe_cb probe_cb, spdk_nvme_attach_cb a
> > 
> >      nvme_mutex_lock(&g_nvme_driver.lock);
> > 
> > +    /*
> > +     * First, check if the requested controller has already been
> > started.
> > +     */
> > +    TAILQ_FOREACH(ctrlr, &g_nvme_driver.attached_ctrlrs, tailq) {
> > +        if (!probe_cb(cb_ctx, ctrlr->devhandle))
> > +            continue;
> > +
> > +        nvme_mutex_unlock(&g_nvme_driver.lock);
> > +        attach_cb(cb_ctx, ctrlr->devhandle, ctrlr);
> > +        return 0;
> > +    }
> > +
> >      enum_ctx.probe_cb = probe_cb;
> >      enum_ctx.cb_ctx = cb_ctx;
> > 
> >      rc = nvme_pci_enumerate(nvme_enum_cb, &enum_ctx);
> > +
> > +    /*
> > +     * Appreciate the error being returned
> > +     * by the nvme_pci_enumerate, if any.
> > +     */
> > +    if (rc) {
> > +        nvme_mutex_unlock(&g_nvme_driver.lock);
> > +        return rc;
> > +    }
> > +
> >      /*
> >       * Keep going even if one or more nvme_attach() calls failed,
> >       *  but maintain the value of rc to signal errors when we
> > return.
> > _______________________________________________
> > SPDK mailing list
> > SPDK(a)lists.01.org
> > https://lists.01.org/mailman/listinfo/spdk
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk