Re: [PATCH] hw/block/nvme: fix zone write finalize

[Keith Busch]

On Tue, Jan 12, 2021 at 10:42:35AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The zone write pointer is unconditionally advanced, even for write
> faults. Make sure that the zone is always transitioned to Full if the
> write pointer reaches zone capacity.

Looks like some spec weirdness. It says we can transition to full:

  b) as a result of successful completion of a write operation that
     writes one or more logical blocks that causes the zone to reach its
     writeable zone capacity;

But a failed write also advances the write pointer as you've pointed
out, so they might want to strike "successful".

Looks fine to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Cc: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  hw/block/nvme.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 0854ee307227..280b31b4459d 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1268,10 +1268,13 @@ static void nvme_finalize_zoned_write(NvmeNamespace 
> *ns, NvmeRequest *req,
>      nlb = le16_to_cpu(rw->nlb) + 1;
>      zone = nvme_get_zone_by_slba(ns, slba);
>  
> +    zone->d.wp += nlb;
> +
>      if (failed) {
>          res->slba = 0;
> -        zone->d.wp += nlb;
> -    } else if (zone->w_ptr == nvme_zone_wr_boundary(zone)) {
> +    }
> +
> +    if (zone->d.wp == nvme_zone_wr_boundary(zone)) {

The previous check was using 'zone->w_ptr', but now it's 'zone->d.wp'.
As far as I can tell, this difference will mean the zone won't finalize
until the last write completes, where before it could finalize after the
zone's last write is submitted. Either way looks okay, but I think these
two values ought to always be in sync.

>          switch (nvme_get_zone_state(zone)) {
>          case NVME_ZONE_STATE_IMPLICITLY_OPEN:
>          case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> @@ -1288,9 +1291,6 @@ static void nvme_finalize_zoned_write(NvmeNamespace 
> *ns, NvmeRequest *req,
>          default:
>              assert(false);
>          }
> -        zone->d.wp = zone->w_ptr;
> -    } else {
> -        zone->d.wp += nlb;
>      }
>  }