[SPDK] Allow attachment to an already probed controller

[SPDK] Allow attachment to an already probed controller

[Andrey Kuzmin]

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

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.

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

[Andrey Kuzmin]

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

On Sat, Feb 27, 2016 at 1:20 AM, Walker, Benjamin
<benjamin.walker(a)intel.com> wrote:
> 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?

That would relieve the caller from the attached controller list
maintenance, but will still require a lookup in the application. A
more intuitive way, IMO, would be to add an extra parameter to
probe(), e.g. 'bool skip_already_attached'. Set to true, it will
preserve current probe() semantics. If the user sets skip_attached to
false, probe() will start with scanning the attached controller TAILQ,
applying the provided probe_cb()/attached_cb() as in the patch
provided.

Regards,
Andrey

>
>>
>> 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

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

[Walker, Benjamin]

[-- 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

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

[Andrey Kuzmin]

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

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.

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

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

[Verkamp, Daniel]

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

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?

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.

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