RE: [PATCH 00/16] hw/block/nvme: zoned namespace command set

[Dmitry Fomichev]

> -----Original Message-----
> From: Qemu-block <qemu-block-
> bounces+dmitry.fomichev=wdc.com@nongnu.org> On Behalf Of Klaus
> Jensen
> Sent: Thursday, September 24, 2020 4:45 PM
> To: qemu-devel@nongnu.org
> Cc: Fam Zheng <fam@euphon.net>; Kevin Wolf <kwolf@redhat.com>; qemu-
> block@nongnu.org; Klaus Jensen <k.jensen@samsung.com>; Max Reitz
> <mreitz@redhat.com>; Keith Busch <kbusch@kernel.org>; Klaus Jensen
> <its@irrelevant.dk>
> Subject: [PATCH 00/16] hw/block/nvme: zoned namespace command set
> 
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> 
> While going through a few rounds of reviews on Dmitry's series I have
> 
> also continued nursing my own implementation since originally posted. I
> 
> did not receive any reviews originally, since it depended on a lot of
> 
> preceding series, but now, with the staging of multiple namespaces on
> 
> nvme-next yesterday, I think it deserves another shot since it now
> 
> applies directly. The series consists of a couple of trivial patches
> 
> followed by the "hw/block/nvme: add support for dulbe and block
> 
> utilization tracking", "hw/block/nvme: support namespace types" and the
> 
> set of zoned namespace support patches.
> 
> 
> 
> A couple of points on how this defers from Dmitry et. al.'s series and
> 
> why I think this implementation deserves a review.
> 
> 
> 
>   * Standard blockdev-based approach to persistent state. The
> 
>     implementation uses a plain blockdev associated with the nvme-ns
> 
>     device for storing state persistently. This same 'pstate' blockdev
> 
>     is also used for logical block allocation tracking.
> 

Is persistent state mandatory or optional? Sorry for asking, but I am
still catching up with your other patches. I think having it optional is
a big benefit for performance testing.

> 
> 
>   * Relies on automatic configuration of DLFEAT according to what the
> 
>     underlying blockdev provides (i.e. BDRV_O_UNMAP for guaranteeing
> 
>     zeroes on discarded blocks) for handling reads in the gaps between
> 
>     write pointer, ZCAP and ZSZE. Issues discards for zone resets. This
> 
>     removes the zero filling.
> 

Doesn't this make non-zero fill patterns impossible? In many storage
environments, vendors and admins are adamant about having varying
fill patterns to see who caused the data corruption if there is one.
Not sure how important this for the particular application, but WDC
series provides the functionality to specify the fill pattern.

> 
> 
> Finally, I wrote this. I am *NOT* saying that this somehow makes it
> 
> better, but as a maintainer, is a big deal to me since both series are
> 
> arguably a lot of code to maintain and support (both series are about
> 
> the same size). But - I am not the only maintainer, so if Keith (now
> 
> suddenly placed in the grim role as some sort of arbiter) signs off on
> 
> Dmitry's series, then so be it, I will rest my case.
> 
> 
> 
> I think we all want to see an implementation of zoned namespaces in QEMU
> 
> sooner rather than later, but I would lie if I said I wouldn't prefer
> 
> that it was this one.
> 
> 
> 
> Based-on: <20200922084533.1273962-1-its@irrelevant.dk>
> 
> 
> 
> Gollu Appalanaidu (1):
> 
>   hw/block/nvme: add commands supported and effects log page
> 
> 
> 
> Klaus Jensen (15):
> 
>   hw/block/nvme: add nsid to get/setfeat trace events
> 
>   hw/block/nvme: add trace event for requests with non-zero status code
> 
>   hw/block/nvme: make lba data size configurable
> 
>   hw/block/nvme: reject io commands if only admin command set selected
> 
>   hw/block/nvme: consolidate read, write and write zeroes
> 
>   hw/block/nvme: add support for dulbe and block utilization tracking
> 
>   hw/block/nvme: support namespace 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: support zone active excursions
> 
>   hw/block/nvme: support reset/finish recommended limits
> 
> 
> 
>  docs/specs/nvme.txt   |   49 +-
> 
>  hw/block/nvme-ns.h    |  166 +++-
> 
>  hw/block/nvme.h       |   24 +
> 
>  include/block/nvme.h  |  262 ++++++-
> 
>  block/nvme.c          |    4 +-
> 
>  hw/block/nvme-ns.c    |  411 +++++++++-
> 
>  hw/block/nvme.c       | 1727 +++++++++++++++++++++++++++++++++++++++-
> -
> 
>  hw/block/trace-events |   50 +-
> 
>  8 files changed, 2580 insertions(+), 113 deletions(-)
> 
> 
> 
> --
> 
> 2.28.0
> 
> 
>