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

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

[Niklas Cassel]

Hello Stefan,

This series fixes how QEMU reports and enforces the two constraints a zoned
device puts on a write to a sequential zone: the write granularity, and
the largest zone append it accepts. It also fixes two bugs in the zone
append emulation in file-posix.

Many of these patches are in preparation for Sam Li's zoned qcow2 series.
The first two patches in the series are taken directly from there, as they
are unrelated to qcow2.

Patches 3 to 6 concern the write granularity. file-posix read it from the
wrong queue attribute, and virtio-blk reported the logical block size while
the driver enforced the backend value, so on a 512e SMR disk a guest could
be told that a request was valid and get an I/O error for it. Writes to
sequential zones were not checked against the granularity at all, and zone
appends had only their offset checked, not their length. Patch 6 refuses at
realize a device whose write pointers the configured logical block size
cannot address.

Patches 7 to 10 concern the append size. Passing
BlockLimits.max_append_sectors straight through made an unset field mean
"zone append unsupported" rather than "no limit of its own", and Linux
refuses to attach a zoned device that reports zero. The sector invariant
moves to bdrv_co_zone_append(), file-posix drops its duplicate check, and
it stops reporting zone_append_max_bytes, which bounds REQ_OP_ZONE_APPEND,
an operation it never issues: it appends with an ordinary pwritev().

Patches 11 and 12 fix the write pointer that raw_co_prw() substitutes for
the offset of an append. An offset that is never bounded against the device
derives an out of range zone index and reads past the write pointer array,
which qemu-io can reach. An append to a full zone uses a pointer recorded
at the end of the zone, so the data is written into the next zone and
success is returned; a guest can reach that one, because nothing in
virtio-blk checks whether a zone is full.

Niklas Cassel (10):
  file-posix: fix zone write granularity assignment for zoned block
    devices
  virtio-blk: report the effective zone write granularity
  virtio-blk: check the write granularity of writes to sequential zones
  hw/block: reject a zoned device whose write pointers are unaddressable
  block: reject zone appends that are not a multiple of the sector size
  file-posix: remove the zone append write granularity check
  file-posix: base the zone append limit on the transfer limit
  virtio-blk: derive the maximum zone append size
  file-posix: reject a zone append past the device capacity
  file-posix: reject a zone append to a full or conventional zone

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

 block/block-backend.c             | 11 ++++
 block/file-posix.c                | 70 +++++++++++++++--------
 block/io.c                        | 10 ++++
 hw/block/block.c                  | 53 ++++++++++++++++++
 hw/block/virtio-blk.c             | 93 ++++++++++++++++++++++++++-----
 include/block/block_int-common.h  |  2 +-
 include/hw/block/block.h          |  9 +++
 include/system/block-backend-io.h |  1 +
 8 files changed, 211 insertions(+), 38 deletions(-)

-- 
2.55.0

[PATCH 01/12] 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>
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 01037f4b5c..dd9e3d4c37 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 02/12] virtio-blk: do not merge writes 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 must stay within a single zone.

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: 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 6b92066aff..f74cc3dbd0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -294,6 +294,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_sector, end_sector;
 
     if (mrb->num_reqs == 1) {
         submit_requests(s, mrb, 0, 1, -1);
@@ -309,17 +312,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) {
+            zone_cross = false;
+
+            /*
+             * 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_sector = zone_size / BDRV_SECTOR_SIZE;
+                end_sector = req->sector_num
+                                + req->qiov.size / BDRV_SECTOR_SIZE - 1;
+                zone_cross = (sector_num / zone_sector) !=
+                             (end_sector / zone_sector);
+            }
+
             /*
              * 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 03/12] file-posix: fix zone write granularity assignment for zoned block devices

[Niklas Cassel]

In the ZBC and ZAC specifications, the requirement is that the write
granularity matches the physical block size.

However, in the ZNS specification, the requirement is simply that the
write granularity matches the logical block size.

See:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a805a4fa4fa376bbc145762bb8b09caa2fa8af48

In Linux, there is a zone_write_granularity sysfs property which abstracts
this away, so let's make use of it.

Currently, it is theoretically possible that QEMU presents an inflated
write granularity for NVMe devices where the physical block size is larger
than the logical block size.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Fixes: a3c41f06d5a8 ("file-posix: add tracking of the zone write pointers")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index dd9e3d4c37..c0d6ee30e4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1493,7 +1493,7 @@ static void raw_refresh_zoned_limits(BlockDriverState *bs, struct stat *st,
         bs->bl.max_append_sectors = ret >> BDRV_SECTOR_BITS;
     }
 
-    ret = get_sysfs_long_val(st, "physical_block_size");
+    ret = get_sysfs_long_val(st, "zone_write_granularity");
     if (ret >= 0) {
         bs->bl.write_granularity = ret;
     }
-- 
2.55.0

[PATCH 04/12] virtio-blk: report the effective zone write granularity

[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 backend granularity and the logical block size
instead. The guest driver cannot issue writes finer than the logical
size, so the larger of the two is the constraint that applies. This
matches what a Linux guest derives for itself: blk_validate_zoned_limits()
raises zone_write_granularity to the logical block size, and
blk_stack_limits() stacks it with max().

Add it as a helper next to blkconf_blocksizes(), since it is derived from
a BlockConf and the limits of the backend below it, 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         |  7 +++++++
 hw/block/virtio-blk.c    | 13 +++++++------
 include/hw/block/block.h |  8 ++++++++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index f187fa025d..1c3135843d 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -201,6 +201,13 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
     return true;
 }
 
+uint32_t blkconf_zone_write_granularity(BlockConf *conf)
+{
+    BlockDriverState *bs = blk_bs(conf->blk);
+
+    return MAX(bs->bl.write_granularity, conf->logical_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 f74cc3dbd0..f8cda1baa7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -520,11 +520,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 = offset / bs->bl.zone_size;
@@ -1274,7 +1274,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/hw/block/block.h b/include/hw/block/block.h
index df941df19f..f98525c01a 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -114,6 +114,14 @@ 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. The
+ * medium may require a coarser granularity than the logical block size, while
+ * a guest cannot issue writes finer than the logical 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.
+ */
+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 05/12] 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.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/virtio-blk.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f8cda1baa7..7977f4abe5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -522,7 +522,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;
         }
@@ -911,6 +911,29 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
             return 0;
         }
 
+        if (is_write) {
+            BlockDriverState *bs = blk_bs(s->blk);
+            int64_t offset = req->sector_num << BDRV_SECTOR_BITS;
+            uint32_t wg_mask =
+                blkconf_zone_write_granularity(&s->conf.conf) - 1;
+
+            /*
+             * Both the offset and the size of a write to a sequential zone
+             * must be a multiple of the write granularity reported by the
+             * device. Conventional zones are not constrained. The zone index
+             * is derived from a guest supplied sector, so this must come after
+             * virtio_blk_sect_range_ok() has bounded it.
+             */
+            if (bs->bl.zoned != BLK_Z_NONE &&
+                (offset & wg_mask || req->qiov.size & wg_mask) &&
+                !BDRV_ZT_IS_CONV(bs->wps->wp[offset / bs->bl.zone_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 06/12] hw/block: reject a zoned device whose write pointers are unaddressable

[Niklas Cassel]

The write pointers of a zoned device outlive any particular use of it,
whether the device keeps them itself or a backend records them, while the
logical block size is a property of the frontend and is chosen afresh
every time the device is attached. Nothing ties the two together. A zone
written while the device was configured with logical_block_size=512
leaves a write pointer that is a multiple of 512, and attaching the same
device with logical_block_size=4096 makes that pointer unaddressable.

Such a pointer is not merely misaligned. The guest addresses the device
in logical blocks, and a zone report expresses the write pointer in 512
byte sectors, so the guest is told about a position that does not fall on
a logical block boundary. It can neither read nor write there, and the
zone can only be recovered by resetting it. The reverse direction is
harmless: a pointer laid down with a larger logical block size is still a
multiple of a smaller one.

On a zoned null_blk device with a logical block size of 512, a 512 byte
append to a sequential zone leaves the write pointer half a logical block
into it:

  $ qemu-io --image-opts -n driver=host_device,filename=/dev/nullb0 \
        -c "zap -p 0x20000000 0x200" -c "zrp 0x20000000 1"
  start: 0x100000, len 0x80000, cap 0x80000, wptr 0x100001, zcond:2

Attaching that disk with logical_block_size=4096 handed the guest a zone
it could not write to.

Check at realize time that the zone size and every write pointer of a
sequential zone are multiples of the write granularity that the device is
about to report, 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         | 46 ++++++++++++++++++++++++++++++++++++++++
 hw/block/virtio-blk.c    |  4 ++++
 include/hw/block/block.h |  1 +
 3 files changed, 51 insertions(+)

diff --git a/hw/block/block.c b/hw/block/block.c
index 1c3135843d..b908e0ef64 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -208,6 +208,52 @@ uint32_t blkconf_zone_write_granularity(BlockConf *conf)
     return MAX(bs->bl.write_granularity, conf->logical_block_size);
 }
 
+bool blkconf_zoned(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);
+
+    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 = bs->wps->wp[i];
+
+        if (BDRV_ZT_IS_CONV(wp)) {
+            continue;
+        }
+
+        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 smaller "
+                              "logical_block_size. Reset them, or keep using "
+                              "the smaller size.\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 7977f4abe5..61f7b3cdc1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1821,6 +1821,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!blkconf_zoned(&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 f98525c01a..3c96de3222 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -122,6 +122,7 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp);
  * value to its guest and validate requests against it.
  */
 uint32_t blkconf_zone_write_granularity(BlockConf *conf);
+bool blkconf_zoned(BlockConf *conf, Error **errp);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
-- 
2.55.0

[PATCH 07/12] 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.

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 a916b236c3..705dc73d76 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3350,6 +3350,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 08/12] 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 kernel and the
device validate writes to a sequential zone themselves, which is why the
same function already passes the request length down without comparing it
against BlockLimits.max_append_sectors. The sector granularity that holds
for every backend is now checked once in bdrv_co_zone_append(), and a
frontend enforces the granularity it advertises to its guest.

Drop the check. BlockLimits.write_granularity is now set in one place, by
this driver from the zone_write_granularity queue attribute, and read in
one place, by the frontend that reports it.

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 c0d6ee30e4..f267513a4e 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 09/12] file-posix: base the zone append limit on the transfer limit

[Niklas Cassel]

The zone_append_max_bytes queue attribute is the largest
REQ_OP_ZONE_APPEND that the device accepts, and Linux has no interface
for issuing one from userspace: include/uapi has no such operation, and
every submitter of REQ_OP_ZONE_APPEND is in the kernel. Userspace writes
to a sequential zone with an ordinary write at the write pointer.

That is what this driver does. raw_co_zone_append() substitutes the write
pointer of the zone for the offset and hands the request to raw_co_prw(),
which reaches handle_aiocb_rw_vector() and issues a plain pwritev(). The
kernel never sees a zone append, so the attribute describes a limit on an
operation that is never issued.

Nor is it a limit that this driver runs into. The kernel splits a write
that exceeds the transfer limit rather than refusing it, so a larger
append succeeds: 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. Reporting the attribute only understates what the driver can do,
because Linux derives it as a minimum that already includes max_sectors
and chunk_sectors.

Report max_hw_transfer instead, the limit that governs the write the
driver actually issues. There is no use in telling a guest that it may
append more than the device carries in one command, and a frontend then
does not have to reason about how this driver implements an append in
order to bound the value it advertises.

On a null_blk device with max_hw_sectors_kb of 127 and
zone_append_max_bytes of 130560, virtio-blk reports 255 sectors before
and after.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index f267513a4e..e019cc3cd8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1488,10 +1488,15 @@ 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;
-    }
+    /*
+     * raw_co_zone_append() carries out an append as an ordinary write at the
+     * write pointer, so the zone_append_max_bytes attribute, which bounds an
+     * operation that this driver never issues, does not apply. The kernel
+     * splits a write that is larger than the transfer limit rather than
+     * refusing it, but there is no use in telling a guest that it may append
+     * more than the device carries in one command.
+     */
+    bs->bl.max_append_sectors = bs->bl.max_hw_transfer >> BDRV_SECTOR_BITS;
 
     ret = get_sysfs_long_val(st, "zone_write_granularity");
     if (ret >= 0) {
-- 
2.55.0

[PATCH 10/12] virtio-blk: derive the maximum zone append size

[Niklas Cassel]

The max_append_sectors field of virtio_blk_zoned_characteristics must be
set by the device to the largest zone append request that can be issued
to it, and a value of zero tells the guest driver that zone append is not
supported at all (virtio 1.4, 5.2.5.2). Linux refuses to attach a zoned
device that reports zero.

We pass BlockLimits.max_append_sectors straight through, which makes that
field mean "zone append unsupported" when it is unset, rather than "this
backend imposes no limit of its own". Only a backend that has a limit of
its own has anything to put there.

Derive the value instead. A backend limit is honoured when there is one,
and otherwise the request is bounded by the zone size, since an append
cannot cross a zone boundary, and by the largest request the block layer
can carry. The result cannot be zero.

A backend that carries out an append itself, rather than passing it to a
device that has a limit of its own, is the one that knows how large a
request its implementation can take, so it reports that in
BlockLimits.max_append_sectors and this does not have to guess at it.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 hw/block/virtio-blk.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 61f7b3cdc1..6e9ee37ffc 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -498,6 +498,27 @@ typedef struct ZoneCmdData {
     };
 } ZoneCmdData;
 
+/*
+ * The maximum zone append data size that the device reports to the driver in
+ * virtio_blk_zoned_characteristics, in 512 byte sectors.
+ *
+ * A backend that has no limit of its own leaves BlockLimits.max_append_sectors
+ * at zero, in which case the limit is whatever else bounds the request: an
+ * append cannot cross a zone boundary, and the block layer cannot carry a
+ * larger one. The result is never zero, which the driver would read as zone
+ * append not being supported at all.
+ */
+static uint32_t virtio_blk_max_append_sectors(VirtIOBlock *s)
+{
+    BlockDriverState *bs = blk_bs(s->blk);
+    uint64_t sectors;
+
+    sectors = MIN_NON_ZERO(bs->bl.zone_size >> BDRV_SECTOR_BITS,
+                           bs->bl.max_append_sectors);
+
+    return MIN_NON_ZERO(sectors, BDRV_REQUEST_MAX_SECTORS);
+}
+
 /*
  * check zoned_request: error checking before issuing requests. If all checks
  * passed, return true.
@@ -533,12 +554,8 @@ 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 ((len >> BDRV_SECTOR_BITS) > virtio_blk_max_append_sectors(s)) {
+            *status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
             return false;
         }
     }
@@ -1300,7 +1317,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;
     }
-- 
2.55.0

[PATCH 11/12] 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")
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 e019cc3cd8..85d735c079 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3597,8 +3597,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 12/12] 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")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 block/file-posix.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 85d735c079..135e60751f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2565,8 +2565,34 @@ 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;
-            offset = bs->wps->wp[index];
+            uint32_t index = offset / bs->bl.zone_size;
+            uint64_t wp = bs->wps->wp[index];
+            uint64_t zone_end =
+                MIN((uint64_t)(index + 1) * bs->bl.zone_size,
+                    (uint64_t)bs->total_sectors << BDRV_SECTOR_BITS);
+
+            /*
+             * The write pointer of the addressed zone becomes the offset of
+             * the write, so it has to name a position inside that zone. It
+             * does not for a conventional zone, which has no write pointer and
+             * stores a type marker in the top bit instead, and it does not for
+             * a full zone, whose write pointer is reported at the zone end.
+             * Either would send the data to a zone that was never addressed.
+             */
+            if (BDRV_ZT_IS_CONV(wp)) {
+                error_report("zone append at offset 0x%" PRIx64 " addresses a "
+                             "conventional zone", offset);
+                qemu_co_mutex_unlock(&bs->wps->colock);
+                return -EINVAL;
+            }
+            if (wp >= zone_end) {
+                error_report("zone append at offset 0x%" PRIx64 " addresses a "
+                             "full zone", offset);
+                qemu_co_mutex_unlock(&bs->wps->colock);
+                return -ENOSPC;
+            }
+
+            offset = wp;
         }
     }
 #endif
-- 
2.55.0

Re: [PATCH 01/12] block: widen BlockLimits.zone_size to uint64_t

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 02/12] virtio-blk: do not merge writes across a zone boundary

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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 must stay within a single zone.
> 
> 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: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

A couple of minor nit below.

> @@ -309,17 +312,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) {
> +            zone_cross = false;

Move this as a else of the if below.

> +
> +            /*
> +             * 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_sector = zone_size / BDRV_SECTOR_SIZE;

Super confusing name. Can we have this be zone_nr_sectors ?

> +                end_sector = req->sector_num
> +                                + req->qiov.size / BDRV_SECTOR_SIZE - 1;
> +                zone_cross = (sector_num / zone_sector) !=
> +                             (end_sector / zone_sector);
> +            }

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 04/12] virtio-blk: report the effective zone write granularity

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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 backend granularity and the logical block size
> instead. The guest driver cannot issue writes finer than the logical
> size, so the larger of the two is the constraint that applies. This
> matches what a Linux guest derives for itself: blk_validate_zoned_limits()
> raises zone_write_granularity to the logical block size, and
> blk_stack_limits() stacks it with max().
> 
> Add it as a helper next to blkconf_blocksizes(), since it is derived from
> a BlockConf and the limits of the backend below it, 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>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 05/12] virtio-blk: check the write granularity of writes to sequential zones

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  hw/block/virtio-blk.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f8cda1baa7..7977f4abe5 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -522,7 +522,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;
>          }
> @@ -911,6 +911,29 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>              return 0;
>          }
>  
> +        if (is_write) {
> +            BlockDriverState *bs = blk_bs(s->blk);
> +            int64_t offset = req->sector_num << BDRV_SECTOR_BITS;
> +            uint32_t wg_mask =
> +                blkconf_zone_write_granularity(&s->conf.conf) - 1;

Getting this without having first checked that this is a write to a zoned disk
is odd...

> +
> +            /*
> +             * Both the offset and the size of a write to a sequential zone
> +             * must be a multiple of the write granularity reported by the
> +             * device. Conventional zones are not constrained. The zone index
> +             * is derived from a guest supplied sector, so this must come after
> +             * virtio_blk_sect_range_ok() has bounded it.
> +             */
> +            if (bs->bl.zoned != BLK_Z_NONE &&

"!=" ??? Why ? The write granularity alignment applies only to zoned disks, no?
So why is this not "bs->bl.zoned == BLK_Z_NONE" ?

> +                (offset & wg_mask || req->qiov.size & wg_mask) &&
> +                !BDRV_ZT_IS_CONV(bs->wps->wp[offset / bs->bl.zone_size])) {

This condition is a little complex. A little inline helper
"virtio_blk_zoned_write_is_aligned()" or something like that would be nice.

> +                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);
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 05/12] virtio-blk: check the write granularity of writes to sequential zones

[Damien Le Moal]

On 8/28/26 15:07, Damien Le Moal wrote:
> On 8/26/26 05:57, Niklas Cassel wrote:
>> 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.
>>
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>  hw/block/virtio-blk.c | 25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index f8cda1baa7..7977f4abe5 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -522,7 +522,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;
>>          }
>> @@ -911,6 +911,29 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>>              return 0;
>>          }
>>  
>> +        if (is_write) {
>> +            BlockDriverState *bs = blk_bs(s->blk);
>> +            int64_t offset = req->sector_num << BDRV_SECTOR_BITS;
>> +            uint32_t wg_mask =
>> +                blkconf_zone_write_granularity(&s->conf.conf) - 1;
> 
> Getting this without having first checked that this is a write to a zoned disk
> is odd...
> 
>> +
>> +            /*
>> +             * Both the offset and the size of a write to a sequential zone
>> +             * must be a multiple of the write granularity reported by the
>> +             * device. Conventional zones are not constrained. The zone index
>> +             * is derived from a guest supplied sector, so this must come after
>> +             * virtio_blk_sect_range_ok() has bounded it.
>> +             */
>> +            if (bs->bl.zoned != BLK_Z_NONE &&
> 
> "!=" ??? Why ? The write granularity alignment applies only to zoned disks, no?
> So why is this not "bs->bl.zoned == BLK_Z_NONE" ?

Doh! My bad. I was reading "NONE" as "ZONE"... 90 degrees off on the first
letter rotation :)

> 
>> +                (offset & wg_mask || req->qiov.size & wg_mask) &&
>> +                !BDRV_ZT_IS_CONV(bs->wps->wp[offset / bs->bl.zone_size])) {
> 
> This condition is a little complex. A little inline helper
> "virtio_blk_zoned_write_is_aligned()" or something like that would be nice.
> 
>> +                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);
>>  
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 06/12] hw/block: reject a zoned device whose write pointers are unaddressable

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> The write pointers of a zoned device outlive any particular use of it,
> whether the device keeps them itself or a backend records them, while the
> logical block size is a property of the frontend and is chosen afresh
> every time the device is attached. Nothing ties the two together. A zone
> written while the device was configured with logical_block_size=512
> leaves a write pointer that is a multiple of 512, and attaching the same
> device with logical_block_size=4096 makes that pointer unaddressable.
> 
> Such a pointer is not merely misaligned. The guest addresses the device
> in logical blocks, and a zone report expresses the write pointer in 512
> byte sectors, so the guest is told about a position that does not fall on
> a logical block boundary. It can neither read nor write there, and the
> zone can only be recovered by resetting it. The reverse direction is
> harmless: a pointer laid down with a larger logical block size is still a
> multiple of a smaller one.
> 
> On a zoned null_blk device with a logical block size of 512, a 512 byte
> append to a sequential zone leaves the write pointer half a logical block
> into it:
> 
>   $ qemu-io --image-opts -n driver=host_device,filename=/dev/nullb0 \
>         -c "zap -p 0x20000000 0x200" -c "zrp 0x20000000 1"
>   start: 0x100000, len 0x80000, cap 0x80000, wptr 0x100001, zcond:2
> 
> Attaching that disk with logical_block_size=4096 handed the guest a zone
> it could not write to.
> 
> Check at realize time that the zone size and every write pointer of a
> sequential zone are multiples of the write granularity that the device is
> about to report, 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         | 46 ++++++++++++++++++++++++++++++++++++++++
>  hw/block/virtio-blk.c    |  4 ++++
>  include/hw/block/block.h |  1 +
>  3 files changed, 51 insertions(+)
> 
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 1c3135843d..b908e0ef64 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -208,6 +208,52 @@ uint32_t blkconf_zone_write_granularity(BlockConf *conf)
>      return MAX(bs->bl.write_granularity, conf->logical_block_size);
>  }
>  
> +bool blkconf_zoned(BlockConf *conf, Error **errp)
> +{
> +    BlockDriverState *bs = blk_bs(conf->blk);
> +    uint32_t wg;
> +
> +    if (bs->bl.zoned == BLK_Z_NONE) {

This test and the name of the function are making things very confusing...
Maybe rename the function: blkconf_zoned_alignement_ok() or something like that?

> +        return true;
> +    }
> +
> +    wg = blkconf_zone_write_granularity(conf);
> +
> +    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 = bs->wps->wp[i];
> +
> +        if (BDRV_ZT_IS_CONV(wp)) {
> +            continue;
> +        }
> +
> +        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 smaller "
> +                              "logical_block_size. Reset them, or keep using "
> +                              "the smaller size.\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 7977f4abe5..61f7b3cdc1 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1821,6 +1821,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (!blkconf_zoned(&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 f98525c01a..3c96de3222 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -122,6 +122,7 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp);
>   * value to its guest and validate requests against it.
>   */
>  uint32_t blkconf_zone_write_granularity(BlockConf *conf);
> +bool blkconf_zoned(BlockConf *conf, Error **errp);
>  bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
>                                     bool resizable, Error **errp);
>  


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 07/12] block: reject zone appends that are not a multiple of the sector size

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 08/12] file-posix: remove the zone append write granularity check

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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 kernel and the
> device validate writes to a sequential zone themselves, which is why the
> same function already passes the request length down without comparing it
> against BlockLimits.max_append_sectors. The sector granularity that holds
> for every backend is now checked once in bdrv_co_zone_append(), and a
> frontend enforces the granularity it advertises to its guest.
> 
> Drop the check. BlockLimits.write_granularity is now set in one place, by
> this driver from the zone_write_granularity queue attribute, and read in
> one place, by the frontend that reports it.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 09/12] file-posix: base the zone append limit on the transfer limit

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> The zone_append_max_bytes queue attribute is the largest
> REQ_OP_ZONE_APPEND that the device accepts, and Linux has no interface
> for issuing one from userspace: include/uapi has no such operation, and
> every submitter of REQ_OP_ZONE_APPEND is in the kernel. Userspace writes
> to a sequential zone with an ordinary write at the write pointer.
> 
> That is what this driver does. raw_co_zone_append() substitutes the write
> pointer of the zone for the offset and hands the request to raw_co_prw(),
> which reaches handle_aiocb_rw_vector() and issues a plain pwritev(). The
> kernel never sees a zone append, so the attribute describes a limit on an
> operation that is never issued.
> 
> Nor is it a limit that this driver runs into. The kernel splits a write
> that exceeds the transfer limit rather than refusing it, so a larger
> append succeeds: 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. Reporting the attribute only understates what the driver can do,
> because Linux derives it as a minimum that already includes max_sectors
> and chunk_sectors.
> 
> Report max_hw_transfer instead, the limit that governs the write the
> driver actually issues. There is no use in telling a guest that it may
> append more than the device carries in one command, and a frontend then
> does not have to reason about how this driver implements an append in
> order to bound the value it advertises.
> 
> On a null_blk device with max_hw_sectors_kb of 127 and
> zone_append_max_bytes of 130560, virtio-blk reports 255 sectors before
> and after.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 10/12] virtio-blk: derive the maximum zone append size

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> The max_append_sectors field of virtio_blk_zoned_characteristics must be
> set by the device to the largest zone append request that can be issued
> to it, and a value of zero tells the guest driver that zone append is not
> supported at all (virtio 1.4, 5.2.5.2). Linux refuses to attach a zoned
> device that reports zero.
> 
> We pass BlockLimits.max_append_sectors straight through, which makes that
> field mean "zone append unsupported" when it is unset, rather than "this
> backend imposes no limit of its own". Only a backend that has a limit of
> its own has anything to put there.
> 
> Derive the value instead. A backend limit is honoured when there is one,
> and otherwise the request is bounded by the zone size, since an append
> cannot cross a zone boundary, and by the largest request the block layer
> can carry. The result cannot be zero.
> 
> A backend that carries out an append itself, rather than passing it to a
> device that has a limit of its own, is the one that knows how large a
> request its implementation can take, so it reports that in
> BlockLimits.max_append_sectors and this does not have to guess at it.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 11/12] file-posix: reject a zone append past the device capacity

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 12/12] file-posix: reject a zone append to a full or conventional zone

[Damien Le Moal]

On 8/26/26 05:57, Niklas Cassel wrote:
> 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")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

It would be nice to have a helper to test if a zone is full...
Nevertheless,

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research