On 3/28/23 11:29, Dmitry Fomichev wrote: > The merged patch series to support zoned block devices in virtio-blk > is not the most up to date version. The merged patch can be found at > > https://lore.kernel.org/linux-block/20221016034127.330942-3-dmitry.fomichev@xxxxxxx/ > > , but the latest and reviewed version is > > https://lore.kernel.org/linux-block/20221110053952.3378990-3-dmitry.fomichev@xxxxxxx/ > > The differences between the two are mostly cleanups, but there is one > change that is very important in terms of compatibility with the > approved virtio-zbd specification. > > Before it was approved, the OASIS virtio spec had a change in > VIRTIO_BLK_T_ZONE_APPEND request layout that is not reflected in the > current virtio-blk driver code. In the running code, the status is > the first byte of the in-header that is followed by some pad bytes > and the u64 that carries the sector at which the data has been written > to the zone back to the driver, aka the append sector. > > This layout turned out to be problematic for implementing in QEMU and > the request status byte has been eventually made the last byte of the > in-header. The current code doesn't expect that and this causes the > append sector value always come as zero to the block layer. This needs > to be fixed ASAP. > > Fixes: 95bfec41bd3d ("virtio-blk: add support for zoned block devices") > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@xxxxxxx> A couple of nits below, but otherwise looks good to me. Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> [...] > @@ -242,11 +240,15 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev, > struct request *req, > struct virtblk_req *vbr) > { > - size_t in_hdr_len = sizeof(vbr->status); > + size_t in_hdr_len = sizeof(vbr->in_hdr.status); > bool unmap = false; > u32 type; > u64 sector = 0; > > + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) && > + op_is_zone_mgmt(req_op(req))) Weird indentation here. Make this a single line, or align op_is_zone_mgmt() call to "!". > + return BLK_STS_NOTSUPP; > + > /* Set fields for all request types */ > vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req)); > [...] > @@ -794,6 +871,7 @@ static inline bool virtblk_has_zoned_feature(struct virtio_device *vdev) > { > return virtio_has_feature(vdev, VIRTIO_BLK_F_ZONED); > } > + > #else > > /* > @@ -809,7 +887,6 @@ static inline int virtblk_probe_zoned_device(struct virtio_device *vdev, > { > return -EOPNOTSUPP; > } > - Keeping the whiteline between the function definitions would be nicer. > static inline bool virtblk_has_zoned_feature(struct virtio_device *vdev) > { > return false; -- Damien Le Moal Western Digital Research
