On Sep 29 15:43, Dmitry Fomichev wrote:
>
>
> > -----Original Message-----
> > From: Klaus Jensen <i...@irrelevant.dk>
> > Sent: Monday, September 28, 2020 3:52 AM
> > To: Dmitry Fomichev <dmitry.fomic...@wdc.com>
> > Cc: Keith Busch <kbu...@kernel.org>; Klaus Jensen
> > <k.jen...@samsung.com>; Kevin Wolf <kw...@redhat.com>; Philippe
> > Mathieu-Daudé <phi...@redhat.com>; Maxim Levitsky
> > <mlevi...@redhat.com>; Fam Zheng <f...@euphon.net>; Niklas Cassel
> > <niklas.cas...@wdc.com>; Damien Le Moal <damien.lem...@wdc.com>;
> > qemu-bl...@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> > <alistair.fran...@wdc.com>; Matias Bjorling <matias.bjorl...@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.fomic...@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".
signature.asc
Description: PGP signature
