Re: [PATCH v6 38/42] nvme: support multiple namespaces

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2020-03-31 at 07:48 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:59, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > > device model. The nvme device creates a bus named from the device name
> > > ('id'). The nvme-ns devices then connect to this and registers
> > > themselves with the nvme device.
> > > 
> > > This changes how an nvme device is created. Example with two namespaces:
> > > 
> > >   -drive file=nvme0n1.img,if=none,id=disk1
> > >   -drive file=nvme0n2.img,if=none,id=disk2
> > >   -device nvme,serial=deadbeef,id=nvme0
> > >   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> > >   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > > 
> > > The drive property is kept on the nvme device to keep the change
> > > backward compatible, but the property is now optional. Specifying a
> > > drive for the nvme device will always create the namespace with nsid 1.
> > > 
> > > Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > Reviewed-by: Keith Busch <kbusch@kernel.org>
> > > ---
> > >  hw/block/Makefile.objs |   2 +-
> > >  hw/block/nvme-ns.c     | 157 +++++++++++++++++++++++++++
> > >  hw/block/nvme-ns.h     |  60 +++++++++++
> > >  hw/block/nvme.c        | 233 ++++++++++++++++++++++++++---------------
> > >  hw/block/nvme.h        |  47 ++++-----
> > >  hw/block/trace-events  |   4 +-
> > >  6 files changed, 389 insertions(+), 114 deletions(-)
> > >  create mode 100644 hw/block/nvme-ns.c
> > >  create mode 100644 hw/block/nvme-ns.h
> > > 
> > > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> > > index 4b4a2b338dc4..d9141d6a4b9b 100644
> > > --- a/hw/block/Makefile.objs
> > > +++ b/hw/block/Makefile.objs
> > > @@ -2518,9 +2561,6 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> > >      id->psd[0].mp = cpu_to_le16(0x9c4);
> > >      id->psd[0].enlat = cpu_to_le32(0x10);
> > >      id->psd[0].exlat = cpu_to_le32(0x4);
> > > -    if (blk_enable_write_cache(n->conf.blk)) {
> > > -        id->vwc = 1;
> > > -    }
> > 
> > Shouldn't that be kept? Assuming that user used the legacy 'drive' option,
> > and it had no write cache enabled.
> > 
> 
> When using the drive option we still end up calling the same code that
> handles the "new style" namespaces and that code will handle the write
> cache similary.
OK. That makes sense.

> 
> > >  
> > >      n->bar.cap = 0;
> > >      NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
> > > @@ -2533,25 +2573,34 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> > >      n->bar.intmc = n->bar.intms = 0;
> > >  }
> > >  
> > > -static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > > +int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > >  {
> > > -    int64_t bs_size;
> > > -    NvmeIdNs *id_ns = &ns->id_ns;
> > > +    uint32_t nsid = nvme_nsid(ns);
> > >  
> > > -    bs_size = blk_getlength(n->conf.blk);
> > > -    if (bs_size < 0) {
> > > -        error_setg_errno(errp, -bs_size, "blk_getlength");
> > > +    if (nsid > NVME_MAX_NAMESPACES) {
> > > +        error_setg(errp, "invalid nsid (must be between 0 and %d)",
> > > +                   NVME_MAX_NAMESPACES);
> > >          return -1;
> > >      }
> > >  
> > > -    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > > -    n->ns_size = bs_size;
> > > +    if (!nsid) {
> > > +        for (int i = 1; i <= n->num_namespaces; i++) {
> > > +            NvmeNamespace *ns = nvme_ns(n, i);
> > > +            if (!ns) {
> > > +                nsid = i;
> > > +                break;
> > > +            }
> > > +        }
> > 
> > This misses an edge error case, where all the namespaces are allocated.
> > Yes, it would be insane to allocate all 256 namespaces but still.
> > 
> 
> Impressive catch! Fixed!
Thanks!
> 
> > 
> > > +    } else {
> > > +        if (n->namespaces[nsid - 1]) {
> > > +            error_setg(errp, "nsid must be unique");
> > 
> > I''l would change that error message to something like 
> > "namespace id %d is already in use" or something like that.
> > 
> 
> Done.
> 

Best regards,
	Maxim Levitsky