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-block@nongnu.org; qemu-de...@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