Re: [PATCH 00/10] hw/block/nvme: namespace types and zoned namespaces

[Philippe Mathieu-Daudé]

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
>