On Sep 29 15:42, Dmitry Fomichev wrote: > > -----Original Message----- > > From: Klaus Jensen <i...@irrelevant.dk> > > Sent: Monday, September 28, 2020 2:37 AM > > To: Dmitry Fomichev <dmitry.fomic...@wdc.com> > > Cc: Keith Busch <kbu...@kernel.org>; Damien Le Moal > > <damien.lem...@wdc.com>; 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>; qemu-bl...@nongnu.org; qemu- > > de...@nongnu.org; Alistair Francis <alistair.fran...@wdc.com>; Matias > > Bjorling <matias.bjorl...@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 <i...@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).
signature.asc
Description: PGP signature
