Re: [PATCH v5 13/14] hw/block/nvme: Use zone metadata file for persistence

[Klaus Jensen <its@irrelevant.dk>]

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

On Sep 29 15:43, Dmitry Fomichev wrote:
> 
> 
> > -----Original Message-----
> > From: Klaus Jensen <its@irrelevant.dk>
> > Sent: Monday, September 28, 2020 3:52 AM
> > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > Cc: Keith Busch <kbusch@kernel.org>; Klaus Jensen
> > <k.jensen@samsung.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> > Mathieu-Daudé <philmd@redhat.com>; Maxim Levitsky
> > <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>; Niklas Cassel
> > <Niklas.Cassel@wdc.com>; Damien Le Moal <Damien.LeMoal@wdc.com>;
> > qemu-block@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> > <Alistair.Francis@wdc.com>; Matias Bjorling <Matias.Bjorling@wdc.com>
> > Subject: Re: [PATCH v5 13/14] hw/block/nvme: Use zone metadata file for
> > persistence
> > 
> > On Sep 28 11:35, Dmitry Fomichev wrote:
> > > A ZNS drive that is emulated by this module is currently initialized
> > > with all zones Empty upon startup. However, actual ZNS SSDs save the
> > > state and condition of all zones in their internal NVRAM in the event
> > > of power loss. When such a drive is powered up again, it closes or
> > > finishes all zones that were open at the moment of shutdown. Besides
> > > that, the write pointer position as well as the state and condition
> > > of all zones is preserved across power-downs.
> > >
> > > This commit adds the capability to have a persistent zone metadata
> > > to the device. The new optional module property, "zone_file",
> > > is introduced. If added to the command line, this property specifies
> > > the name of the file that stores the zone metadata. If "zone_file" is
> > > omitted, the device will be initialized with all zones empty, the same
> > > as before.
> > >
> > > If zone metadata is configured to be persistent, then zone descriptor
> > > extensions also persist across controller shutdowns.
> > >
> > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > ---
> > >  hw/block/nvme-ns.c    | 341
> > ++++++++++++++++++++++++++++++++++++++++--
> > >  hw/block/nvme-ns.h    |  33 ++++
> > >  hw/block/nvme.c       |   2 +
> > >  hw/block/trace-events |   1 +
> > >  4 files changed, 362 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > > index 47751f2d54..a94021da81 100644
> > > --- a/hw/block/nvme-ns.c
> > > +++ b/hw/block/nvme-ns.c
> > > @@ -293,12 +421,180 @@ static void
> > nvme_init_zone_meta(NvmeNamespace *ns)
> > >              i--;
> > >          }
> > >      }
> > > +
> > > +    if (ns->params.zone_file) {
> > > +        nvme_set_zone_meta_dirty(ns);
> > > +    }
> > > +}
> > > +
> > > +static int nvme_open_zone_file(NvmeNamespace *ns, bool *init_meta,
> > > +                               Error **errp)
> > > +{
> > > +    Object *file_be;
> > > +    HostMemoryBackend *fb;
> > > +    struct stat statbuf;
> > > +    int ret;
> > > +
> > > +    ret = stat(ns->params.zone_file, &statbuf);
> > > +    if (ret && errno == ENOENT) {
> > > +        *init_meta = true;
> > > +    } else if (!S_ISREG(statbuf.st_mode)) {
> > > +        error_setg(errp, "\"%s\" is not a regular file",
> > > +                   ns->params.zone_file);
> > > +        return -1;
> > > +    }
> > > +
> > > +    file_be = object_new(TYPE_MEMORY_BACKEND_FILE);
> > > +    object_property_set_str(file_be, "mem-path", ns->params.zone_file,
> > > +                            &error_abort);
> > > +    object_property_set_int(file_be, "size", ns->meta_size, &error_abort);
> > > +    object_property_set_bool(file_be, "share", true, &error_abort);
> > > +    object_property_set_bool(file_be, "discard-data", false, &error_abort);
> > > +    if (!user_creatable_complete(USER_CREATABLE(file_be), errp)) {
> > > +        object_unref(file_be);
> > > +        return -1;
> > > +    }
> > > +    object_property_add_child(OBJECT(ns), "_fb", file_be);
> > > +    object_unref(file_be);
> > > +
> > > +    fb = MEMORY_BACKEND(file_be);
> > > +    ns->zone_mr = host_memory_backend_get_memory(fb);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static int nvme_map_zone_file(NvmeNamespace *ns, bool *init_meta)
> > > +{
> > > +    ns->zone_meta = (void *)memory_region_get_ram_ptr(ns->zone_mr);
> > 
> > I forgot that the HostMemoryBackend doesn't magically make the memory
> > available to the device, so of course this is still needed.
> > 
> > Anyway.
> > 
> > No reason for me to keep complaining about this. I do not like it, I
> > will not ACK it and I think I made my reasons pretty clear.
> 
> So, memory_region_msync() is ok, but memory_region_get_ram_ptr() is not??
> This is the same API! You are really splitting hairs here to suit your agenda.
> Moving goal posts again....
> 
> The "I do not like it" part is priceless. It is great that we have mail archives available.
> 

If you read my review again, its pretty clear that I am calling out the
abstraction. I was clear that if it *really* had to be mmap based, then
it should use hostmem. Sorry for moving your patchset forward by
suggesting an improvement.

But again, I also made it pretty clear that I did not agree with the
abstraction. And that I very much disliked that it was non-portable. And
had endiannes issues. I made it SUPER clear that that was why I "did not
like it".

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