[virtio-dev] [PATCH v2 0/4] virtio-blk: discard and write zeroes clarifications

[virtio-dev] [PATCH v2 0/4] virtio-blk: discard and write zeroes clarifications

[Stefan Hajnoczi]

Several clarifications have been requested for the VIRTIO 1.1 virtio-blk
discard and write zeroes commands.  This series takes care of them.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/32

Stefan Hajnoczi (4):
  virtio-blk: document data[] size constraints
  virtio-blk: move virtio_blk_discard_write_zeroes definition
  virtio-blk: describe write zeroes unmap semantics
  virtio-blk: avoid inconsistent "DISCARD" term

 content.tex | 47 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

-- 
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Stefan Hajnoczi]

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.
+
 If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the driver MAY
 switch to writethrough or writeback mode by writing respectively 0 and
 1 to the \field{writeback} field.  After writing a 0 to \field{writeback},
-- 
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

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

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? And what's the value of status
in case some of the requests fail?


>  If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the driver MAY
>  switch to writethrough or writeback mode by writing respectively 0 and
>  1 to the \field{writeback} field.  After writing a 0 to \field{writeback},
> -- 
> 2.20.1

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Jan Kiszka]

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.
>> +
> 
> 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? And what's the value of status
> in case some of the requests fail?
> 
> 

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.

Jan

>>   If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the driver MAY
>>   switch to writethrough or writeback mode by writing respectively 0 and
>>   1 to the \field{writeback} field.  After writing a 0 to \field{writeback},
>> -- 
>> 2.20.1
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4898 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Michael S. Tsirkin]

On Mon, Feb 18, 2019 at 02:04:20PM +0000, Stefan Hajnoczi wrote:
> 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.

And what's the status?

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

Also pls include explanation about failure mode.

-- 
MST

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Mon, Feb 18, 2019 at 02:04:20PM +0000, Stefan Hajnoczi wrote:
> 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

Jan could you confirm please? Let's create an issue and close this.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5648 bytes --]

On Fri, Feb 22, 2019 at 12:53:42AM -0500, Michael S. Tsirkin wrote:
> On Mon, Feb 18, 2019 at 02:04:20PM +0000, Stefan Hajnoczi wrote:
> > 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
> 
> Jan could you confirm please? Let's create an issue and close this.

I have sent a new revision of the patches with the clarifications we've
discussed here.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[virtio-dev] [PATCH v2 2/4] virtio-blk: move virtio_blk_discard_write_zeroes definition

[Stefan Hajnoczi]

struct virtio_blk_discard_write_zeroes is defined alongside
struct virtio_blk_req but only discussed later in the text.  Move it to
where it belongs.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 content.tex | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/content.tex b/content.tex
index b185bb0..4201c7e 100644
--- a/content.tex
+++ b/content.tex
@@ -3944,15 +3944,6 @@ struct virtio_blk_req {
         u8 data[];
         u8 status;
 };
-
-struct virtio_blk_discard_write_zeroes {
-       le64 sector;
-       le32 num_sectors;
-       struct {
-               le32 unmap:1;
-               le32 reserved:31;
-       } flags;
-};
 \end{lstlisting}
 
 The type of the request is either a read (VIRTIO_BLK_T_IN), a write
@@ -3977,10 +3968,22 @@ 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
-\field{num_sectors} indicates the number of sectors in each discarded
-range. \field{unmap} is only used for write zeroes command.
+by one or more virtio_blk_discard_write_zeroes structs:
+
+\begin{lstlisting}
+struct virtio_blk_discard_write_zeroes {
+       le64 sector;
+       le32 num_sectors;
+       struct {
+               le32 unmap:1;
+               le32 reserved:31;
+       } flags;
+};
+\end{lstlisting}
+
+\field{sector} indicates the starting offset (in 512-byte units) of the
+segment, while \field{num_sectors} indicates the number of sectors in each
+discarded range. \field{unmap} is only used for write zeroes command.
 
 The final \field{status} byte is written by the device: either
 VIRTIO_BLK_S_OK for success, VIRTIO_BLK_S_IOERR for device or driver
-- 
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v2 3/4] virtio-blk: describe write zeroes unmap semantics

[Stefan Hajnoczi]

Explain the meaning of the unmap flag.  The details are already covered
in the device normative section but mentioning it here makes the text
easier to understand.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 content.tex | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 4201c7e..8ea8320 100644
--- a/content.tex
+++ b/content.tex
@@ -3983,7 +3983,9 @@ struct virtio_blk_discard_write_zeroes {
 
 \field{sector} indicates the starting offset (in 512-byte units) of the
 segment, while \field{num_sectors} indicates the number of sectors in each
-discarded range. \field{unmap} is only used for write zeroes command.
+discarded range. \field{unmap} is only used in write zeroes commands and allows
+the device to discard the specified range, provided that following reads return
+zeroes.
 
 The final \field{status} byte is written by the device: either
 VIRTIO_BLK_S_OK for success, VIRTIO_BLK_S_IOERR for device or driver
-- 
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v2 4/4] virtio-blk: avoid inconsistent "DISCARD" term

[Stefan Hajnoczi]

"discard" (lowercase) is used throughout the text.  Remove a lone
instance of "DISCARD" (uppercase).

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 content.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 8ea8320..c5fdc34 100644
--- a/content.tex
+++ b/content.tex
@@ -4040,7 +4040,7 @@ sectors in the device backend storage.
 
 For write zeroes commands, if the \field{unmap} is set, the device MAY
 deallocate the specified range of sectors in the device backend storage,
-as if the DISCARD command had been sent.  After a write zeroes command
+as if the discard command had been sent.  After a write zeroes command
 is completed, reads of the specified ranges of sectors MUST return
 zeroes.  This is true independent of whether \field{unmap} was set or clear.
 
-- 
2.20.1


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org