On Mon, Jun 29, 2026 at 7:40 PM Stefan Hajnoczi <[email protected]> wrote:
>
> On Tue, Jun 23, 2026 at 08:48:29PM +0200, Sam Li wrote:
> > +/*
> > + * Read/Write the new wp value for zone `index` through write pointe
>
> pointe -> pointer
>
> > +static int coroutine_fn
> > +qcow2_co_zone_report(BlockDriverState *bs, int64_t offset,
> > + unsigned int *nr_zones, BlockZoneDescriptor *zones)
> > +{
> > + BDRVQcow2State *s = bs->opaque;
> > + uint64_t zone_size = s->zoned_header.zone_size;
> > + uint64_t zone_capacity = s->zoned_header.zone_capacity;
> > + int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
> > + int64_t size = bs->bl.nr_zones * zone_size;
> > + unsigned int nrz;
> > + int i = 0;
> > + int si;
>
> Should i and si be unsigned int since nr_zones is an unsigned int?
>
> > +static int coroutine_fn GRAPH_RDLOCK
> > +qcow2_reset_zone(BlockDriverState *bs, uint32_t index,
> > + int64_t len)
> > +{
> > + BDRVQcow2State *s = bs->opaque;
> > + int nrz = bs->bl.nr_zones;
> > + int64_t zone_size = bs->bl.zone_size;
> > + int n, ret = 0;
> > + bool any_dirtied = false;
>
> nr_zones is an unsigned int. Variables holding numbers of zones like
> nrz, i, and n should be unsigned int, too. That way they behave
> correctly over their full range instead of behaving unexpectedly when
> they become negative (e.g. in a for loop with i < n).
Makes sense.
>
> > +
> > + qemu_co_mutex_lock(&bs->wps->colock);
> > + uint64_t *wp = &bs->wps->wp[index];
> > + if (len == bs->total_sectors << BDRV_SECTOR_BITS) {
> > + n = nrz;
> > + index = 0;
> > + wp = &bs->wps->wp[0];
> > + } else {
> > + n = len / zone_size;
> > + }
> > +
> > + for (int i = 0; i < n; ++i) {
> > + uint64_t *wp_i = (uint64_t *)(wp + i);
> > + uint64_t wpi_v = *wp_i;
> > + if (QCOW2_ZT_IS_CONV(wpi_v)) {
> > + continue;
> > + }
> > +
> > + /* Reject if any write is in flight for this zone. */
> > + if (s->zone_wp_state &&
> > + !QTAILQ_EMPTY(&s->zone_wp_state[index + i].in_flight)) {
> > + ret = -EBUSY;
> > + goto unlock;
> > + }
>
> This is a coroutine function and it should yield to wait instead of
> returning -EBUSY. If -EBUSY is propagated to the guest that would result
> in unexpected behavior - I think real devices don't do this and the
> guest will not be prepared to handle -EBUSY.
You are right. It should yield until drain, then proceed. A zoned
null_blk does this.
>
> > +
> > + qemu_co_mutex_lock(&s->lock);
> > + BlockZoneState zs = qcow2_get_zone_state(bs, index + i);
> > + switch (zs) {
> > + case BLK_ZS_EMPTY:
> > + break;
> > + case BLK_ZS_IOPEN:
> > + qcow2_rm_imp_open_zone(s, index + i);
> > + trace_qcow2_imp_open_zones(BLK_ZO_RESET, s->nr_zones_imp_open);
> > + break;
> > + case BLK_ZS_EOPEN:
> > + qcow2_rm_exp_open_zone(s, index + i);
> > + break;
> > + case BLK_ZS_CLOSED:
> > + qcow2_rm_closed_zone(s, index + i);
> > + break;
> > + case BLK_ZS_FULL:
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + qemu_co_mutex_unlock(&s->lock);
> > + goto unlock;
> > + }
> > + qemu_co_mutex_unlock(&s->lock);
> > +
> > + if (zs == BLK_ZS_EMPTY) {
> > + continue;
> > + }
> > +
> > + /*
> > + * Zero the data extent first. Date write fires before the WP
> > cluster
>
> Date -> Data
>
> > + * hits disk. So the wp advance cannot become durable while stale
> > data
> > + * is still readable.
> > + */
> > + ret = qcow2_co_pwrite_zeroes(bs, (uint64_t)(index + i) * zone_size,
> > + zone_size, 0);
> > + if (ret < 0) {
> > + error_report("Failed to clear zone data at zone %u",
> > + index + i);
> > + goto unlock;
> > + }
> > +
> > + qemu_co_mutex_lock(&s->lock);
> > + qcow2_cache_depends_on_flush(s->wp_cache);
> > + qemu_co_mutex_unlock(&s->lock);
> > +
> > + *wp_i = (uint64_t)(index + i) * zone_size;
> > + ret = qcow2_rw_wp_at(bs, wp_i, index + i, true);
> > + if (ret < 0) {
> > + goto unlock;
>
> *wp_i has been modified but the operation will fail. The write pointers
> in memory and the write pointers in the qcow2 cache and on disk are now
> inconsistent. I expect the failure semantics to be that the write
> pointers in memory are consistent with the on-disk write pointers,
> although I haven't checked specs to see if they say anything about this.
Right, on wp-write failure, the in-memory wp needs to be stored so RAM
stays consistent with disk.
>
> > + }
> > + any_dirtied = true;
> > + }
> > +
> > + if (any_dirtied) {
> > + /* Single flush at the end. */
> > + qemu_co_mutex_lock(&s->lock);
> > + ret = qcow2_cache_flush(bs, s->wp_cache);
> > + qemu_co_mutex_unlock(&s->lock);
> > + }
> > +
> > +unlock:
> > + qemu_co_mutex_unlock(&bs->wps->colock);
> > + return ret;
> > +}
> > +
> > +static int coroutine_fn GRAPH_RDLOCK
> > +qcow2_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp op,
> > + int64_t offset, int64_t len)
>
> Indentation is off here.
>
> > +{
> > + BDRVQcow2State *s = bs->opaque;
> > + int ret = 0;
> > + int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
> > + int64_t zone_size = s->zoned_header.zone_size;
> > + int64_t zone_size_mask = zone_size - 1;
> > + uint32_t index = offset / zone_size;
>
> It would be safer to check that this qcow2 image supports zone emulation
> before getting deeper into this function. zone_size could be 0 here if
> zone emulation is not enabled and it would cause a divide-by-zero
> exception.
>
> All block driver zone callbacks in qcow2 should perform this check just
> in case an emulated device exposes a code path where the guest can
> perform a zoned operation on a non-zoned BlockDriverState.
> Alternatively, you could add a generic check in bdrv_co_zone_mgmt() and
> similar functions.
I'll add this check to bdrv_co_zone_mgmt() to prevent the leak.
>
> > + BlockZoneWps *wps = bs->wps;
> > +
> > + if (offset >= capacity) {
> > + error_report("offset %" PRId64 " is equal to or greater than the"
> > + "device capacity %" PRId64 "", offset, capacity);
> > + return -EINVAL;
> > + }
> > +
> > + if (offset & zone_size_mask) {
> > + error_report("sector offset %" PRId64 " is not aligned to zone
> > size"
> > + " %" PRId64 "", offset / 512, zone_size / 512);
> > + return -EINVAL;
> > + }
> > +
> > + if (((offset + len) < capacity && len & zone_size_mask) ||
>
> Here is how the capacity check is expressed without an integer overflow
> in block/block-backend.c:blk_check_byte_request():
>
> if (offset > len || len - offset < bytes) {
> ^^^^^^^^^^^^^^^^^^^^
> return -EIO;
> }
>
> It would be preferrable to use that form to avoid integer overflow.
Nice approach!
>
> > + offset + len > capacity) {
> > + error_report("number of sectors %" PRId64 " is not aligned to zone"
> > + " size %" PRId64 "", len / 512, zone_size / 512);
> > + return -EINVAL;
> > + }
> > +
> > + qemu_co_mutex_lock(&wps->colock);
> > + uint64_t wpv = wps->wp[index];
> > + qemu_co_mutex_unlock(&wps->colock);
> > +
> > + if (QCOW2_ZT_IS_CONV(wpv)) {
> > + /*
> > + * ZONE_RESET_ALL is a global operation that is allowed when the
> > + * starting zone is conventional; the zone reset path itself skips
> > + * conventional zones.
> > + */
> > + if (op != BLK_ZO_RESET || len != capacity) {
> > + error_report("zone mgmt operation 0x%x is not allowed on "
> > + "a conventional zone", op);
> > + return -EIO;
> > + }
> > + }
> > +
> > + switch (op) {
> > + case BLK_ZO_OPEN:
> > + ret = qcow2_open_zone(bs, index);
> > + break;
> > + case BLK_ZO_CLOSE:
> > + ret = qcow2_close_zone(bs, index);
> > + break;
> > + case BLK_ZO_FINISH:
> > + ret = qcow2_finish_zone(bs, index);
> > + break;
> > + case BLK_ZO_RESET:
> > + ret = qcow2_reset_zone(bs, index, len);
> > + break;
> > + default:
> > + error_report("Unsupported zone op: 0x%x", op);
> > + ret = -ENOTSUP;
> > + break;
> > + }
> > + return ret;
> > +}
> > +
> > +static int coroutine_fn GRAPH_RDLOCK
> > +qcow2_co_zone_append(BlockDriverState *bs, int64_t *offset, QEMUIOVector
> > *qiov,
> > + BdrvRequestFlags flags)
> > +{
> > + assert(flags == 0);
> > + int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
> > + int64_t zone_size_mask = bs->bl.zone_size - 1;
> > + int64_t iov_len = 0;
> > + int64_t len = 0;
> > +
> > + if (*offset >= capacity) {
> > + error_report("*offset %" PRId64 " is equal to or greater than the"
> > + "device capacity %" PRId64 "", *offset, capacity);
> > + return -EINVAL;
>
> I'm not sure whether this should be -ENOSPC or -EINVAL. Did you look
> into what is most appropriate? pwrite(2) on Linux returns ENOSPC when
> writing beyond the end of the block device.
>
> This also applies to other places in this patch that check if offset >=
> capacity.
Ah, it should be -ENOSPC. That's also what a zoned null_blk device
returns when writing at/beyond device capacity.
>
> > + }
> > +
> > + /* offset + len should not pass the end of that zone starting from
> > offset */
> > + if (*offset & zone_size_mask) {
>
> The comment doesn't match the check that is performed here.
Will fix it.
Thanks for reviewing!
Sam
