On 6/30/20 2:59 PM, Niklas Cassel wrote: > On Tue, Jun 30, 2020 at 12:01:29PM +0200, Klaus Jensen wrote: >> From: Klaus Jensen <[email protected]> >> >> Hi all, > > Hello Klaus, > >> >> This series adds support for TP 4056 ("Namespace Types") and TP 4053 >> ("Zoned Namespaces") and is an alternative implementation to the one >> submitted by Dmitry[1]. >> >> While I don't want this to end up as a discussion about the merits of >> each version, I want to point out a couple of differences from Dmitry's >> version. At a glance, my version >> >> * builds on my patch series that adds fairly complete NVMe v1.4 >> mandatory support, as well as nice-to-have feature such as SGLs, >> multiple namespaces and mostly just overall clean up. This finally >> brings the nvme device into a fairly compliant state on which we can >> add new features. I've tried hard to get these compliance and >> clean-up patches merged for a long time (in parallel with developing >> the emulation of NST and ZNS) and I would be really sad to see them >> by-passed since they have already been through many iterations and >> already carries Acked- and Reviewed-by's for the bulk of the >> patches. I think the nvme device is already in a "frankenstate" wrt. >> the implemented nvme version and the features it currently supports, >> so I think this kind of cleanup is long overdue. >> >> * uses an attached blockdev and standard blk_aio for persistent zone >> info. This is the same method used in our patches for Write >> Uncorrectable and (separate and extended lba) metadata support, but >> I've left those optional features out for now to ease the review >> process. >> >> * relies on the universal dulbe support added in ("hw/block/nvme: add >> support for dulbe") and sparse images for handling reads in gaps >> (above write pointer and below ZSZE); that is - the size of the >> underlying blockdev is in terms of ZSZE, not ZCAP >> >> * the controller uses timers to autonomously finish zones (wrt. FRL) > > AFAICT, Dmitry's patches does this as well. > >> >> 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: <[email protected]> >> [2]: https://github.com/birkelund/qemu/tree/for-master/nvme >> >> >> Based-on: <[email protected]> >> ("[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". > >> >> Klaus Jensen (10): >> hw/block/nvme: support I/O Command Sets >> hw/block/nvme: add zns specific fields and types >> hw/block/nvme: add basic read/write for zoned namespaces >> hw/block/nvme: add the zone management receive command >> hw/block/nvme: add the zone management send command >> hw/block/nvme: add the zone append command >> hw/block/nvme: track and enforce zone resources >> hw/block/nvme: allow open to close transitions by controller >> hw/block/nvme: allow zone excursions >> hw/block/nvme: support reset/finish recommended limits >> >> block/nvme.c | 6 +- >> hw/block/nvme-ns.c | 397 +++++++++- >> hw/block/nvme-ns.h | 148 +++- >> hw/block/nvme.c | 1676 +++++++++++++++++++++++++++++++++++++++-- >> hw/block/nvme.h | 76 +- >> hw/block/trace-events | 43 +- >> include/block/nvme.h | 252 ++++++- >> 7 files changed, 2469 insertions(+), 129 deletions(-) >> >> -- >> 2.27.0 >> > > 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. > > 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 > > > This patch series seems minor and could probably be merged immediately: > hw/block/nvme: handle transient dma errors > > > 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?) > > > 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. > > > 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. > > 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. > > Shared 4:th) Your other patch series: > hw/block/nvme: AIO and address mapping refactoring could get merged. > > > Please don't take this suggestion the wrong way, I'm simply trying > to come up with a way to move forward from here.
Few months ago Klaus sent a bomb series with ~80 patches, we asked him to split in digestible series of ~20 patches. Earlier in this cover Klaus provided a link to his git repository with all the patches sorted [2]: https://github.com/birkelund/qemu/tree/for-master/nvme This seems enough to get the big picture. Niklas Cassel, it would be helpful if you or Dmitry can review Klaus patches. I see Klaus is already reviewing Dmitry ones. Both Keith and Kevin are quite busy recently. To help them I suggest once you reviewed your patches each other, one of you could send the big series with all patches together. Anyway soft-freeze is next week, so you have to decide what is critical. What I see doable for the following days is: - hw/block/nvme: Fix I/O BAR structure [3] - hw/block/nvme: handle transient dma errors - hw/block/nvme: bump to v1.3 [3] https://www.mail-archive.com/[email protected]/msg718086.html > > > Kind regards, > Niklas >
