[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Na
From: |
Klaus Jensen |
Subject: |
Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set |
Date: |
Mon, 28 Sep 2020 08:36:48 +0200 |
On Sep 28 02:33, Dmitry Fomichev wrote:
> > -----Original Message-----
> > From: Klaus Jensen <its@irrelevant.dk>
> >
> > If it really needs to be memory mapped, then I think a hostmem-based
> > approach similar to what Andrzej did for PMR is needed (I think that
> > will get rid of the CONFIG_POSIX ifdef at least, but still leave it
> > slightly tricky to get it to work on all platforms AFAIK).
>
> Ok, it looks that using the HostMemoryBackendFile backend will be
> more appropriate. This will remove the need for conditional compile.
>
> The mmap() portability is pretty decent across software platforms.
> Any poor Windows user who is forced to emulate ZNS on mingw will be
> able to do so, just without having zone state persistency. Considering
> how specialized this stuff is in first place, I estimate the number of users
> affected by this "limitation" to be exactly zero.
>
QEMU is a cross platform project - we should strive for portability.
Alienating developers that use a Windows platform and calling them out
as "poor" is not exactly good for the zoned ecosystem.
> > But really,
> > since we do not require memory semantics for this, then I think the
> > abstraction is fundamentally wrong.
> >
>
> Seriously, what is wrong with using mmap :) ? It is used successfully for
> similar applications, for example -
> https://github.com/open-iscsi/tcmu-runner/blob/master/file_zbc.c
>
There is nothing fundamentally wrong with mmap. I just think it is the
wrong abstraction here (and it limits portability for no good reason).
For PMR there is a good reason - it requires memory semantics.
> > I am, of course, blowing my own horn, since my implementation uses a
> > portable blockdev for this.
> >
>
> You are making it sound like the entire WDC series relies on this approach.
> Actually, the persistency is introduced in the second to last patch in the
> series and it only adds a couple of lines of code in the i/o path to mark
> zones dirty. This is possible because of using mmap() and I find the way
> it is done to be quite elegant, not ugly :)
>
No, I understand that your implementation works fine without
persistance, but persistance is key. That is why my series adds it in
the first patch. Without persistence it is just a toy. And the QEMU
device is not just an "NVMe-version" of null_blk.
And I don't think I ever called the use of mmap ugly. I called out the
physical memory API shenanigans as a hack.
> > Another issue is the complete lack of endian conversions. Does it
> > matter? It depends. Will anyone ever use this on a big endian host and
> > move the meta data backing file to a little endian host? Probably not.
> > So does it really matter? Probably not, but it is cutting corners.
> >
After I had replied this, I considered a follow-up, because there are
probably QEMU developers that would call me out on this.
This definitely DOES matter to QEMU.
>
> Great point on endianness! Naturally, all file backed values are stored in
> their native endianness. This way, there is no extra overhead on big endian
> hardware architectures. Portability concerns can be easily addressed by
> storing metadata endianness as a byte flag in its header. Then, during
> initialization, the metadata validation code can detect the possible
> discrepancy in endianness and automatically convert the metadata to the
> endianness of the host. This part is out of scope of this series, but I would
> be able to contribute such a solution as an enhancement in the future.
>
It is not out of scope. I don't see why we should merge something that
is arguably buggy.
Bottomline is that I just don't see why we should accept an
implementation that
a) excludes some platforms (Windows) from using persistence; and
b) contains endianness conversion issues
when there is a portable implementation posted that at least tries to
convert endianness as needed.
signature.asc
Description: PGP signature
- [PATCH v4 08/14] hw/block/nvme: Define Zoned NS Command Set trace events, (continued)
- [PATCH v4 08/14] hw/block/nvme: Define Zoned NS Command Set trace events, Dmitry Fomichev, 2020/09/23
- [PATCH v4 10/14] hw/block/nvme: Introduce max active and open zone limits, Dmitry Fomichev, 2020/09/23
- [PATCH v4 12/14] hw/block/nvme: Add injection of Offline/Read-Only zones, Dmitry Fomichev, 2020/09/23
- [PATCH v4 09/14] hw/block/nvme: Support Zoned Namespace Command Set, Dmitry Fomichev, 2020/09/23
- [PATCH v4 11/14] hw/block/nvme: Support Zone Descriptor Extensions, Dmitry Fomichev, 2020/09/23
- [PATCH v4 13/14] hw/block/nvme: Use zone metadata file for persistence, Dmitry Fomichev, 2020/09/23
- [PATCH v4 14/14] hw/block/nvme: Document zoned parameters in usage text, Dmitry Fomichev, 2020/09/23
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Klaus Jensen, 2020/09/24
- RE: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Dmitry Fomichev, 2020/09/27
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set,
Klaus Jensen <=
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Keith Busch, 2020/09/28
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Damien Le Moal, 2020/09/28
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Klaus Jensen, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Damien Le Moal, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Keith Busch, 2020/09/29
- RE: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Dmitry Fomichev, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Klaus Jensen, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Keith Busch, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Klaus Jensen, 2020/09/29
- Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Keith Busch, 2020/09/29