| Message ID | 20200630100139.1483002-1-its@irrelevant.dk |
|---|---|
| Headers | show |
| Series | hw/block/nvme: namespace types and zoned namespaces | expand |
On Tue, Jun 30, 2020 at 12:01:29PM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > 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: <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". > > 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. Kind regards, Niklas
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 <k.jensen@samsung.com> >> >> 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: <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". > >> >> 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/qemu-devel@nongnu.org/msg718086.html > > > Kind regards, > Niklas >
On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: > 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 These look like sensible patches to rebase future work on, IMO. The 1.3 updates had been prepared a while ago, at least.
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. Cheers, Klaus
On Jun 30 08:42, Keith Busch wrote: > On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: > > 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 > > > These look like sensible patches to rebase future work on, IMO. The 1.3 > updates had been prepared a while ago, at least. I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a no-brainer. It just needs to get in asap. The "hw/block/nvme: handle transient dma errors" series would really benefit from most of the patches in my "hw/block/nvme: AIO and address mapping refactoring" series. The elephant in the room is the AIO part (the "hw/block/nvme: allow multiple aios per command" patch), so I will get rid of it and leave the cleanup patches there and post it as a new series together with the "handle transient dma errors" fixes. This would make it a series of around ~17-18 patches, but I think they are all quite reviewable. The bump to v1.3 should also pretty much be ready for merging.
> -----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
Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: > On Jun 30 08:42, Keith Busch wrote: > > On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: > > > 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 > > > > > > These look like sensible patches to rebase future work on, IMO. The 1.3 > > updates had been prepared a while ago, at least. > > I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a > no-brainer. It just needs to get in asap. I think we need to talk about how nvme patches are supposed to get merged. I'm not familiar with the hardware nor the code, so the model was that I just blindly merge patches that Keith has reviewed/acked, just to spare him the work to prepare a pull request. But obviously, we started doing things this way when there was a lot less activity around the nvme emulation. If we find that this doesn't scale any more, maybe we need to change something. Depending on how much time Keith can spend on review in the near future and how much control he wants to keep over the development, I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer or as a reviewer. Then I could rely on reviews/acks from either of you for merging series. Of course, the patches don't necessarily have to go through my tree either if this only serves to complicate things these days. If sending separate pull requests directly to Peter would make things easier, I certainly wouldn't object. Kevin
On Jul 1 12:34, Kevin Wolf wrote: > Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: > > On Jun 30 08:42, Keith Busch wrote: > > > On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: > > > > 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 > > > > > > > > > These look like sensible patches to rebase future work on, IMO. The 1.3 > > > updates had been prepared a while ago, at least. > > > > I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a > > no-brainer. It just needs to get in asap. > > I think we need to talk about how nvme patches are supposed to get > merged. I'm not familiar with the hardware nor the code, so the model > was that I just blindly merge patches that Keith has reviewed/acked, > just to spare him the work to prepare a pull request. But obviously, we > started doing things this way when there was a lot less activity around > the nvme emulation. > > If we find that this doesn't scale any more, maybe we need to change > something. Honestly, I do not think the current model has worked very well for some time; especially for larger series where I, for one, has felt that my work was largely ignored due to a lack of designated reviewers. Things only picked up when Beata, Maxim and Philippe started reviewing my series - maybe out of pity or because I was bombing the list, I don't know ;) We've also seen good patches from Andrzej linger on the list for quite a while, prompting a number of RESENDs. I only recently allocated more time and upped my review game, but I hope that contributors feel that stuff gets reviewed in a timely fashion by now. Please understand that this is in NO WAY a criticism of Keith who already made it very clear to me that he did not have a lot time to review, but only ack the odd patch. > Depending on how much time Keith can spend on review in the > near future and how much control he wants to keep over the development, > I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer > or as a reviewer. Then I could rely on reviews/acks from either of you > for merging series. > I would be happy to step up (officially) to help maintain the device with Keith and review on a daily basis, and my position can support this. > Of course, the patches don't necessarily have to go through my tree > either if this only serves to complicate things these days. If sending > separate pull requests directly to Peter would make things easier, I > certainly wouldn't object. > I don't think there is any reason to by-pass your tree. I think the volume would need to increase even further for that to make sense.
On Wed, 2020-07-01 at 15:18 +0200, Klaus Jensen wrote: > On Jul 1 12:34, Kevin Wolf wrote: > > Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: > > > On Jun 30 08:42, Keith Busch wrote: > > > > On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: > > > > > 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 > > > > > > > > These look like sensible patches to rebase future work on, IMO. The 1.3 > > > > updates had been prepared a while ago, at least. > > > > > > I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a > > > no-brainer. It just needs to get in asap. > > > > I think we need to talk about how nvme patches are supposed to get > > merged. I'm not familiar with the hardware nor the code, so the model > > was that I just blindly merge patches that Keith has reviewed/acked, > > just to spare him the work to prepare a pull request. But obviously, we > > started doing things this way when there was a lot less activity around > > the nvme emulation. > > > > If we find that this doesn't scale any more, maybe we need to change > > something. > > Honestly, I do not think the current model has worked very well for some > time; especially for larger series where I, for one, has felt that my > work was largely ignored due to a lack of designated reviewers. Things > only picked up when Beata, Maxim and Philippe started reviewing my > series - maybe out of pity or because I was bombing the list, I don't > know ;) > > We've also seen good patches from Andrzej linger on the list for quite a > while, prompting a number of RESENDs. I only recently allocated more > time and upped my review game, but I hope that contributors feel that > stuff gets reviewed in a timely fashion by now. > > Please understand that this is in NO WAY a criticism of Keith who > already made it very clear to me that he did not have a lot time to > review, but only ack the odd patch. > > > Depending on how much time Keith can spend on review in the > > near future and how much control he wants to keep over the development, > > I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer > > or as a reviewer. Then I could rely on reviews/acks from either of you > > for merging series. > > > > I would be happy to step up (officially) to help maintain the device > with Keith and review on a daily basis, and my position can support > this. > > > Of course, the patches don't necessarily have to go through my tree > > either if this only serves to complicate things these days. If sending > > separate pull requests directly to Peter would make things easier, I > > certainly wouldn't object. > > > > I don't think there is any reason to by-pass your tree. I think the > volume would need to increase even further for that to make sense. > It my fault as well - I need to get back to reviewing these. (I'll review few of them today I hope) Best regards, Maxim Levitsky
On 7/1/20 3:18 PM, Klaus Jensen wrote: > On Jul 1 12:34, Kevin Wolf wrote: >> Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: >>> On Jun 30 08:42, Keith Busch wrote: >>>> On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: >>>>> 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 >>>> >>>> >>>> These look like sensible patches to rebase future work on, IMO. The 1.3 >>>> updates had been prepared a while ago, at least. >>> >>> I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a >>> no-brainer. It just needs to get in asap. >> >> I think we need to talk about how nvme patches are supposed to get >> merged. I'm not familiar with the hardware nor the code, so the model >> was that I just blindly merge patches that Keith has reviewed/acked, >> just to spare him the work to prepare a pull request. But obviously, we >> started doing things this way when there was a lot less activity around >> the nvme emulation. >> >> If we find that this doesn't scale any more, maybe we need to change >> something. > > Honestly, I do not think the current model has worked very well for some > time; especially for larger series where I, for one, has felt that my > work was largely ignored due to a lack of designated reviewers. Things > only picked up when Beata, Maxim and Philippe started reviewing my > series - maybe out of pity or because I was bombing the list, I don't > know ;) I have no interest in the NVMe device emulation, but one of the first thing I notice when I look at the wiki the time I wanted to send my first patch, is the "Return the favor" paragraph: https://wiki.qemu.org/Contribute/SubmitAPatch#Return_the_favor "Peer review only works if everyone chips in a bit of review time. If everyone submitted more patches than they reviewed, we would have a patch backlog. A good goal is to try to review at least as many patches from others as what you submit. Don't worry if you don't know the code base as well as a maintainer; it's perfectly fine to admit when your review is weak because you are unfamiliar with the code." So as some reviewed my patches, I try to return the favor to the community, in particular when I see someone is stuck waiting for review, and the patch topic is some area I can understand. I don't see that as an "out of pity" reaction. Note, it is true bomb series scares reviewers. You learned it the bad way. But you can see, after resending the first part of your "bomb", even if it took 10 versions, the result is a great improvement! > We've also seen good patches from Andrzej linger on the list for quite a > while, prompting a number of RESENDs. I only recently allocated more > time and upped my review game, but I hope that contributors feel that > stuff gets reviewed in a timely fashion by now. > > Please understand that this is in NO WAY a criticism of Keith who > already made it very clear to me that he did not have a lot time to > review, but only ack the odd patch. > >> Depending on how much time Keith can spend on review in the >> near future and how much control he wants to keep over the development, >> I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer >> or as a reviewer. Then I could rely on reviews/acks from either of you >> for merging series. >> > > I would be happy to step up (officially) to help maintain the device > with Keith and review on a daily basis, and my position can support > this. Sounds good to me, but it is up to Keith Busch to accept. It would be nice to have at least one developer from WDC listed as designated reviewer too. Maxim is candidate for designated reviewer but I think he doesn't have the time. It would also nice to have Andrzej Jakowski listed, if he is interested. > >> Of course, the patches don't necessarily have to go through my tree >> either if this only serves to complicate things these days. If sending >> separate pull requests directly to Peter would make things easier, I >> certainly wouldn't object. >> > > I don't think there is any reason to by-pass your tree. I think the > volume would need to increase even further for that to make sense. >
On Wed, Jul 01, 2020 at 03:57:27PM +0200, Philippe Mathieu-Daudé wrote: > On 7/1/20 3:18 PM, Klaus Jensen wrote: > > We've also seen good patches from Andrzej linger on the list for quite a > > while, prompting a number of RESENDs. I only recently allocated more > > time and upped my review game, but I hope that contributors feel that > > stuff gets reviewed in a timely fashion by now. > > > > Please understand that this is in NO WAY a criticism of Keith who > > already made it very clear to me that he did not have a lot time to > > review, but only ack the odd patch. > > > >> Depending on how much time Keith can spend on review in the > >> near future and how much control he wants to keep over the development, > >> I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer > >> or as a reviewer. Then I could rely on reviews/acks from either of you > >> for merging series. > >> > > > > I would be happy to step up (officially) to help maintain the device > > with Keith and review on a daily basis, and my position can support > > this. > > Sounds good to me, but it is up to Keith Busch to accept. I definitely want to continue at least having the opprotunity to review, though you may have noticed I am a bit low on time for more thorough maintenance this project deserves. The recent development pace for nvme would benefit from having its own tree, so I'm open to either co-maintenance, or handing off this to others. Please allow me to send a few queries off-list today to check-in with potentially interested parties. > It would be nice to have at least one developer from WDC listed as > designated reviewer too. > > Maxim is candidate for designated reviewer but I think he doesn't > have the time. > > It would also nice to have Andrzej Jakowski listed, if he is interested.
On 7/1/20 6:57 AM, Philippe Mathieu-Daudé wrote: > On 7/1/20 3:18 PM, Klaus Jensen wrote: >> On Jul 1 12:34, Kevin Wolf wrote: >>> Am 30.06.2020 um 22:36 hat Klaus Jensen geschrieben: >>>> On Jun 30 08:42, Keith Busch wrote: >>>>> On Tue, Jun 30, 2020 at 04:09:46PM +0200, Philippe Mathieu-Daudé wrote: >>>>>> 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 >>>>> >>>>> >>>>> These look like sensible patches to rebase future work on, IMO. The 1.3 >>>>> updates had been prepared a while ago, at least. >>>> >>>> I think Philippe's "hw/block/nvme: Fix I/O BAR structure" series is a >>>> no-brainer. It just needs to get in asap. >>> >>> I think we need to talk about how nvme patches are supposed to get >>> merged. I'm not familiar with the hardware nor the code, so the model >>> was that I just blindly merge patches that Keith has reviewed/acked, >>> just to spare him the work to prepare a pull request. But obviously, we >>> started doing things this way when there was a lot less activity around >>> the nvme emulation. >>> >>> If we find that this doesn't scale any more, maybe we need to change >>> something. >> >> Honestly, I do not think the current model has worked very well for some >> time; especially for larger series where I, for one, has felt that my >> work was largely ignored due to a lack of designated reviewers. Things >> only picked up when Beata, Maxim and Philippe started reviewing my >> series - maybe out of pity or because I was bombing the list, I don't >> know ;) > > I have no interest in the NVMe device emulation, but one of the first > thing I notice when I look at the wiki the time I wanted to send my > first patch, is the "Return the favor" paragraph: > https://wiki.qemu.org/Contribute/SubmitAPatch#Return_the_favor > > "Peer review only works if everyone chips in a bit of review time. > If everyone submitted more patches than they reviewed, we would > have a patch backlog. A good goal is to try to review at least as > many patches from others as what you submit. Don't worry if you > don't know the code base as well as a maintainer; it's perfectly > fine to admit when your review is weak because you are unfamiliar > with the code." > > So as some reviewed my patches, I try to return the favor to the > community, in particular when I see someone is stuck waiting for > review, and the patch topic is some area I can understand. > > I don't see that as an "out of pity" reaction. > > Note, it is true bomb series scares reviewers. You learned it the > bad way. But you can see, after resending the first part of your > "bomb", even if it took 10 versions, the result is a great > improvement! > >> We've also seen good patches from Andrzej linger on the list for quite a >> while, prompting a number of RESENDs. I only recently allocated more >> time and upped my review game, but I hope that contributors feel that >> stuff gets reviewed in a timely fashion by now. >> >> Please understand that this is in NO WAY a criticism of Keith who >> already made it very clear to me that he did not have a lot time to >> review, but only ack the odd patch. >> >>> Depending on how much time Keith can spend on review in the >>> near future and how much control he wants to keep over the development, >>> I could imagine adding Klaus to MAINTAINERS, either as a co-maintainer >>> or as a reviewer. Then I could rely on reviews/acks from either of you >>> for merging series. >>> >> >> I would be happy to step up (officially) to help maintain the device >> with Keith and review on a daily basis, and my position can support >> this. > > Sounds good to me, but it is up to Keith Busch to accept. > > It would be nice to have at least one developer from WDC listed as > designated reviewer too. > > Maxim is candidate for designated reviewer but I think he doesn't > have the time. > > It would also nice to have Andrzej Jakowski listed, if he is interested. Thx! Of course I am interested in helping and I think it is actually great idea to have couple of designated maintainers/reviewers as it would be easier for folks to receive feedback vs requesting it in polling manner :) And please don't get me wrong -- I'm not complaining about anything -- I think it is just reality that everybody is stretched out into multiple directions struggling to allocate time for multiple things. Having many people will actually increase likelihood of introducing high quality improvements. Also, +1 on separate tree for nvme emulation. > >> >>> Of course, the patches don't necessarily have to go through my tree >>> either if this only serves to complicate things these days. If sending >>> separate pull requests directly to Peter would make things easier, I >>> certainly wouldn't object. >>> >> >> I don't think there is any reason to by-pass your tree. I think the >> volume would need to increase even further for that to make sense. >> >
On Thu, Jul 02, 2020 at 01:29:26PM -0700, Andrzej Jakowski wrote: > > Thx! Of course I am interested in helping and I think it is actually great > idea to have couple of designated maintainers/reviewers as it would be easier > for folks to receive feedback vs requesting it in polling manner :) > And please don't get me wrong -- I'm not complaining about anything -- I > think it is just reality that everybody is stretched out into multiple directions > struggling to allocate time for multiple things. Having many people will > actually increase likelihood of introducing high quality improvements. > > Also, +1 on separate tree for nvme emulation. Thanks for your help. Klaus and I will be setting up an external tree for qemu-nvme development (tentatively on git.infradead.org) and pull-request. I'm just waiting for the server admin to upload our public keys. If I don't hear back by Monday, I will use an alternate server in the interim.
From: Klaus Jensen <k.jensen@samsung.com> Hi all, 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) 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") 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(-)