Re: [PATCH 1/2] virtio-blk: migrate to the latest patchset version
From: Damien Le Moal <hidden>
Date: 2023-03-29 08:18:33
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@wdc.com/ (local) , but the latest and reviewed version is https://lore.kernel.org/linux-block/20221110053952.3378990-3-dmitry.fomichev@wdc.com/ (local) 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@vger.kernel.org Signed-off-by: Dmitry Fomichev <redacted>
A couple of nits below, but otherwise looks good to me. Reviewed-by: Damien Le Moal <redacted> [...]
quoted hunk ↗ jump to hunk
@@ -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));
[...]
quoted hunk ↗ jump to hunk
@@ -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