Re: [PATCH v3 3/4] qcow2: add zoned emulation capability

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Aug 28, 2023 at 11:09:54PM +0800, Sam Li wrote:
> By adding zone operations and zoned metadata, the zoned emulation
> capability enables full emulation support of zoned device using
> a qcow2 file. The zoned device metadata includes zone type,
> zoned device state and write pointer of each zone, which is stored
> to an array of unsigned integers.
> 
> Each zone of a zoned device makes state transitions following
> the zone state machine. The zone state machine mainly describes
> five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
> READ ONLY and OFFLINE states will generally be affected by device
> internal events. The operations on zones cause corresponding state
> changing.
> 
> Zoned devices have a limit on zone resources, which puts constraints on
> write operations into zones.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/qcow2.c          | 657 ++++++++++++++++++++++++++++++++++++++++-
>  block/qcow2.h          |   2 +
>  block/trace-events     |   1 +
>  docs/interop/qcow2.txt |   6 +
>  4 files changed, 664 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7074bfc620..bc98d98c8e 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -194,6 +194,153 @@ qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
>      return cryptoopts_qdict;
>  }
>  
> +#define QCOW2_ZT_IS_CONV(wp)    (wp & 1ULL << 59)
> +
> +static inline int qcow2_get_wp(uint64_t wp)
> +{
> +    /* clear state and type information */
> +    return ((wp << 5) >> 5);
> +}
> +
> +static inline int qcow2_get_zs(uint64_t wp)
> +{
> +    return (wp >> 60);
> +}
> +
> +static inline void qcow2_set_zs(uint64_t *wp, BlockZoneState zs)
> +{
> +    uint64_t addr = qcow2_get_wp(*wp);
> +    addr |= ((uint64_t)zs << 60);
> +    *wp = addr;
> +}
> +
> +/*
> + * Perform a state assignment and a flush operation that writes the new wp
> + * value to the dedicated location of the disk file.
> + */
> +static int qcow2_write_wp_at(BlockDriverState *bs, uint64_t *wp,
> +                             uint32_t index, BlockZoneState zs) {
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t wpv = *wp;
> +    int ret;
> +
> +    qcow2_set_zs(wp, zs);
> +    ret = bdrv_pwrite(bs->file, s->zoned_header.zonedmeta_offset
> +        + sizeof(uint64_t) * index, sizeof(uint64_t), wp, 0);
> +
> +    if (ret < 0) {
> +        goto exit;
> +    }
> +    trace_qcow2_wp_tracking(index, qcow2_get_wp(*wp) >> BDRV_SECTOR_BITS);
> +    return ret;
> +
> +exit:
> +    *wp = wpv;
> +    error_report("Failed to write metadata with file");
> +    return ret;
> +}
> +
> +static bool qcow2_check_active_zones(BlockDriverState *bs)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +
> +    if (!s->zoned_header.max_active_zones) {
> +        return true;
> +    }
> +
> +    if (s->nr_zones_exp_open + s->nr_zones_imp_open + s->nr_zones_closed
> +        < s->zoned_header.max_active_zones) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static int qcow2_check_open_zones(BlockDriverState *bs)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    int ret;
> +
> +    if (!s->zoned_header.max_open_zones) {
> +        return 0;
> +    }
> +
> +    if (s->nr_zones_exp_open + s->nr_zones_imp_open
> +        < s->zoned_header.max_open_zones) {
> +        return 0;
> +    }
> +
> +    if(s->nr_zones_imp_open && qcow2_check_active_zones(bs)) {
> +        /* TODO: it takes O(n) time complexity (n = nr_zones).
> +         * Optimizations required. */
> +        /* close one implicitly open zones to make it available */
> +        for (int i = s->zoned_header.nr_conv_zones;

Please use uint32_t to keep the types consistent.

> +        i < bs->bl.nr_zones; ++i) {
> +            uint64_t *wp = &bs->wps->wp[i];
> +            if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
> +                ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +                bs->wps->wp[i] = *wp;

This assignment is unnecessary since wp points to bs->wps->wp[i]. The
value has already been updated.

> +                s->nr_zones_imp_open--;
> +                s->nr_zones_closed++;
> +                break;
> +            }
> +        }
> +        return 0;

If this for loop completes with i == bs->bl.nr_zones then we've failed
to close an IOPEN zone. The return value should be an error like -EBUSY.

> +    }
> +
> +    return -EINVAL;

Which case does this -EINVAL cover? Won't we get here when no zones are
open yet?

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