[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 38/42] nvme: support multiple namespaces
From: |
Klaus Birkelund Jensen |
Subject: |
Re: [PATCH v6 38/42] nvme: support multiple namespaces |
Date: |
Tue, 31 Mar 2020 07:48:29 +0200 |
On Mar 25 12:59, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Klaus Jensen <address@hidden>
> > Reviewed-by: Keith Busch <address@hidden>
> > ---
> > 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.
> >
> > 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!
>
> > + } 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.
Re: [PATCH v6 00/42] nvme: support NVMe v1.3d, SGLs and multiple namespaces, no-reply, 2020/03/16
Re: [PATCH v6 00/42] nvme: support NVMe v1.3d, SGLs and multiple namespaces, Maxim Levitsky, 2020/03/25