Re: [PATCH 2/3] hw/block/nvme: Check zone state before checking boundaries

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

On Jan 26 14:02, Dmitry Fomichev wrote:
> The code in nvme_check_zone_write() first checks for zone boundary
> condition violation and only after that it proceeds to verify that the
> zone state is suitable the write to happen. This is not consistent with
> nvme_check_zone_read() flow - in that function, zone state is checked
> before making any boundary checks. Besides, checking that ZSLBA + NLB
> does not cross the write boundary is now redundant for Zone Append and
> only needs to be done for writes.
> 
> Move the check in the code to be performed for Write and Write Zeroes
> commands only and to occur after zone state checks.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  hw/block/nvme.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 67538010ef..b712634c27 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1138,13 +1138,8 @@ static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
>      uint64_t bndry = nvme_zone_wr_boundary(zone);
>      uint16_t status;
>  
> -    if (unlikely(slba + nlb > bndry)) {
> -        status = NVME_ZONE_BOUNDARY_ERROR;
> -    } else {
> -        status = nvme_check_zone_state_for_write(zone);
> -    }

Alright. The double check on boundary that I pointed out in the previous
patch is fixed here.

> -
> -    if (status != NVME_SUCCESS) {
> +    status = nvme_check_zone_state_for_write(zone);
> +    if (status) {
>          trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
>      } else {
>          assert(nvme_wp_is_valid(zone));
> @@ -1158,10 +1153,14 @@ static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
>                  trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
>                  status = NVME_INVALID_FIELD;
>              }
> -        } else if (unlikely(slba != zone->w_ptr)) {
> -            trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
> -                                               zone->w_ptr);
> -            status = NVME_ZONE_INVALID_WRITE;
> +        } else {
> +            if (unlikely(slba != zone->w_ptr)) {
> +                trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
> +                                                   zone->w_ptr);
> +                status = NVME_ZONE_INVALID_WRITE;
> +            } else if (unlikely(slba + nlb > bndry)) {
> +                status = NVME_ZONE_BOUNDARY_ERROR;
> +            }
>          }
>      }
>  
> -- 
> 2.28.0
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]