Re: [PATCH 1/2] virtio-blk: migrate to the latest patchset version

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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/
> 
> , but the latest and reviewed version is
> 
> https://lore.kernel.org/linux-block/20221110053952.3378990-3-dmitry.fomichev@wdc.com/
> 
> 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 <dmitry.fomichev@wdc.com>

A couple of nits below, but otherwise looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

[...]

> @@ -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