Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set

[Klaus Jensen <its@irrelevant.dk>]

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

On Sep 29 15:42, Dmitry Fomichev wrote:
> > -----Original Message-----
> > From: Klaus Jensen <its@irrelevant.dk>
> > Sent: Monday, September 28, 2020 2:37 AM
> > To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> > Cc: Keith Busch <kbusch@kernel.org>; Damien Le Moal
> > <Damien.LeMoal@wdc.com>; 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>; qemu-block@nongnu.org; qemu-
> > devel@nongnu.org; Alistair Francis <Alistair.Francis@wdc.com>; Matias
> > Bjorling <Matias.Bjorling@wdc.com>
> > Subject: Re: [PATCH v4 00/14] hw/block/nvme: Support Namespace Types
> > and Zoned Namespace Command Set
> > 
> > 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.
> > 
> 
> Wow. By bringing up political correctness here you are basically admitting
> the fact that you have no real technical argument here.

I prefer that we support all platforms if and when we can. That's a
technical argument, not a personal one like you those you start using
now.

> The whole Windows issue is red herring that you are using to attack
> the code that is absolutely legit, but comes from a competitor.

I can't even...

> Your initial complaint was that it doesn't compile in mingw and that
> it uses "wrong" API. You have even suggested the API to use. Now, the
> code uses that API and builds fine, but now it's still not good simply
> because you "do not like it". It's a disgrace.
> 

I answered this in a previous reply.

> > > > 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.
> > 
> 
> We are trying to emulate NVMEe controller NVRAM.  The best abstraction
> for emulating NVRAM would be... NVRAM!
> 

You never brought that up before and sure it could be a fair argument,
except it is not true.

PMR is emulating NVRAM (and requires memory semantics). Persistent state
is not emulating anything. It is an implementation detail.

> > > > 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.
> 
> Again, wow! Now you turned around and arbitrarily elevated this issue from
> moderate ("Does it matter?, cutting corners") to severe ("buggy"). Likely
> because v5 of WDC patchset has been posted.

No, exactly as I wrote above, after I hit reply I considered a
follow-up. I guess I should have.

> This, again, just shows your lack of integrity as a maintainer.
> 

I can't believe I just read that.

I will not put up with this. It is completely non-called for. I stand up
for my opinions and I will fight to make sure the best possible code
goes upstream. Yes, I am paid by Samsung. But I can compartmentalize. I
have been working on QEMU before Samsung and I know how to separate
corporate interest and open source. I have a proven record on this list
to show that. I really cannot believe that you brought it down to that
level. I have been putting forth technical arguments throughout this
entire review process and now you are getting personal.

Not. Cool. Please keep things professional from now.

> This "issue" is a real trivial one to fix as I described above and you are
> blowing it up way out of proportion, making it look like it is a fundamental
> problem that can not be resolved. It's not.
> 

If it is so trival to fix, please fix it. I think I made it clear that I
won't be happy until it is portable.

And please note that I have *not* complained about other parts of your
series. I have complained ALOT about the persistence implementation -
and I continue to stand behind those complaints.

I'm getting super tired of this one-sided process. I have continuously
reviewed and commented your series, I have found multiple bugs, I have
suggested improvements. Maybe if just one or two of those 9 people who
signed off on your zoned implementation could look past their own nose
and look at my series - you might just realize that its decent, portable
and offers some niceties that yours do not have (at the cost of the same
amount of code mind you).

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