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

[Niklas Cassel]

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