[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES
From: |
Stefano Garzarella |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES features |
Date: |
Thu, 31 Jan 2019 18:01:34 +0100 |
User-agent: |
NeoMutt/20180716 |
On Thu, Jan 31, 2019 at 11:04:10AM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 31, 2019 at 04:19:12PM +0100, Stefano Garzarella wrote:
> > This patch adds the support of DISCARD and WRITE ZEROES commands,
> > that have been introduced in the virtio-blk protocol to have
> > better performance when using SSD backend.
> >
> > We support only one segment per request since multiple segments
> > are not widely used and there are no userspace APIs that allow
> > applications to submit multiple segments in a single call.
> >
> > Signed-off-by: Stefano Garzarella <address@hidden>
>
>
> This does not seem to match the spec clarifications
> that Stefan Hajnoczi has posted on
> the virtio TC.
>
> I think this can go any of the following ways:
>
> - we agree that a request should have at most one segment
> no padding allowed
> - we agree that a request should have at most one segment
> we require padding to 512 bytes
> - we agree that a request should have at most one segment
> we also support padding to 512 bytes
> - we agree that a request should have at most one segment
> we also support arbitrary padding
> - we agree that a request can have any # of segments
Hi Michael,
reading the latest patch [1] sent by Stefan, I supposed that the padding
is not allowed, but if we need to support it, I'll fix the implementation.
About the number of segments, I followed the description of
max_discard_sectors [2] and the implementation provided by SPDK's
virtio-blk driver and vhost-user-blk device backend. They also only
support one segment per request, properly setting "max_discard_seg" and
"max_write_zeroes_seg", so if I understood correctly, the specification
leave to the device the freedom to support one or more segments.
>
> I would also need your feedback on whether all this
> is a material change to 1.1 public review according to the oasis definition:
>
> "Material Change" is any change to the content of a Work Product that
> that would require a compliant application or implementation to be
> modified or rewritten in order to remain compliant or which adds new
> features or otherwise expands the scope of the work product.
>
IMHO and if I understood correctly, maybe only the second way (require
padding to 512 bytes) should be a "Material Change" because we need to
modify all the implementations (Linux driver, SPDK, vhost).
The other ways should be already supported, because all the
implementations set the status right behind the last byte (regardless of
padding).
#1: https://lists.oasis-open.org/archives/virtio-dev/201901/msg00135.html
#2: https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L3876
Thanks,
Stefano
[Qemu-devel] [PATCH v2 1/5] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error(), Stefano Garzarella, 2019/01/31
[Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES features, Stefano Garzarella, 2019/01/31
Re: [Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES features, Stefan Hajnoczi, 2019/01/31
[Qemu-devel] [PATCH v2 4/5] tests/virtio-blk: change assert on data_size in virtio_blk_request(), Stefano Garzarella, 2019/01/31
[Qemu-devel] [PATCH v2 5/5] tests/virtio-blk: add test for WRITE_ZEROES command, Stefano Garzarella, 2019/01/31
Re: [Qemu-devel] [PATCH v2 0/5] virtio-blk: add DISCARD and WRITE ZEROES features, Michael S. Tsirkin, 2019/01/31