[virtio-dev] [PATCH v3 0/5] virtio-blk: discard and write zeroes clarifications

[virtio-dev] [PATCH v3 0/5] virtio-blk: discard and write zeroes clarifications

[Stefan Hajnoczi]

v3:
 * Added Patch 5 to explicitly document multi-segment requests
   (including limits and failure semantics)

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 (5):
  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
  virtio-blk: clarify semantics of multi-segment discard/write zeroes
    commands

 content.tex | 68 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 16 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 v3 1/5] 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] [PATCH v3 2/5] 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 v3 3/5] 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 v3 4/5] 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

[virtio-dev] [PATCH v3 5/5] virtio-blk: clarify semantics of multi-segment discard/write zeroes commands

[Stefan Hajnoczi]

Describe the failure case and maximum number of segments in a
multi-segment discard/write zeroes command.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 content.tex | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/content.tex b/content.tex
index c5fdc34..7bfcc78 100644
--- a/content.tex
+++ b/content.tex
@@ -3967,8 +3967,10 @@ 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:
+The \field{data} used for discard or write zeroes commands consists of one or
+more segments.  The maximum number of segments is \field{max_discard_seg} for
+discard commands and \field{max_write_zeroes_seg} for write zeroes commands.
+Each segment is of form:
 
 \begin{lstlisting}
 struct virtio_blk_discard_write_zeroes {
@@ -3997,6 +3999,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device:
 #define VIRTIO_BLK_S_UNSUPP    2
 \end{lstlisting}
 
+The status of individual segments is indeterminate when a discard or write zero
+command produces VIRTIO_BLK_S_IOERR.  A segment may have completed
+successfully, failed, or not been processed by the device.
+
 \drivernormative{\subsubsection}{Device Operation}{Device Types / Block Device / Device Operation}
 
 A driver MUST NOT submit a request which would cause a read or write
@@ -4014,6 +4020,14 @@ 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.
 
+VIRTIO_BLK_T_DISCARD requests MUST NOT contain more than
+\field{max_discard_seg} struct virtio_blk_discard_write_zeroes segments in
+\field{data}.
+
+VIRTIO_BLK_T_WRITE_ZEROES requests MUST NOT contain more than
+\field{max_write_zeroes_seg} struct virtio_blk_discard_write_zeroes segments in
+\field{data}.
+
 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},
@@ -4024,6 +4038,11 @@ The \field{unmap} bit MUST be zero for discard commands.  The driver
 MUST NOT assume anything about the data returned by read requests after
 a range of sectors has been discarded.
 
+A driver MUST NOT assume that individual segments in a multi-segment
+VIRTIO_BLK_T_DISCARD or VIRTIO_BLK_T_WRITE_ZEROES request completed
+successfully, failed, or were processed by the device at all if the request
+failed with VIRTIO_BLK_S_IOERR.
+
 \devicenormative{\subsubsection}{Device Operation}{Device Types / Block Device / Device Operation}
 
 A device MUST set the \field{status} byte to VIRTIO_BLK_S_IOERR
-- 
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] [PATCH v3 5/5] virtio-blk: clarify semantics of multi-segment discard/write zeroes commands

[Michael S. Tsirkin]

On Thu, Feb 21, 2019 at 04:22:19PM +0000, Stefan Hajnoczi wrote:
> Describe the failure case and maximum number of segments in a
> multi-segment discard/write zeroes command.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

OK so just making sure.  Patches 1-4 have already been approved, so I
should apply them. Patch 5 needs another vote. Is that right?


> ---
>  content.tex | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index c5fdc34..7bfcc78 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3967,8 +3967,10 @@ 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:
> +The \field{data} used for discard or write zeroes commands consists of one or
> +more segments.  The maximum number of segments is \field{max_discard_seg} for
> +discard commands and \field{max_write_zeroes_seg} for write zeroes commands.
> +Each segment is of form:
>  
>  \begin{lstlisting}
>  struct virtio_blk_discard_write_zeroes {
> @@ -3997,6 +3999,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device:
>  #define VIRTIO_BLK_S_UNSUPP    2
>  \end{lstlisting}
>  
> +The status of individual segments is indeterminate when a discard or write zero
> +command produces VIRTIO_BLK_S_IOERR.  A segment may have completed
> +successfully, failed, or not been processed by the device.
> +
>  \drivernormative{\subsubsection}{Device Operation}{Device Types / Block Device / Device Operation}
>  
>  A driver MUST NOT submit a request which would cause a read or write
> @@ -4014,6 +4020,14 @@ 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.
>  
> +VIRTIO_BLK_T_DISCARD requests MUST NOT contain more than
> +\field{max_discard_seg} struct virtio_blk_discard_write_zeroes segments in
> +\field{data}.
> +
> +VIRTIO_BLK_T_WRITE_ZEROES requests MUST NOT contain more than
> +\field{max_write_zeroes_seg} struct virtio_blk_discard_write_zeroes segments in
> +\field{data}.
> +
>  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},
> @@ -4024,6 +4038,11 @@ The \field{unmap} bit MUST be zero for discard commands.  The driver
>  MUST NOT assume anything about the data returned by read requests after
>  a range of sectors has been discarded.
>  
> +A driver MUST NOT assume that individual segments in a multi-segment
> +VIRTIO_BLK_T_DISCARD or VIRTIO_BLK_T_WRITE_ZEROES request completed
> +successfully, failed, or were processed by the device at all if the request
> +failed with VIRTIO_BLK_S_IOERR.
> +
>  \devicenormative{\subsubsection}{Device Operation}{Device Types / Block Device / Device Operation}
>  
>  A device MUST set the \field{status} byte to VIRTIO_BLK_S_IOERR
> -- 
> 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

---------------------------------------------------------------------
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] [PATCH v3 5/5] virtio-blk: clarify semantics of multi-segment discard/write zeroes commands

[Stefan Hajnoczi]

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

On Fri, Feb 22, 2019 at 10:01:13AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 21, 2019 at 04:22:19PM +0000, Stefan Hajnoczi wrote:
> > Describe the failure case and maximum number of segments in a
> > multi-segment discard/write zeroes command.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> OK so just making sure.  Patches 1-4 have already been approved, so I
> should apply them. Patch 5 needs another vote. Is that right?

Yes, sorry for combining it all into a single series.

Stefan

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