Re: [Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES features

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 06:01:34PM +0100, Stefano Garzarella wrote:
> 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.


Oh I missed the fact that you set the config space values to 1.
I confused it with # of sectors.


Now it looks right to me, so pls ignore my comments.

Reviewed-by: Michael S. Tsirkin <address@hidden>



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