From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============8633700037409435010==" MIME-Version: 1.0 From: Walker, Benjamin Subject: Re: [SPDK] Allow attachment to an already probed controller Date: Fri, 26 Feb 2016 22:20:52 +0000 Message-ID: <1456525252.80454.153.camel@intel.com> In-Reply-To: CANvN+enm0XjKit0P-mU5vCX80dSPhVt3TU6EoKhY1ivPyommyw@mail.gmail.com List-ID: To: spdk@lists.01.org --===============8633700037409435010== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Fri, 2016-02-26 at 21:18 +0300, Andrey Kuzmin wrote: > On Fri, Feb 26, 2016 at 9:08 PM, Verkamp, Daniel > wrote: > > Hi Andrey, > > = > > I am not sure what behavior you would like to see from probe when > > calling it again after attaching some controllers.=C2=A0=C2=A0We 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.=C2=A0=C2= =A0The > > 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 > > = > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0nvme_mutex_lock(&g_nvme_driver.lock); > > = > > +=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* First, check if the requested controll= er has already been > > started. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0TAILQ_FOREACH(ctrlr, &g_nvme_driver.attached_c= trlrs, tailq) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!probe_cb(cb_ctx, = ctrlr->devhandle)) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0continue; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0nvme_mutex_unlock(&g_n= vme_driver.lock); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0attach_cb(cb_ctx, ctrl= r->devhandle, ctrlr); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0; > > +=C2=A0=C2=A0=C2=A0=C2=A0} > > + > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0enum_ctx.probe_cb =3D probe_cb; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0enum_ctx.cb_ctx =3D cb_ctx; > > = > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D nvme_pci_enumerate(nvme_enum_cb, &= enum_ctx); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Appreciate the error being returned > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* by the nvme_pci_enumerate, if any. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0if (rc) { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0nvme_mutex_unlock(&g_n= vme_driver.lock); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return rc; > > +=C2=A0=C2=A0=C2=A0=C2=A0} > > + > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Keep going even if one or more nv= me_attach() calls failed, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0but 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 --===============8633700037409435010==--