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 <sgarz...@redhat.com> > > > > > > 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 <m...@redhat.com> > > > > 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
