RE: [PATCH v7 05/11] hw/block/nvme: Support Zoned Namespace Command Set

[Dmitry Fomichev]

> -----Original Message-----
> From: Klaus Jensen <its@irrelevant.dk>
> Sent: Wednesday, October 21, 2020 6:26 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Keith Busch <kbusch@kernel.org>; Klaus Jensen
> <k.jensen@samsung.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Maxim Levitsky
> <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>; Niklas Cassel
> <Niklas.Cassel@wdc.com>; Damien Le Moal <Damien.LeMoal@wdc.com>;
> qemu-block@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> <Alistair.Francis@wdc.com>; Matias Bjorling <Matias.Bjorling@wdc.com>
> Subject: Re: [PATCH v7 05/11] hw/block/nvme: Support Zoned Namespace
> Command Set
> 
> On Oct 19 11:17, Dmitry Fomichev wrote:
> > +/*
> > + * Close or finish all the zones that are currently open.
> > + */
> > +static void nvme_zoned_clear_ns(NvmeNamespace *ns)
> > +{
> > +    NvmeZone *zone;
> > +    uint32_t set_state;
> > +    int i;
> > +
> > +    zone = ns->zone_array;
> > +    for (i = 0; i < ns->num_zones; i++, zone++) {
> > +        switch (nvme_get_zone_state(zone)) {
> > +        case NVME_ZONE_STATE_IMPLICITLY_OPEN:
> > +            QTAILQ_REMOVE(&ns->imp_open_zones, zone, entry);
> > +            break;
> > +        case NVME_ZONE_STATE_EXPLICITLY_OPEN:
> > +            QTAILQ_REMOVE(&ns->exp_open_zones, zone, entry);
> > +            break;
> > +        case NVME_ZONE_STATE_CLOSED:
> > +            /* fall through */
> > +        default:
> > +            continue;
> > +        }
> > +
> > +        if (zone->d.wp == zone->d.zslba) {
> > +            set_state = NVME_ZONE_STATE_EMPTY;
> > +        } else {
> > +            set_state = NVME_ZONE_STATE_CLOSED;
> > +        }
> > +
> > +        switch (set_state) {
> > +        case NVME_ZONE_STATE_CLOSED:
> > +            trace_pci_nvme_clear_ns_close(nvme_get_zone_state(zone),
> > +                                          zone->d.zslba);
> > +            QTAILQ_INSERT_TAIL(&ns->closed_zones, zone, entry);
> > +            break;
> > +        case NVME_ZONE_STATE_EMPTY:
> > +            trace_pci_nvme_clear_ns_reset(nvme_get_zone_state(zone),
> > +                                          zone->d.zslba);
> > +            break;
> > +        case NVME_ZONE_STATE_FULL:
> > +            trace_pci_nvme_clear_ns_full(nvme_get_zone_state(zone),
> > +                                         zone->d.zslba);
> > +            zone->d.wp = nvme_zone_wr_boundary(zone);
> > +            QTAILQ_INSERT_TAIL(&ns->full_zones, zone, entry);
> > +        }
> 
> No need for the switch here - just add to the closed list in the
> conditional.

The switch becomes handy later in the series, particularly after adding
descriptor extensions. For easier reviewing, it makes sense to add it from
the beginning even though it is rudimentary at this point.

> 
> The NVME_ZONE_STATE_FULL case is unreachable.

Indeed. This should be introduced in the next patch.

Now, I've looked at this code again and the active/open counting in this
function ends up to be not quite right, I am fixing it.

> 
> > +
> > +        zone->w_ptr = zone->d.wp;
> > +        nvme_set_zone_state(zone, set_state);
> > +    }
> > +}