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

[Dmitry Fomichev]

> -----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.