Re: [PATCH for-6.0 7/7] hw/block/nvme: fix handling of private namespaces

From: Klaus Jensen <its@irrelevant.dk>
To: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	qemu-block@nongnu.org, Klaus Jensen <k.jensen@samsung.com>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
	Minwoo Im <minwoo.im.dev@gmail.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Keith Busch <kbusch@kernel.org>
Subject: Re: [PATCH for-6.0 7/7] hw/block/nvme: fix handling of private namespaces
Date: Mon, 5 Apr 2021 15:56:17 +0200	[thread overview]
Message-ID: <YGsXAZ8Sn9vaMeol@apples.localdomain> (raw)
In-Reply-To: <20210405123239.GA1248@2030045822>

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

On Apr  5 18:02, Gollu Appalanaidu wrote:
> On Wed, Mar 24, 2021 at 09:09:07PM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Prior to this patch, if a private nvme-ns device (that is, a namespace
> > that is not linked to a subsystem) is wired up to an nvme-subsys linked
> > nvme controller device, the device fails to verify that the namespace id
> > is unique within the subsystem. NVM Express v1.4b, Section 6.1.6 ("NSID
> > and Namespace Usage") states that because the device supports Namespace
> > Management, "NSIDs *shall* be unique within the NVM subsystem".
> > 
> > Additionally, prior to this patch, private namespaces are not known to
> > the subsystem and the namespace is considered exclusive to the
> > controller with which it is initially wired up to. However, this is not
> > the definition of a private namespace; per Section 1.6.33 ("private
> > namespace"), a private namespace is just a namespace that does not
> > support multipath I/O or namespace sharing, which means "that it is only
> > able to be attached to one controller at a time".
> > 
> > Fix this by always allocating namespaces in the subsystem (if one is
> > linked to the controller), regardsless of the shared/private status of
> > the namespace. Whether or not the namespace is shareable is controlled
> > by a new `shared` nvme-ns parameter.
> > 
> > Finally, this fix allows the nvme-ns `subsys` parameter to be removed,
> > since the `shared` parameter now serves the purpose of attaching the
> > namespace to all controllers in the subsystem upon device realization.
> > It is invalid to have an nvme-ns namespace device with a linked
> > subsystem without the parent nvme controller device also being linked to
> > one and since the nvme-ns devices will unconditionally be "attached" (in
> > QEMU terms that is) to an nvme controller device through an NvmeBus, the
> > nvme-ns namespace device can always get a reference to the subsystem of
> > the controller it is explicitly (using 'bus=' parametr) or implicitly
> > attaching to.
> > 
> > Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem")
> > Cc: Minwoo Im <minwoo.im.dev@gmail.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/block/nvme-ns.h     |  10 ++--
> > hw/block/nvme-subsys.h |   7 ++-
> > hw/block/nvme.h        |  39 +-------------
> > include/block/nvme.h   |   1 +
> > hw/block/nvme-ns.c     |  74 +++++++++++++++++++++++----
> > hw/block/nvme-subsys.c |  28 -----------
> > hw/block/nvme.c        | 112 +++++++++++++----------------------------
> > hw/block/trace-events  |   1 -
> > 8 files changed, 106 insertions(+), 166 deletions(-)
> > 

<snip>

> > 
> > static Property nvme_ns_props[] = {
> >     DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
> > -    DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS,
> > -                     NvmeSubsystem *),
> >     DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false),
> > +    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false)i,
> 
> Nice change point, hope we need update the usage, removing "subsys" from
> nvme-ns device params and adding "shared" param?
> 

Good catch, thanks. Fixing that up for a v2.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]