[PATCH v5 00/15] block: fix the zone write granularity and the zone append limit

[PATCH v5 00/15] block: fix the zone write granularity and the zone append limit

[Niklas Cassel]

Hello Stefan, Kevin, and everyone else,

This series fixes how QEMU reports and enforces the two constraints that a
zoned device places on a write to a sequential zone: the write granularity,
and the largest zone append that it accepts. It also fixes two bugs in the
zone append emulation in file-posix, one of which a guest can reach.

Most of it is preparation for Sam Li's zoned qcow2 series. The first two
patches are taken directly from there, as they are unrelated to qcow2.

Neither of the two values that this series touches is derived from
BlockLimits any more, which was Stefan's objection to v4: the guest has
been told the value, so it is part of the migration stream, and a
destination host whose disks report something else breaks the guest. The
write granularity now comes from the logical and the physical block size,
the append limit from a property, and a backend that needs more than the
configuration can express is refused at realize. The conditions that hold
for every backend rather than for a particular one moved into the block
layer, where they also cover callers that are not a device model.

Changes since v4:
- Patch 4 is new: a helper to check if a zone is conventional. The device
  model tested that by reaching into bs->wps->wp[] and decoding the bit
  that the block layer encodes it in, which two patches here would have
  added two more instances of.
- Patch 5: derive the granularity from the logical and the physical block
  size rather than from BlockLimits. Document
  BlockLimits.write_granularity. Dropped Damien's Reviewed-by, as the
  derivation itself changed.
- Patch 7: also refuse a backend that needs a coarser granularity than the
  block sizes can express. Dropped Damien's Reviewed-by, as the patch
  gained a check.
- Patch 9 is new: reject an append larger than a zone. This is the bound
  that the device model gives up by no longer deriving its own limit.
- Patch 11: a max-append-sectors property rather than a value derived from
  the backend. It now precedes patch 12, which v4 had first; that way round
  left a commit that reported a zero limit to the guest.
- Patch 12: remove BlockLimits.max_append_sectors, rather than have
  file-posix report max_hw_transfer for it. Nothing reads the field once
  virtio-blk takes its limit from a property, and file-posix was its only
  writer. Damien, your Reviewed-by was for reporting max_hw_transfer, so I
  have dropped it.
- Patch 15 is new: the CoroutineIOCompletion cleanup.

Niklas Cassel (13):
  block: add a helper for the index of the zone an offset falls in
  block: add a helper to check if a zone is conventional
  hw/block: derive the zone write granularity from the block sizes
  virtio-blk: check the write granularity of writes to sequential zones
  hw/block: reject a zoned device that the block sizes cannot address
  block: reject zone appends that are not a multiple of the sector size
  block: reject a zone append that is larger than a zone
  file-posix: remove the zone append write granularity check
  virtio-blk: add a max-append-sectors property
  block: remove BlockLimits.max_append_sectors
  file-posix: reject a zone append past the device capacity
  file-posix: reject a zone append to a full or conventional zone
  block: do not use CoroutineIOCompletion to hold a return value

Sam Li (2):
  block: widen BlockLimits.zone_size to uint64_t
  virtio-blk: do not merge requests across a zone boundary

 block/block-backend.c             |  11 +++
 block/file-posix.c                |  59 +++++++-----
 block/io.c                        |  79 ++++++++++++----
 hw/block/block.c                  |  68 ++++++++++++++
 hw/block/virtio-blk.c             | 146 ++++++++++++++++++++++++++----
 include/block/block-io.h          |  14 +++
 include/block/block_int-common.h  |  19 +++-
 include/hw/block/block.h          |  14 +++
 include/hw/virtio/virtio-blk.h    |   1 +
 include/system/block-backend-io.h |   1 +
 10 files changed, 349 insertions(+), 63 deletions(-)


base-commit: 3b5d71cc3652fa7f5ecc449f05dae3ead2b718d7
-- 
2.55.0

[PATCH v5 01/15] block: widen BlockLimits.zone_size to uint64_t

[Niklas Cassel]

From: Sam Li <faithilikerun@gmail.com>

The zone-size field in BlockLimits is currently uint32_t, capping
expressible zone sizes at 4 GiB. Real zoned-device protocols
like NVMe ZNS allow larger zones. Widen BlockLimits.zone_size to
uint64_t to match.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c               | 2 +-
 include/block/block_int-common.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9aad156aad..c0d6ee30e4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3598,7 +3598,7 @@ raw_co_zone_append(BlockDriverState *bs,
 
     if (*offset & zone_size_mask) {
         error_report("sector offset %" PRId64 " is not aligned to zone size "
-                     "%" PRId32 "", *offset / 512, bs->bl.zone_size / 512);
+                     "%" PRId64 "", *offset / 512, bs->bl.zone_size / 512);
         return -EINVAL;
     }
 
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 147c08155f..7571ed9968 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -901,7 +901,7 @@ typedef struct BlockLimits {
     BlockZoneModel zoned;
 
     /* zone size expressed in bytes */
-    uint32_t zone_size;
+    uint64_t zone_size;
 
     /* total number of zones */
     uint32_t nr_zones;
-- 
2.55.0

[PATCH v5 02/15] virtio-blk: do not merge requests across a zone boundary

[Niklas Cassel]

From: Sam Li <faithilikerun@gmail.com>

virtio_blk_submit_multireq() fuses adjacent in-zone requests into a
single request. On a zoned backend, a merged zone append request
that straddles a zone boundary is rejected by the device because
each write/read must stay within a single zone (virtio 1.4, 5.2.6.1).

Add a bail condition to the merge coalescer: if combining the
candidate request into the current batch would cross a zone
boundary, flush the current batch and start a new one.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/block-backend.c             | 11 +++++++++++
 hw/block/virtio-blk.c             | 22 +++++++++++++++++++++-
 include/system/block-backend-io.h |  1 +
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 37ba7e9fc4..049e70ddcb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2326,6 +2326,17 @@ uint32_t blk_get_request_alignment(BlockBackend *blk)
     return bs ? bs->bl.request_alignment : BDRV_SECTOR_SIZE;
 }
 
+/*
+ * Returns the zone size in bytes for a zoned backend, or 0 if @blk does
+ * not present zoned geometry.
+ */
+uint64_t blk_get_zone_size(BlockBackend *blk)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    IO_CODE();
+    return bs ? bs->bl.zone_size : 0;
+}
+
 /* Returns the optimal write zeroes alignment, in bytes; guaranteed nonzero */
 uint32_t blk_get_pwrite_zeroes_alignment(BlockBackend *blk)
 {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 61c341ce2c..1b93364b24 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -288,6 +288,9 @@ static void virtio_blk_submit_multireq(VirtIOBlock *s, MultiReqBuffer *mrb)
     int i = 0, start = 0, num_reqs = 0, niov = 0, nb_sectors = 0;
     uint32_t max_transfer;
     int64_t sector_num = 0;
+    uint64_t zone_size = blk_get_zone_size(s->blk);
+    bool zone_cross;
+    int64_t zone_nr_sectors, end_sector;
 
     if (mrb->num_reqs == 1) {
         submit_requests(s, mrb, 0, 1, -1);
@@ -303,17 +306,34 @@ static void virtio_blk_submit_multireq(VirtIOBlock *s, MultiReqBuffer *mrb)
     for (i = 0; i < mrb->num_reqs; i++) {
         VirtIOBlockReq *req = mrb->reqs[i];
         if (num_reqs > 0) {
+            /*
+             * On zoned backends, a single backend write/read must not span
+             * a zone boundary. Bail out of merging if combining req into
+             * the current batch would straddle a zone.
+             */
+            if (zone_size > 0) {
+                zone_nr_sectors = zone_size / BDRV_SECTOR_SIZE;
+                end_sector = req->sector_num
+                                + req->qiov.size / BDRV_SECTOR_SIZE - 1;
+                zone_cross = (sector_num / zone_nr_sectors) !=
+                             (end_sector / zone_nr_sectors);
+            } else {
+                zone_cross = false;
+            }
+
             /*
              * NOTE: We cannot merge the requests in below situations:
              * 1. requests are not sequential
              * 2. merge would exceed maximum number of IOVs
              * 3. merge would exceed maximum transfer length of backend device
+             * 4. merge would cross a zone boundary on a zoned backend
              */
             if (sector_num + nb_sectors != req->sector_num ||
                 niov > blk_get_max_iov(s->blk) - req->qiov.niov ||
                 req->qiov.size > max_transfer ||
                 nb_sectors > (max_transfer -
-                              req->qiov.size) / BDRV_SECTOR_SIZE) {
+                              req->qiov.size) / BDRV_SECTOR_SIZE ||
+                zone_cross) {
                 submit_requests(s, mrb, start, num_reqs, niov);
                 num_reqs = 0;
             }
diff --git a/include/system/block-backend-io.h b/include/system/block-backend-io.h
index fd84723d9d..78d410b8df 100644
--- a/include/system/block-backend-io.h
+++ b/include/system/block-backend-io.h
@@ -121,6 +121,7 @@ uint32_t blk_get_request_alignment(BlockBackend *blk);
 uint32_t blk_get_pwrite_zeroes_alignment(BlockBackend *blk);
 uint32_t blk_get_max_transfer(BlockBackend *blk);
 uint64_t blk_get_max_hw_transfer(BlockBackend *blk);
+uint64_t blk_get_zone_size(BlockBackend *blk);
 
 int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                                    BlockBackend *blk_out, int64_t off_out,
-- 
2.55.0

[PATCH v5 03/15] block: add a helper for the index of the zone an offset falls in

[Niklas Cassel]

Deriving the zone that an offset belongs to is open coded in five places,
in two files, as a division of the offset by BlockLimits.zone_size.

The zone size of a zoned device is always a power of two. Linux requires
that of every zoned device it accepts, in blk_revalidate_disk_zones() and
in each of the drivers that report one, and qcow2 rejects an image whose
header says otherwise. So the division is a shift, and the shift is worth
deriving once rather than at every use: record it in BlockLimits as
zone_size_bits, next to the size it comes from, and assert the property
that it relies on where it is computed.

Add bdrv_zone_index() and use it. No functional change; the count of zones
that a management operation spans stays a division, since it is a length
rather than an offset.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c               |  8 ++++----
 block/io.c                       | 12 ++++++++++++
 hw/block/virtio-blk.c            |  2 +-
 include/block/block-io.h         |  2 ++
 include/block/block_int-common.h |  7 +++++++
 5 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c0d6ee30e4..00323f7a9f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1359,7 +1359,7 @@ static int get_zones_wp(BlockDriverState *bs, int fd, int64_t offset,
     size_t rep_size;
     uint64_t sector = offset >> BDRV_SECTOR_BITS;
     BlockZoneWps *wps = bs->wps;
-    unsigned int j = offset / bs->bl.zone_size;
+    unsigned int j = bdrv_zone_index(bs, offset);
     unsigned int n = 0, i = 0;
     int ret;
     rep_size = sizeof(struct blk_zone_report) + nrz * sizeof(struct blk_zone);
@@ -2560,7 +2560,7 @@ raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes,
         bs->bl.zoned != BLK_Z_NONE) {
         qemu_co_mutex_lock(&bs->wps->colock);
         if (type & QEMU_AIO_ZONE_APPEND) {
-            int index = offset / bs->bl.zone_size;
+            int index = bdrv_zone_index(bs, offset);
             offset = bs->wps->wp[index];
         }
     }
@@ -2615,7 +2615,7 @@ out:
         bs->bl.zoned != BLK_Z_NONE) {
         BlockZoneWps *wps = bs->wps;
         if (ret == 0) {
-            uint64_t *wp = &wps->wp[offset / bs->bl.zone_size];
+            uint64_t *wp = &wps->wp[bdrv_zone_index(bs, offset)];
             if (!BDRV_ZT_IS_CONV(*wp)) {
                 if (type & QEMU_AIO_ZONE_APPEND) {
                     *offset_ptr = *wp;
@@ -3513,7 +3513,7 @@ static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp op,
         return -EINVAL;
     }
 
-    uint32_t i = offset / bs->bl.zone_size;
+    uint32_t i = bdrv_zone_index(bs, offset);
     uint32_t nrz = len / bs->bl.zone_size;
     uint64_t *wp = &wps->wp[i];
     if (BDRV_ZT_IS_CONV(*wp) && len != capacity) {
diff --git a/block/io.c b/block/io.c
index a916b236c3..cef23ee5bc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -227,6 +227,11 @@ void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp)
         }
     }
 
+    if (bs->bl.zone_size) {
+        assert(is_power_of_2(bs->bl.zone_size));
+        bs->bl.zone_size_bits = ctz64(bs->bl.zone_size);
+    }
+
     if (bs->bl.request_alignment > BDRV_MAX_ALIGNMENT) {
         error_setg(errp, "Driver requires too large request alignment");
     }
@@ -3361,6 +3366,13 @@ out:
     return co.ret;
 }
 
+uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset)
+{
+    IO_CODE();
+
+    return offset >> bs->bl.zone_size_bits;
+}
+
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
 {
     IO_CODE();
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 1b93364b24..6e4a7a4076 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -523,7 +523,7 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
             }
         }
 
-        index = offset / bs->bl.zone_size;
+        index = bdrv_zone_index(bs, offset);
         if (BDRV_ZT_IS_CONV(bs->wps->wp[index])) {
             *status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
             return false;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index d34d846bb2..9d1c0eb7fb 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -126,6 +126,8 @@ int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_append(BlockDriverState *bs,
                                                   int64_t *offset,
                                                   QEMUIOVector *qiov,
                                                   BdrvRequestFlags flags);
+/* The index of the zone that @offset falls in. */
+uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset);
 
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 7571ed9968..82aa0c1a6e 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -903,6 +903,13 @@ typedef struct BlockLimits {
     /* zone size expressed in bytes */
     uint64_t zone_size;
 
+    /*
+     * log2 of zone_size, derived by bdrv_refresh_limits(). A zoned device
+     * always has a zone size that is a power of two, so the zone an offset
+     * falls in is a shift rather than a division. See bdrv_zone_index().
+     */
+    uint32_t zone_size_bits;
+
     /* total number of zones */
     uint32_t nr_zones;
 
-- 
2.55.0

[PATCH v5 04/15] block: add a helper to check if a zone is conventional

[Niklas Cassel]

Whether a zone is conventional is encoded in the most significant bit of
the write pointer that the block layer keeps for it, so a caller that
wants to know has to reach into bs->wps->wp[] and apply BDRV_ZT_IS_CONV()
to the raw value. virtio-blk does that, which means a device model both
knows how a block driver stores its write pointers and knows what one of
the bits of a write pointer means.

Add an accessor next to bdrv_zone_index() and use it there, so that the
encoding stays with the code that chose it.

Unlike bdrv_zone_is_full(), this does not need the write pointer lock.
get_zones_wp() only ever sets the bit for a conventional zone and never
clears it, and a zone does not change type, so the answer cannot become
stale. The rest of the word does change, on every write, which is what
the lock is there for.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/io.c               | 7 +++++++
 hw/block/virtio-blk.c    | 2 +-
 include/block/block-io.h | 6 ++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index cef23ee5bc..da61e641ac 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3373,6 +3373,13 @@ uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset)
     return offset >> bs->bl.zone_size_bits;
 }
 
+bool bdrv_zone_is_conv(BlockDriverState *bs, uint32_t index)
+{
+    IO_CODE();
+
+    return BDRV_ZT_IS_CONV(bs->wps->wp[index]);
+}
+
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
 {
     IO_CODE();
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6e4a7a4076..dd69d81532 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -524,7 +524,7 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
         }
 
         index = bdrv_zone_index(bs, offset);
-        if (BDRV_ZT_IS_CONV(bs->wps->wp[index])) {
+        if (bdrv_zone_is_conv(bs, index)) {
             *status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
             return false;
         }
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 9d1c0eb7fb..e469df7061 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -128,6 +128,12 @@ int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_append(BlockDriverState *bs,
                                                   BdrvRequestFlags flags);
 /* The index of the zone that @offset falls in. */
 uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset);
+/*
+ * True if the zone at @index is a conventional zone, which can be written
+ * anywhere in any order. The type of a zone does not change, so unlike
+ * bdrv_zone_is_full() this does not need the write pointer lock.
+ */
+bool bdrv_zone_is_conv(BlockDriverState *bs, uint32_t index);
 
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 
-- 
2.55.0

[PATCH v5 05/15] hw/block: derive the zone write granularity from the block sizes

[Niklas Cassel]

For ZBC/ZAC devices the write granularity is the physical block size, so
a 512e SMR disk exposed through a host_device backend has a logical block
size of 512 and a zone write granularity of 4096. We told the guest
driver 512 while raw_co_zone_append() rejects anything that is not 4096
byte aligned, so the driver saw a plain I/O error for a request it had
been told was valid.

Report the larger of the logical and the physical block size instead.
Both are properties of the device model, so the value that a guest is
given does not change when it is migrated to a host whose disks differ,
which a value taken from BlockLimits would. It is also what a guest
derives for itself: blk_validate_zoned_limits() raises
zone_write_granularity to the logical block size, and for a SCSI disk
sd_zbc_read_zones() takes it from the physical block size.

blkconf_blocksizes() rejects a logical block size larger than the
physical block size, so this is the physical block size in practice. It
is expressed as the larger of the two because that is the constraint
which applies: a guest cannot issue a write finer than the logical block
size, and the medium cannot take one finer than the physical block size.

Add it as a helper next to blkconf_blocksizes(), since it is derived from
a BlockConf, and use it for the zone append offset check in
check_zoned_request(), which validated against bs->bl.write_granularity.
The value reported to the driver and the value that requests are
validated against then cannot drift apart. The helper cannot return zero
because blkconf_blocksizes() always leaves a logical block size behind,
so the check no longer needs to guard against an unset granularity.

Fixes: 4f7366506a96 ("virtio-blk: add zoned storage emulation for zoned devices")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/block.c                 |  5 +++++
 hw/block/virtio-blk.c            | 13 +++++++------
 include/block/block_int-common.h |  7 +++++++
 include/hw/block/block.h         | 13 +++++++++++++
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index f187fa025d..79b0a67cea 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -201,6 +201,11 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
     return true;
 }
 
+uint32_t blkconf_zone_write_granularity(BlockConf *conf)
+{
+    return MAX(conf->logical_block_size, conf->physical_block_size);
+}
+
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp)
 {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index dd69d81532..6bf1049aea 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -516,11 +516,11 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
     }
 
     if (append) {
-        if (bs->bl.write_granularity) {
-            if ((offset % bs->bl.write_granularity) != 0) {
-                *status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
-                return false;
-            }
+        uint32_t wg_mask = blkconf_zone_write_granularity(&s->conf.conf) - 1;
+
+        if (offset & wg_mask) {
+            *status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
+            return false;
         }
 
         index = bdrv_zone_index(bs, offset);
@@ -1276,7 +1276,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
                      bs->bl.max_active_zones);
         virtio_stl_p(vdev, &blkcfg.zoned.max_open_zones,
                      bs->bl.max_open_zones);
-        virtio_stl_p(vdev, &blkcfg.zoned.write_granularity, blk_size);
+        virtio_stl_p(vdev, &blkcfg.zoned.write_granularity,
+                     blkconf_zone_write_granularity(conf));
         virtio_stl_p(vdev, &blkcfg.zoned.max_append_sectors,
                      bs->bl.max_append_sectors);
     } else {
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 82aa0c1a6e..7abd2b12bb 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -922,6 +922,13 @@ typedef struct BlockLimits {
     /* maximum number of active zones */
     uint32_t max_active_zones;
 
+    /*
+     * The granularity that the backend requires of a write to a sequential
+     * zone, in bytes, or zero if it has none. This describes the host, so it
+     * must not be reported to a guest: a frontend reports
+     * blkconf_zone_write_granularity() and checks this against it when the
+     * device is realized.
+     */
     uint32_t write_granularity;
 } BlockLimits;
 
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index df941df19f..c769b1297d 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -114,6 +114,19 @@ bool blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
 bool blkconf_blocksizes(BlockConf *conf, Error **errp);
+/*
+ * The alignment constraint that applies to writes to a sequential zone. A guest
+ * cannot issue a write finer than the logical block size, and the medium cannot
+ * take one finer than the physical block size, so the constraint that applies
+ * is the larger of the two. A frontend must report this value to its guest and
+ * validate requests against it.
+ *
+ * It is derived from the configuration and not from the backend, so that a
+ * guest is not given a property of the host, which would differ after
+ * migration. blkconf_blocksizes() rejects a logical block size larger than the
+ * physical block size, so this is the physical block size in practice.
+ */
+uint32_t blkconf_zone_write_granularity(BlockConf *conf);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
-- 
2.55.0

[PATCH v5 06/15] virtio-blk: check the write granularity of writes to sequential zones

[Niklas Cassel]

All VIRTIO_BLK_T_OUT requests issued to sequential zones and all
VIRTIO_BLK_T_ZONE_APPEND requests must have an offset and a data size
that are multiples of the write granularity reported by the device
(virtio 1.4, 5.2.6.1), and a violation is reported as
VIRTIO_BLK_S_ZONE_UNALIGNED_WP (virtio 1.4, 5.2.6).

Neither request type was fully checked. Zone appends validated only the
offset, while writes were not checked at all.

Check the size of the appended data, and both the offset and the size of
a write, against blkconf_zone_write_granularity(), so that every request the
device accepts is one that the guest driver was told is valid. Writes to
conventional zones keep no alignment constraint beyond the logical block
size. The write path performs the check after virtio_blk_sect_range_ok()
so that the zone index derived from the guest supplied sector is known to
be in range.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/virtio-blk.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6bf1049aea..449de307b4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -392,6 +392,33 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     return true;
 }
 
+/*
+ * Both the offset and the size of a write to a sequential zone must be a
+ * multiple of the write granularity that the device reports. Conventional
+ * zones are not constrained. The zone index is derived from a guest supplied
+ * sector, so this must only be called once virtio_blk_sect_range_ok() has
+ * bounded it.
+ */
+static bool virtio_blk_zone_write_granularity_ok(VirtIOBlock *dev,
+                                                 uint64_t sector, size_t size)
+{
+    BlockDriverState *bs = blk_bs(dev->blk);
+    uint64_t offset = sector << BDRV_SECTOR_BITS;
+    uint32_t wg_mask;
+
+    if (bs->bl.zoned == BLK_Z_NONE) {
+        return true;
+    }
+
+    if (bdrv_zone_is_conv(bs, bdrv_zone_index(bs, offset))) {
+        return true;
+    }
+
+    wg_mask = blkconf_zone_write_granularity(&dev->conf.conf) - 1;
+
+    return !(offset & wg_mask) && !(size & wg_mask);
+}
+
 static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
     struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
 {
@@ -518,7 +545,7 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
     if (append) {
         uint32_t wg_mask = blkconf_zone_write_granularity(&s->conf.conf) - 1;
 
-        if (offset & wg_mask) {
+        if (offset & wg_mask || len & wg_mask) {
             *status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
             return false;
         }
@@ -913,6 +940,15 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
             return 0;
         }
 
+        if (is_write &&
+            !virtio_blk_zone_write_granularity_ok(s, req->sector_num,
+                                                  req->qiov.size)) {
+            virtio_blk_req_complete(req, VIRTIO_BLK_S_ZONE_UNALIGNED_WP);
+            block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
+            g_free(req);
+            return 0;
+        }
+
         block_acct_start(blk_get_stats(s->blk), &req->acct, req->qiov.size,
                          is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
 
-- 
2.55.0

[PATCH v5 07/15] hw/block: reject a zoned device that the block sizes cannot address

[Niklas Cassel]

The write granularity that a frontend reports is derived from the logical
and the physical block size, which are properties of the device model,
while the granularity that the medium requires and the write pointers
that it holds belong to the backend. Nothing ties them together, and
where they disagree a guest is told about a device it cannot use.

A backend can require a coarser granularity than the block sizes express.
A 512e host managed disk has a logical block size of 512 and requires
writes to a sequential zone to be a multiple of 4096, so attaching it
with a finer physical block size reports a granularity that the disk will
not accept, and the guest is given a plain I/O error for a request that
it was told was valid.

The write pointers can disagree in the same way. They outlive any
particular use of the device, while the block sizes are chosen afresh
every time it is attached, so a zone written with logical_block_size=512
leaves a write pointer that logical_block_size=4096 cannot address. A
zone report expresses it in 512 byte sectors, so the guest is told about
a position that does not fall on a logical block boundary, which it can
neither read nor write, and the zone can then only be recovered by
resetting it. The reverse is harmless: a pointer laid down with a larger
logical block size is still a multiple of a smaller one.

Check both at realize time, along with the zone size and the zone
capacity, and refuse to start otherwise. The write pointers are already
held in memory by the driver, so this costs no I/O.

The check uses blkconf_zone_write_granularity(), the same value that a
frontend reports to its guest and validates requests against, so the
three cannot disagree.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/block.c         | 63 ++++++++++++++++++++++++++++++++++++++++
 hw/block/virtio-blk.c    |  4 +++
 include/hw/block/block.h |  1 +
 3 files changed, 68 insertions(+)

diff --git a/hw/block/block.c b/hw/block/block.c
index 79b0a67cea..3bb8121e9e 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -206,6 +206,69 @@ uint32_t blkconf_zone_write_granularity(BlockConf *conf)
     return MAX(conf->logical_block_size, conf->physical_block_size);
 }
 
+bool blkconf_check_zoned_geometry(BlockConf *conf, Error **errp)
+{
+    BlockDriverState *bs = blk_bs(conf->blk);
+    uint32_t wg;
+
+    if (bs->bl.zoned == BLK_Z_NONE) {
+        return true;
+    }
+
+    wg = blkconf_zone_write_granularity(conf);
+
+    /*
+     * A backend that requires a coarser granularity than the one that is about
+     * to be reported would reject writes that the guest has been told are
+     * valid, leaving it with a plain I/O error.
+     */
+    if (bs->bl.write_granularity > wg) {
+        error_setg(errp, "the backend requires writes to sequential zones to "
+                   "be a multiple of %" PRIu32 " bytes, which is coarser than "
+                   "the zone write granularity %" PRIu32 " that would be "
+                   "reported", bs->bl.write_granularity, wg);
+        error_append_hint(errp, "Set physical_block_size=%" PRIu32 ".\n",
+                          bs->bl.write_granularity);
+        return false;
+    }
+
+    if (!QEMU_IS_ALIGNED(bs->bl.zone_size, wg)) {
+        error_setg(errp, "zone size %" PRIu64 " is not a multiple of the zone "
+                   "write granularity %" PRIu32, bs->bl.zone_size, wg);
+        return false;
+    }
+
+    /*
+     * A write pointer that is not a multiple of the write granularity does not
+     * fall on a logical block boundary, so the guest can neither read nor write
+     * at it and the zone can only be recovered by resetting it. A backend that
+     * records its write pointers, rather than reading them back from a device,
+     * can hand us such a pointer when the zones were written while the device
+     * was configured with a smaller logical block size.
+     */
+    for (uint32_t i = 0; i < bs->bl.nr_zones; i++) {
+        uint64_t wp;
+
+        if (bdrv_zone_is_conv(bs, i)) {
+            continue;
+        }
+
+        wp = bs->wps->wp[i];
+        if (!QEMU_IS_ALIGNED(wp, wg)) {
+            error_setg(errp, "write pointer 0x%" PRIx64 " of zone %" PRIu32
+                       " is not a multiple of the zone write granularity %"
+                       PRIu32, wp, i, wg);
+            error_append_hint(errp, "The zones were written with a finer zone "
+                              "write granularity. Reset them, or attach the "
+                              "device with the block sizes that they were "
+                              "written with.\n");
+            return false;
+        }
+    }
+
+    return true;
+}
+
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp)
 {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 449de307b4..74bd08b0ec 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1840,6 +1840,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!blkconf_check_zoned_geometry(&conf->conf, errp)) {
+        return;
+    }
+
     bs = blk_bs(conf->conf.blk);
     if (bs->bl.zoned != BLK_Z_NONE) {
         virtio_add_feature(&s->host_features, VIRTIO_BLK_F_ZONED);
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index c769b1297d..c0a8b84df1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -127,6 +127,7 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp);
  * physical block size, so this is the physical block size in practice.
  */
 uint32_t blkconf_zone_write_granularity(BlockConf *conf);
+bool blkconf_check_zoned_geometry(BlockConf *conf, Error **errp);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
-- 
2.55.0

[PATCH v5 08/15] block: reject zone appends that are not a multiple of the sector size

[Niklas Cassel]

Zone write pointers are tracked and reported in units of
BDRV_SECTOR_SIZE, so an append whose data size is not a multiple of it
would leave a write pointer that cannot be represented, neither in a
BlockZoneDescriptor nor in the virtio and NVMe zone reports derived from
one.

Nothing states that invariant. file-posix, the only driver that carries
out an append itself, does enforce it, but only as a side effect of
checking each iovec against BlockLimits.write_granularity, which is never
smaller than the sector size. That conflates two constraints: the sector
granularity holds for every driver, whereas write_granularity describes a
coarser requirement of the medium below a driver, and only a driver that
has such a medium should express it.

Check the invariant once in bdrv_co_zone_append(), so that it no longer
rests on a driver that happens to have a granularity of its own to
report.

This is a lower bound, not the alignment that a guest has to observe. The
block layer cannot know that one, because it also depends on the logical
block size the device is configured with, which is a property of the
frontend. The alignment that applies to a guest is the larger of the two,
and it is the frontend that reports it, that validates requests against
it, and that has to refuse a device whose write pointers do not satisfy
it.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/io.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/io.c b/block/io.c
index da61e641ac..b9d0b3fd7b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3355,6 +3355,16 @@ int coroutine_fn bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset,
         return ret;
     }
 
+    /*
+     * Zone write pointers are kept and reported in units of BDRV_SECTOR_SIZE,
+     * so an append that would leave a write pointer at a finer granularity
+     * cannot be represented. Drivers may impose a coarser granularity of their
+     * own, see BlockLimits.write_granularity.
+     */
+    if (!QEMU_IS_ALIGNED(qiov->size, BDRV_SECTOR_SIZE)) {
+        return -EINVAL;
+    }
+
     bdrv_inc_in_flight(bs);
     if (!drv || !drv->bdrv_co_zone_append || bs->bl.zoned == BLK_Z_NONE) {
         co.ret = -ENOTSUP;
-- 
2.55.0

[PATCH v5 09/15] block: reject a zone append that is larger than a zone

[Niklas Cassel]

A zone append cannot cross a zone boundary, so a request larger than a
zone can never be carried out.

Nothing rejects one. The block layer does not look at the length at all,
and raw_co_zone_append() writes qiov->size bytes at the write pointer
once it has validated the offset, so the write runs past the end of the
zone: on a disk with 8 MiB zones, an append of 8 MiB plus one block
fills the zone and advances the write pointer of the zone after it.

virtio-blk does compare the length, but against
BlockLimits.max_append_sectors, which is how much the backend takes in
one command and not how large a zone is. For a Linux block device that
value is never larger than a zone, so virtio-blk is protected by what
the backend happens to report rather than by the check it makes.

Check the length in bdrv_co_zone_append(), next to the check that an
append is a multiple of the sector size. Both are conditions of the
operation rather than of a medium, so the block layer is where they hold
for every caller and where no driver has to repeat them.

The exact bound is tighter: a request also has to fit in the part of the
zone that is still writable, which depends on the write pointer and so
stays with the drivers.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/io.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/io.c b/block/io.c
index b9d0b3fd7b..d7c403bc08 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3365,6 +3365,18 @@ int coroutine_fn bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset,
         return -EINVAL;
     }
 
+    /*
+     * An append cannot cross a zone boundary, so one that is larger than a
+     * zone can never be carried out, wherever the write pointer of the zone
+     * is. The exact bound is the part of the zone that is still writable,
+     * which only a driver can check, as only it holds the write pointer.
+     * zone_size is zero when the device is not zoned, which is reported as
+     * unsupported below.
+     */
+    if (bs->bl.zone_size && qiov->size > bs->bl.zone_size) {
+        return -EINVAL;
+    }
+
     bdrv_inc_in_flight(bs);
     if (!drv || !drv->bdrv_co_zone_append || bs->bl.zoned == BLK_Z_NONE) {
         co.ret = -ENOTSUP;
-- 
2.55.0

[PATCH v5 10/15] file-posix: remove the zone append write granularity check

[Niklas Cassel]

The check rejects a zone append whose individual iovec lengths are not
multiples of the zone write granularity. That is stricter than the
constraint it is meant to enforce, which applies to the size of the
request as a whole. A request whose total is properly aligned but which
is split across, say, a 512 byte and a 3584 byte iovec is refused here,
even though the iovec boundaries do not survive into the scatter gather
list that reaches the device.

Nor is the driver the right place to enforce it. The granularity that
holds for every backend is the sector granularity, which is now checked
once in bdrv_co_zone_append(). The coarser granularity that a guest has
to observe depends on the logical block size that the device was
configured with, so only a frontend can know it, and a frontend both
reports it to its guest and validates requests against it.

Drop the check. BlockLimits.write_granularity is then set in one place,
by this driver from the zone_write_granularity queue attribute, and read
in one place, where a device whose granularity the configured block sizes
cannot express is refused.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 00323f7a9f..c1ac49d23d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3593,8 +3593,6 @@ raw_co_zone_append(BlockDriverState *bs,
                    BdrvRequestFlags flags) {
     assert(flags == 0);
     int64_t zone_size_mask = bs->bl.zone_size - 1;
-    int64_t iov_len = 0;
-    int64_t len = 0;
 
     if (*offset & zone_size_mask) {
         error_report("sector offset %" PRId64 " is not aligned to zone size "
@@ -3602,20 +3600,8 @@ raw_co_zone_append(BlockDriverState *bs,
         return -EINVAL;
     }
 
-    int64_t wg = bs->bl.write_granularity;
-    int64_t wg_mask = wg - 1;
-    for (int i = 0; i < qiov->niov; i++) {
-        iov_len = qiov->iov[i].iov_len;
-        if (iov_len & wg_mask) {
-            error_report("len of IOVector[%d] %" PRId64 " is not aligned to "
-                         "block size %" PRId64 "", i, iov_len, wg);
-            return -EINVAL;
-        }
-        len += iov_len;
-    }
-
     trace_zbd_zone_append(bs, *offset >> BDRV_SECTOR_BITS);
-    return raw_co_prw(bs, offset, len, qiov, QEMU_AIO_ZONE_APPEND, 0);
+    return raw_co_prw(bs, offset, qiov->size, qiov, QEMU_AIO_ZONE_APPEND, 0);
 }
 #endif
 
-- 
2.55.0

[PATCH v5 11/15] virtio-blk: add a max-append-sectors property

[Niklas Cassel]

The maximum zone append size that the device reports to its driver was
taken from BlockLimits.max_append_sectors, a limit of the host disk that
the backend happens to sit on. A guest reads it out of the configuration
space, so it becomes part of what the guest has been told about its
device, and migrating that guest to a host whose disks report something
smaller leaves it appending more than the device accepts.

Make it a property, as max-discard-sectors and max-write-zeroes-sectors
already are, so that the value a guest is given is part of the device
model and does not change under it. Nothing from BlockLimits reaches the
configuration space any more.

Zero is a valid value here, unlike for those two. There is no feature bit
for zone append, and a maximum of zero is how the specification has a
device say that it does not support the operation (virtio 1.4, 5.2.5.2).
It is also what a ZBC or ZAC disk is: such a disk has no zone append
command at all, and Linux emulates one in its block layer.

What is reported is capped by the size of a zone. The specification asks
for the largest append that can be carried out, and one larger than a
zone never can be, so the default would otherwise advertise a size that
the block layer refuses. The cap tells a guest nothing about the host
that zone_sectors has not already told it.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/virtio-blk.c          | 67 ++++++++++++++++++++++++++++++----
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 74bd08b0ec..536d71a91a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -521,6 +521,24 @@ typedef struct ZoneCmdData {
     };
 } ZoneCmdData;
 
+/*
+ * The maximum zone append size that the device reports to its driver, in 512
+ * byte sectors.
+ *
+ * An append cannot cross a zone boundary, so the configured maximum is capped
+ * by the size of a zone: the specification asks for the largest append that
+ * can be carried out, and a larger one never can be. The zone size is already
+ * reported to the driver as zone_sectors, so this tells it nothing new about
+ * the host.
+ */
+static uint32_t virtio_blk_max_append_sectors(VirtIOBlock *s)
+{
+    BlockDriverState *bs = blk_bs(s->blk);
+
+    return MIN(s->conf.max_append_sectors,
+               bs->bl.zone_size >> BDRV_SECTOR_BITS);
+}
+
 /*
  * check zoned_request: error checking before issuing requests. If all checks
  * passed, return true.
@@ -556,12 +574,13 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
             return false;
         }
 
-        if (len / 512 > bs->bl.max_append_sectors) {
-            if (bs->bl.max_append_sectors == 0) {
-                *status = VIRTIO_BLK_S_UNSUPP;
-            } else {
-                *status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
-            }
+        if (!s->conf.max_append_sectors) {
+            *status = VIRTIO_BLK_S_UNSUPP;
+            return false;
+        }
+
+        if ((len >> BDRV_SECTOR_BITS) > virtio_blk_max_append_sectors(s)) {
+            *status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
             return false;
         }
     }
@@ -1315,7 +1334,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
         virtio_stl_p(vdev, &blkcfg.zoned.write_granularity,
                      blkconf_zone_write_granularity(conf));
         virtio_stl_p(vdev, &blkcfg.zoned.max_append_sectors,
-                     bs->bl.max_append_sectors);
+                     virtio_blk_max_append_sectors(s));
     } else {
         blkcfg.zoned.model = VIRTIO_BLK_Z_NONE;
     }
@@ -1852,6 +1871,34 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_ZONED)) {
+        uint32_t wg = blkconf_zone_write_granularity(&conf->conf);
+
+        /*
+         * Checked before the granularity below, so that a driver can shift the
+         * value that it reads into a byte count without overflowing.
+         */
+        if (conf->max_append_sectors > BDRV_REQUEST_MAX_SECTORS) {
+            error_setg(errp, "invalid max-append-sectors property (%" PRIu32
+                       "), must not exceed %d", conf->max_append_sectors,
+                       (int)BDRV_REQUEST_MAX_SECTORS);
+            return;
+        }
+
+        /*
+         * Zero is allowed, and says that zone append is not supported, which
+         * is what a ZBC or ZAC disk is. Anything else has to be able to
+         * express a single write.
+         */
+        if (conf->max_append_sectors &&
+            (conf->max_append_sectors << BDRV_SECTOR_BITS) < wg) {
+            error_setg(errp, "invalid max-append-sectors property (%" PRIu32
+                       "), must be zero or at least the zone write granularity "
+                       "(%" PRIu32 " bytes)", conf->max_append_sectors, wg);
+            return;
+        }
+    }
+
     if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD) &&
         (!conf->max_discard_sectors ||
          conf->max_discard_sectors > BDRV_REQUEST_MAX_SECTORS)) {
@@ -1986,6 +2033,12 @@ static const Property virtio_blk_properties[] = {
                        conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
     DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
                        conf.max_write_zeroes_sectors, BDRV_REQUEST_MAX_SECTORS),
+    /*
+     * Zero is valid, and says that the device does not support zone append,
+     * as a ZBC or ZAC disk does not.
+     */
+    DEFINE_PROP_UINT32("max-append-sectors", VirtIOBlock,
+                       conf.max_append_sectors, BDRV_REQUEST_MAX_SECTORS),
     DEFINE_PROP_BOOL("x-enable-wce-if-config-wce", VirtIOBlock,
                      conf.x_enable_wce_if_config_wce, true),
 };
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 3d8dee7ec1..0070ca6ba9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -47,6 +47,7 @@ struct VirtIOBlkConf
     bool report_discard_granularity;
     uint32_t max_discard_sectors;
     uint32_t max_write_zeroes_sectors;
+    uint32_t max_append_sectors;
     bool x_enable_wce_if_config_wce;
 };
 
-- 
2.55.0

[PATCH v5 12/15] block: remove BlockLimits.max_append_sectors

[Niklas Cassel]

The field described the largest zone append that a backend accepts. It
had one writer and one reader, and neither is left.

file-posix filled it from the zone_append_max_bytes queue attribute,
which bounds REQ_OP_ZONE_APPEND, an operation it never issues: Linux has
no userspace interface for one, so raw_co_zone_append() substitutes the
write pointer of the zone for the offset and the request reaches the
host as a plain pwritev().

The transfer limit does not apply either, no more than it does to an
ordinary write. This driver reports max_hw_transfer but never sets
BlockLimits.max_transfer, which is what bdrv_aligned_pwritev() splits
by, so a request of any size goes to the host kernel whole and is split
there: on a null_blk device whose zone_append_max_bytes is 130560, a
16 MiB append completes and advances the write pointer by 16 MiB. That
is safe for a sequential zone, as Linux issues the fragments of a split
write in order, with zone write plugging since 6.10 and zone write
locking before that.

virtio-blk was the reader, and now takes what it reports to a guest from
a property instead.

Remove the field and the assignment that filled it. Keeping it would be
worse than not having it: a driver that set it, believing something
enforced it, would be silently ignored.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c               | 5 -----
 include/block/block_int-common.h | 3 ---
 2 files changed, 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c1ac49d23d..1a4ca9d942 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1488,11 +1488,6 @@ static void raw_refresh_zoned_limits(BlockDriverState *bs, struct stat *st,
     }
     bs->bl.nr_zones = ret;
 
-    ret = get_sysfs_long_val(st, "zone_append_max_bytes");
-    if (ret > 0) {
-        bs->bl.max_append_sectors = ret >> BDRV_SECTOR_BITS;
-    }
-
     ret = get_sysfs_long_val(st, "zone_write_granularity");
     if (ret >= 0) {
         bs->bl.write_granularity = ret;
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 7abd2b12bb..cfc7b0f427 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -913,9 +913,6 @@ typedef struct BlockLimits {
     /* total number of zones */
     uint32_t nr_zones;
 
-    /* maximum sectors of a zone append write operation */
-    uint32_t max_append_sectors;
-
     /* maximum number of open zones */
     uint32_t max_open_zones;
 
-- 
2.55.0

[PATCH v5 13/15] file-posix: reject a zone append past the device capacity

[Niklas Cassel]

raw_co_zone_append() checks that the offset it is given is aligned to the
zone size, but not that it names a zone of the device. raw_co_prw() then
derives a zone index from it and reads that entry of the write pointer
array, so an offset past the end of the device reads past the end of the
array.

bdrv_co_zone_append() does not catch it either: bdrv_check_qiov_request()
bounds the request against BDRV_MAX_LENGTH, which has nothing to do with
the size of this device. A guest cannot reach it, because
check_zoned_request() in virtio-blk rejects an out of range offset first,
but qemu-io and any other caller of blk_co_zone_append() can:

  $ qemu-io --image-opts -n driver=host_device,filename=/dev/nullb0 \
        -c "zap -p 0x100000000000 0x1000"
  Segmentation fault

On a null_blk device with 1000 zones of 256 MiB, that offset yields zone
index 65536 and reads 512 KiB beyond an 8000 byte allocation.

Reject an offset that lies outside the device. That also bounds the
zone index that raw_co_prw() derives from it, so its write pointer
lookup stays inside the array.

Fixes: 4751d09adcc3 ("block: introduce zone append write for zoned devices")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1a4ca9d942..e1e6a03b25 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3587,8 +3587,15 @@ raw_co_zone_append(BlockDriverState *bs,
                    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;
 
+    if (*offset >= capacity) {
+        error_report("*offset %" PRId64 " is equal to or greater than the "
+                     "device capacity %" PRId64 "", *offset, capacity);
+        return -ENOSPC;
+    }
+
     if (*offset & zone_size_mask) {
         error_report("sector offset %" PRId64 " is not aligned to zone size "
                      "%" PRId64 "", *offset / 512, bs->bl.zone_size / 512);
-- 
2.55.0

[PATCH v5 14/15] file-posix: reject a zone append to a full or conventional zone

[Niklas Cassel]

raw_co_prw() replaces the offset of a zone append with the write pointer
of the addressed zone, which assumes that the stored value names a
position inside that zone. It does not in two cases.

A full zone has its write pointer recorded at the end of the zone, since
get_zones_wp() stores start + len for BLK_ZONE_COND_FULL. That is the
first sector of the following zone, so the append is submitted there. The
kernel accepts it whenever that zone is empty, because it is a legal
write at its write pointer, and the completion path advances the wrong
zone because it recomputes the zone index from the replaced offset. The
data is written to a zone that was never addressed and success is
returned:

  zone 2 finished, then a 4 KiB append to zone 2:
  After zap done, the append sector is 0x180000     <- zone 3
  zone 2: wptr 0x180000, zcond:14 (full)
  zone 3: wptr 0x180008                             <- advanced

A conventional zone has no write pointer at all, and its array entry
carries only the type marker in the top bit, so the offset becomes
negative and the write fails with EINVAL. That is harmless but it reports
nothing about the actual mistake.

Reject both while the write pointer lock is held, since the state has to
be read and acted on atomically. check_zoned_request() in virtio-blk
refuses an append to a conventional zone, so that case needs a caller
that goes to the driver directly, but nothing there examines whether a
zone is full, so a guest can reach the misdirected write.

Fixes: 4751d09adcc3 ("block: introduce zone append write for zoned devices")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c       | 23 ++++++++++++++++++++++-
 block/io.c               |  9 +++++++++
 include/block/block-io.h |  6 ++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index e1e6a03b25..cd3c9f0fd0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2555,7 +2555,28 @@ raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes,
         bs->bl.zoned != BLK_Z_NONE) {
         qemu_co_mutex_lock(&bs->wps->colock);
         if (type & QEMU_AIO_ZONE_APPEND) {
-            int index = bdrv_zone_index(bs, offset);
+            uint32_t index = bdrv_zone_index(bs, offset);
+
+            /*
+             * The write pointer of the addressed zone becomes the offset of
+             * the write, so it has to name a position inside that zone. A
+             * conventional zone has no write pointer, and the pointer of a
+             * full zone is reported at the end of the zone. Either would send
+             * the data to a zone that was never addressed.
+             */
+            if (bdrv_zone_is_conv(bs, index)) {
+                error_report("zone append at offset 0x%" PRIx64 " addresses a "
+                             "conventional zone", offset);
+                qemu_co_mutex_unlock(&bs->wps->colock);
+                return -EINVAL;
+            }
+            if (bdrv_zone_is_full(bs, index)) {
+                error_report("zone append at offset 0x%" PRIx64 " addresses a "
+                             "full zone", offset);
+                qemu_co_mutex_unlock(&bs->wps->colock);
+                return -ENOSPC;
+            }
+
             offset = bs->wps->wp[index];
         }
     }
diff --git a/block/io.c b/block/io.c
index d7c403bc08..a4fb56f799 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3402,6 +3402,15 @@ bool bdrv_zone_is_conv(BlockDriverState *bs, uint32_t index)
     return BDRV_ZT_IS_CONV(bs->wps->wp[index]);
 }
 
+bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index)
+{
+    uint64_t zone_end = MIN((uint64_t)(index + 1) * bs->bl.zone_size,
+                            (uint64_t)bs->total_sectors << BDRV_SECTOR_BITS);
+    IO_CODE();
+
+    return bs->wps->wp[index] >= zone_end;
+}
+
 void *qemu_blockalign(BlockDriverState *bs, size_t size)
 {
     IO_CODE();
diff --git a/include/block/block-io.h b/include/block/block-io.h
index e469df7061..f671476f08 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -134,6 +134,12 @@ uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset);
  * bdrv_zone_is_full() this does not need the write pointer lock.
  */
 bool bdrv_zone_is_conv(BlockDriverState *bs, uint32_t index);
+/*
+ * True when the write pointer of a zone has reached the end of the writable
+ * part of that zone, so that nothing more can be written to it until it is
+ * reset. The write pointer lock must be held when called.
+ */
+bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index);
 
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 
-- 
2.55.0

[PATCH v5 15/15] block: do not use CoroutineIOCompletion to hold a return value

[Niklas Cassel]

bdrv_co_zone_report(), bdrv_co_zone_mgmt() and bdrv_co_zone_append() each
declare a CoroutineIOCompletion in order to use its ret field, and never
touch the coroutine field that the rest of the structure exists for.

That structure is for a request which completes through
bdrv_co_io_em_complete(), where the callback has to store the result
somewhere and wake the coroutine that is waiting for it. These three call
into the driver directly and are already running in the coroutine that
wants the result, so there is nothing to wake and nowhere else for the
value to live.

Use a plain int. bdrv_co_zone_append() already has one.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/io.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/block/io.c b/block/io.c
index a4fb56f799..c62c3764d3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3303,40 +3303,36 @@ int coroutine_fn bdrv_co_zone_report(BlockDriverState *bs, int64_t offset,
                         BlockZoneDescriptor *zones)
 {
     BlockDriver *drv = bs->drv;
-    CoroutineIOCompletion co = {
-            .coroutine = qemu_coroutine_self(),
-    };
+    int ret;
     IO_CODE();
 
     bdrv_inc_in_flight(bs);
     if (!drv || !drv->bdrv_co_zone_report || bs->bl.zoned == BLK_Z_NONE) {
-        co.ret = -ENOTSUP;
+        ret = -ENOTSUP;
         goto out;
     }
-    co.ret = drv->bdrv_co_zone_report(bs, offset, nr_zones, zones);
+    ret = drv->bdrv_co_zone_report(bs, offset, nr_zones, zones);
 out:
     bdrv_dec_in_flight(bs);
-    return co.ret;
+    return ret;
 }
 
 int coroutine_fn bdrv_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp op,
         int64_t offset, int64_t len)
 {
     BlockDriver *drv = bs->drv;
-    CoroutineIOCompletion co = {
-            .coroutine = qemu_coroutine_self(),
-    };
+    int ret;
     IO_CODE();
 
     bdrv_inc_in_flight(bs);
     if (!drv || !drv->bdrv_co_zone_mgmt || bs->bl.zoned == BLK_Z_NONE) {
-        co.ret = -ENOTSUP;
+        ret = -ENOTSUP;
         goto out;
     }
-    co.ret = drv->bdrv_co_zone_mgmt(bs, op, offset, len);
+    ret = drv->bdrv_co_zone_mgmt(bs, op, offset, len);
 out:
     bdrv_dec_in_flight(bs);
-    return co.ret;
+    return ret;
 }
 
 int coroutine_fn bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset,
@@ -3345,9 +3341,6 @@ int coroutine_fn bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset,
 {
     int ret;
     BlockDriver *drv = bs->drv;
-    CoroutineIOCompletion co = {
-            .coroutine = qemu_coroutine_self(),
-    };
     IO_CODE();
 
     ret = bdrv_check_qiov_request(*offset, qiov->size, qiov, 0, NULL);
@@ -3379,13 +3372,13 @@ int coroutine_fn bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset,
 
     bdrv_inc_in_flight(bs);
     if (!drv || !drv->bdrv_co_zone_append || bs->bl.zoned == BLK_Z_NONE) {
-        co.ret = -ENOTSUP;
+        ret = -ENOTSUP;
         goto out;
     }
-    co.ret = drv->bdrv_co_zone_append(bs, offset, qiov, flags);
+    ret = drv->bdrv_co_zone_append(bs, offset, qiov, flags);
 out:
     bdrv_dec_in_flight(bs);
-    return co.ret;
+    return ret;
 }
 
 uint32_t bdrv_zone_index(BlockDriverState *bs, uint64_t offset)
-- 
2.55.0

Re: [PATCH v5 00/15] block: fix the zone write granularity and the zone append limit

[Niklas Cassel]

On Fri, Sep 18, 2026 at 07:29:08PM +0200, Niklas Cassel wrote:
> Hello Stefan, Kevin, and everyone else,
> 
> This series fixes how QEMU reports and enforces the two constraints that a
> zoned device places on a write to a sequential zone: the write granularity,
> and the largest zone append that it accepts. It also fixes two bugs in the
> zone append emulation in file-posix, one of which a guest can reach.
> 
> Most of it is preparation for Sam Li's zoned qcow2 series. The first two
> patches are taken directly from there, as they are unrelated to qcow2.
> 
> Neither of the two values that this series touches is derived from
> BlockLimits any more, which was Stefan's objection to v4: the guest has
> been told the value, so it is part of the migration stream, and a
> destination host whose disks report something else breaks the guest. The
> write granularity now comes from the logical and the physical block size,
> the append limit from a property, and a backend that needs more than the
> configuration can express is refused at realize. The conditions that hold
> for every backend rather than for a particular one moved into the block
> layer, where they also cover callers that are not a device model.
> 
> Changes since v4:
> - Patch 4 is new: a helper to check if a zone is conventional. The device
>   model tested that by reaching into bs->wps->wp[] and decoding the bit
>   that the block layer encodes it in, which two patches here would have
>   added two more instances of.
> - Patch 5: derive the granularity from the logical and the physical block
>   size rather than from BlockLimits. Document
>   BlockLimits.write_granularity. Dropped Damien's Reviewed-by, as the
>   derivation itself changed.
> - Patch 7: also refuse a backend that needs a coarser granularity than the
>   block sizes can express. Dropped Damien's Reviewed-by, as the patch
>   gained a check.
> - Patch 9 is new: reject an append larger than a zone. This is the bound
>   that the device model gives up by no longer deriving its own limit.
> - Patch 11: a max-append-sectors property rather than a value derived from
>   the backend. It now precedes patch 12, which v4 had first; that way round
>   left a commit that reported a zero limit to the guest.
> - Patch 12: remove BlockLimits.max_append_sectors, rather than have
>   file-posix report max_hw_transfer for it. Nothing reads the field once
>   virtio-blk takes its limit from a property, and file-posix was its only
>   writer. Damien, your Reviewed-by was for reporting max_hw_transfer, so I
>   have dropped it.
> - Patch 15 is new: the CoroutineIOCompletion cleanup.

Stefan, it seems like my tooling failed to pick up your tags because they
were multipart messages.... I'm very sorry about that.

Patches that you have reviewed, and are supposed to have your R-b tag:
1, 2, 3, 6, 8, 10, 13, 14



Patches that you need to review - changed so much that I dropped tags:
7, 12

Patches that you need to review - patches that you had comments on:
5, 11

Patches that you need to review - patches that are new:
4, 9, 15



Kind regards,
Niklas