> -----Original Message-----
> From: Klaus Jensen <its@irrelevant.dk>
> Sent: Tuesday, June 30, 2020 4:30 PM
> To: Niklas Cassel <Niklas.Cassel@wdc.com>
> Cc: qemu-block@nongnu.org; Klaus Jensen <k.jensen@samsung.com>;
> qemu-devel@nongnu.org; Keith Busch <kbusch@kernel.org>; Max Reitz
> <mreitz@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Javier Gonzalez
> <javier.gonz@samsung.com>; Maxim Levitsky <mlevitsk@redhat.com>;
> Philippe Mathieu-Daudé <philmd@redhat.com>; Dmitry Fomichev
> <Dmitry.Fomichev@wdc.com>; Damien Le Moal
> <Damien.LeMoal@wdc.com>; Matias Bjorling <Matias.Bjorling@wdc.com>
> Subject: Re: [PATCH 00/10] hw/block/nvme: namespace types and zoned
> namespaces
> 
> On Jun 30 12:59, Niklas Cassel wrote:
> > On Tue, Jun 30, 2020 at 12:01:29PM +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > >
> > > Hi all,
> >
> > Hello Klaus,
> >
> 
> Hi Niklas,
> 
> > >
> > >   * the controller uses timers to autonomously finish zones (wrt. FRL)
> >
> > AFAICT, Dmitry's patches does this as well.
> >
> 
> Hmm, yeah. Something is going on at least. It's not really clear to me
> why it works or what is happening with that admin completion queue
> timer, but I'll dig through it.
> 
> > >
> > > I've been on paternity leave for a month, so I havn't been around to
> > > review Dmitry's patches, but I have started that process now. I would
> > > also be happy to work with Dmitry & Friends on merging our versions to
> > > get the best of both worlds if it makes sense.
> > >
> > > This series and all preparatory patch sets (the ones I've been posting
> > > yesterday and today) are available on my GitHub[2]. Unfortunately
> > > Patchew got screwed up in the middle of me sending patches and it
> never
> > > picked up v2 of "hw/block/nvme: support multiple namespaces" because
> it
> > > was getting late and I made a mistake with the CC's. So my posted series
> > > don't apply according to Patchew, but they actually do if you follow the
> > > Based-on's (... or just grab [2]).
> > >
> > >
> > >   [1]: Message-Id: <20200617213415.22417-1-dmitry.fomichev@wdc.com>
> > >   [2]: https://github.com/birkelund/qemu/tree/for-master/nvme
> > >
> > >
> > > Based-on: <20200630043122.1307043-1-its@irrelevant.dk>
> > > ("[PATCH 0/3] hw/block/nvme: bump to v1.4")
> >
> > Is this the only patch series that this series depends on?
> >
> > In the beginning of the cover letter, you mentioned
> > "NVMe v1.4 mandatory support", "SGLs", "multiple namespaces",
> > and "and mostly just overall clean up".
> >
> 
> No, its a string of series that all has a Based-on tag (that is, "[PATCH
> 0/3] hw/block/nvme: bump to v1.4" has another Based-on tag that points
> to the dependency of that). The point was to have patchew nicely apply
> everything, but it broke midway...
> 
> As Philippe pointed out, all of the patch sets are integrated in the
> GitHub tree, applied to QEMU master.
> 
> >
> > I think that you have done a great job getting the NVMe
> > driver out of a frankenstate, and made it compliant with
> > a proper spec (NVMe 1.4).
> >
> > I'm also a big fan of the refactoring so that the driver
> > handles more than one namespace, and the new bus model.
> >
> 
> Well, thanks! :)
> 
> > I know that you first sent your
> > "nvme: support NVMe v1.3d, SGLs and multiple namespaces"
> > patch series July, last year.
> >
> > Looking at your outstanding patch series on patchwork:
> > https://patchwork.kernel.org/project/qemu-devel/list/?submitter=188679
> >
> > (Feel free to correct me if I have misunderstood anything.)
> >
> > I see that these are related to your patch series from July last year:
> > hw/block/nvme: bump to v1.3
> > hw/block/nvme: support scatter gather lists
> > hw/block/nvme: support multiple namespaces
> > hw/block/nvme: bump to v1.4
> >
> 
> Yeah this stuff has been around for a while so the history on patchwork
> is a mess.
> 
> >
> > This patch series seems minor and could probably be merged immediately:
> > hw/block/nvme: handle transient dma errors
> >
> 
> Sure, but it's nicer in combination with the previous series
> ("hw/block/nvme: AIO and address mapping refactoring"). What I /can/ do
> is rip out "hw/block/nvme: allow multiple aios per command" as that
> patch might require more time for reviews. The rest of that series are
> clean ups and a couple of bug fixes.
> 
> >
> > This patch series looks a bit weird:
> > hw/block/nvme: AIO and address mapping refactoring
> >
> > Since it looks like a V1 post, and was first posted yesterday.
> > However, 2 out of the 17 patches in are Acked-by: Keith.
> > (Perhaps some of your previously posted patches was put inside
> > this new patch series?)
> >
> 
> Yes that and reviewers requested a lot of separation, so basically the
> patch set ballooned.
> 
> >
> > This patch series:
> > hw/block/nvme: namespace types and zoned namespaces
> >
> > Which was first posted today. Up until earlier today, we haven't seen
> > any patches from you related to ZNS (only overall NVMe cleanups).
> > Dmitry's ZNS patches have been on the list since 2020-06-16.
> >
> 
> Yeah, as I mentioned in my cover letter, I was on leave, so I wasn't
> around for the big ZNS release day either. But, honestly, I think this
> is irrelevant - code should be merged based on technical reasons (not
> technicalities).
> 
> >
> > Just a friendly suggestion, how about:
> >
> > 1) We get your
> >
> > hw/block/nvme: bump to v1.3
> > hw/block/nvme: support scatter gather lists
> > hw/block/nvme: support multiple namespaces
> > hw/block/nvme: bump to v1.4
> >
> > patch series merged.
> >
> 
> Blowing my own horn here, but yeah, it seems like everyone would like to
> see this merged.
> 
> > 2) We get Dmitry's patch series merged.
> >
> > Shared 4:th) If there is any feature that you miss in Dmitry's patch series,
> > perhaps you could send patches to add what you are missing.
> >
> 
> Looks like the two version are pretty much on par in terms of features.
> 
> > Shared 4:th) Your other patch series:
> > hw/block/nvme: AIO and address mapping refactoring could get merged.
> >
> 
> As stated above I think its only a single commit ("hw/block/nvme: allow
> multiple aios per command") that is controversial in that series.
> 
> >
> > Please don't take this suggestion the wrong way, I'm simply trying
> > to come up with a way to move forward from here.
> >
> 
> Absolutely - I totally get that you want to move forward with Dmitry's
> series, but I'd like to finish my review before committing to anything.
> 

Klaus,

I see that ZNS series from WDC is pretty much orthogonal to most of your
patches from "bump to 1.3/bump to 1.4" series... with a few notable
exceptions - I had to add support for Get Log Page command, Command
Effects Log and AERs to fulfill ZNS protocol requirements. I understand that
you've had that functionality already staged in your "bump to 1.3"
patchset and beyond and I don't insist that the implementations of these
features from our ZNS series are necessarily to be used. I think Niklas' plan
appears to be quite productive - for us to rebase on top of "bump to 1.3..1.4"
and review these patches in the process, apply our series and then
incorporate some of the ideas from your ZNS/NSTypes patchset on top of that.
This way, very little work will go to waste.

Cheers,
Dmitry

> 
> Cheers,
> Klaus