Re: [virtio-dev] Re: [PATCH v2 1/4] virtio-blk: document data[] size constraints

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Feb 18, 2019 at 08:22:00AM +0100, Jan Kiszka wrote:
> On 31.01.19 05:14, Michael S. Tsirkin wrote:
> > On Thu, Jan 31, 2019 at 10:36:14AM +0800, Stefan Hajnoczi wrote:
> > > The struct virtio_blk_req->data[] field is a multiple of 512 bytes long
> > > for read and write requests.  Flush requests don't use data[] at all.
> > > 
> > > The new discard and write zeroes requests being introduced in VIRTIO 1.1
> > > put struct virtio_blk_discard_write_zeroes elements into data[], so it
> > > must be a multiple of the struct size.
> > > 
> > > The uint8_t data[][512] pseudo-code makes it look like discard and write
> > > zeroes requests must pad to 512 bytes.  This wastes memory since struct
> > > virtio_blk_discard_write_data is only 16 bytes long.
> > > 
> > > Furthermore, all known implementations wishing to take advantage of this
> > > upcoming VIRTIO 1.1 feature do not use 512-byte padding (Linux
> > > virtio_blk.ko, QEMU virtio-blk device emulation, the SPDK virtio-blk
> > > driver, and the SPDK vhost-user-blk device backend).
> > > 
> > > This patch documents the data[] size constraints clearly in the driver
> > > normative section.  This is clearer than the current pseudo-code.
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Changpeng Liu <changpeng.liu@intel.com>
> > > Cc: Stefano Garzarella <sgarzare@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >   content.tex | 14 +++++++++++++-
> > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 836ee52..b185bb0 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -3941,7 +3941,7 @@ struct virtio_blk_req {
> > >           le32 type;
> > >           le32 reserved;
> > >           le64 sector;
> > > -        u8 data[][512];
> > > +        u8 data[];
> > >           u8 status;
> > >   };
> > > @@ -3971,6 +3971,11 @@ The \field{sector} number indicates the offset (multiplied by 512) where
> > >   the read or write is to occur. This field is unused and set to 0 for
> > >   commands other than read or write.
> > > +VIRTIO_BLK_T_IN requests populate \field{data} with the contents of sectors
> > > +read from the block device (in multiples of 512 bytes).  VIRTIO_BLK_T_OUT
> > > +requests write the contents of \field{data} to the block device (in multiples
> > > +of 512 bytes).
> > > +
> > >   The \field{data} used for discard or write zeroes command is described
> > >   by one or more virtio_blk_discard_write_zeroes structs. \field{sector}
> > >   indicates the starting offset (in 512-byte units) of the segment, while
> > > @@ -3997,6 +4002,13 @@ A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered.
> > >   A driver MUST set \field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request.
> > >   A driver SHOULD NOT include any data in a VIRTIO_BLK_T_FLUSH request.
> > > +The length of \field{data} MUST be a multiple of 512 bytes for VIRTIO_BLK_T_IN
> > > +and VIRTIO_BLK_T_OUT requests.
> > > +
> > > +The length of \field{data} MUST be a multiple of the size of struct
> > > +virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD and
> > > +VIRTIO_BLK_T_WRITE_ZEROES requests.
> > > +
> > 

I'm not the original spec author.  Feel free to correct me if this is
wrong:

> > So a single request can discard/write multiple ranges?
> > It might be a good idea to make this explicit.
> > Also is this capability useful/used?

The multiple segments feature was included because the underlying
storage might support it.  Currently we don't expect much use but future
hardware may rely on it more heavily (e.g. for performance).

> > And what's the value of status
> > in case some of the requests fail?

A failure for any segment causes the entire request to fail with no
information about which segments completed or failed.

> What happened with this comment? I don't see a follow-up nor a resolution
> elsewhere, just the opening of issue #32 for voting. Please clarify.

With #32 applied the spec says:

"max_discard_seg can be read to determine the [...] maximum number of discard segments for the block driver to use"

and

"The length of \field{data} MUST be a multiple of the size of struct
virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD and
VIRTIO_BLK_T_WRITE_ZEROES requests."

This is not very explicit but it means multiple struct
virtio_blk_discard_write_zeroes can be included in a request, up to
max_discard_seg.

I think two things are appropriate:
1. A driver normative statement saying up to
   max_discard_seg/max_write_zeroes_seg structs may be included in a
   request
2. A general description that says DISCARD/WRITE_ZEROES requests may
   have more than 1 "segment" (struct virtio_blk_discard_write_zeroes)

Does that sound good?

Stefan

Attachment: signature.asc
Description: PGP signature