> -----Original Message----- > From: Klaus Jensen <i...@irrelevant.dk> > Sent: Tuesday, June 30, 2020 4:30 PM > To: Niklas Cassel <niklas.cas...@wdc.com> > Cc: qemu-bl...@nongnu.org; Klaus Jensen <k.jen...@samsung.com>; > qemu-devel@nongnu.org; Keith Busch <kbu...@kernel.org>; Max Reitz > <mre...@redhat.com>; Kevin Wolf <kw...@redhat.com>; Javier Gonzalez > <javier.g...@samsung.com>; Maxim Levitsky <mlevi...@redhat.com>; > Philippe Mathieu-Daudé <phi...@redhat.com>; Dmitry Fomichev > <dmitry.fomic...@wdc.com>; Damien Le Moal > <damien.lem...@wdc.com>; Matias Bjorling <matias.bjorl...@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.jen...@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.fomic...@wdc.com> > > > [2]: https://github.com/birkelund/qemu/tree/for-master/nvme > > > > > > > > > Based-on: <20200630043122.1307043-1-...@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
