Re: [virtio] [PATCH] virtio-blk: restore VIRTIO_BLK_F_CONFIG_WCE (VIRTIO-144)

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 02, 2015 at 12:41:04AM +0200, Paolo Bonzini wrote:
> VIRTIO_BLK_F_CONFIG_WCE is important in order to achieve good performance
> (up to 2x, though more realistically +30-40%) in latency-bound workloads.
> However, it was removed by mistake together with VIRTIO_BLK_F_FLUSH.
> 
> Restore it, clarifying the definition of writeback and writethrough cache
> according to what is commonly done by high-end storage.  In addition, fix
> two occurrences where the driver was mentioned rather than the device, and
> clarify the influence of the legacy VIRTIO_BLK_F_FLUSH feature on caching.
> 
> There is a difference between the legacy and virtio1 behavior of
> VIRTIO_BLK_F_CONFIG_WCE.  In legacy devices, the writeback field was
> only available if VIRTIO_BLK_F_CONFIG_WCE was negotiated, while now
> it is always available.  However, the VIRTIO_BLK_F_CONFIG_WCE feature
> remains and describes whether the field is writable.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Please mention the issue ID in the commit log in the future.
E.g. on a line by itself:

VIRTIO-144

> ---
>  content.tex | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 1efdcc8..7c00fd7 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3725,6 +3725,9 @@ device except where noted.
>  
>  \item[VIRTIO_BLK_F_TOPOLOGY (10)] Device exports information on optimal I/O
>      alignment.
> +
> +\item[VIRTIO_BLK_F_CONFIG_WCE (11)] Device can toggle its cache between writeback
> +    and writethrough modes.
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Block Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3735,9 +3738,6 @@ device except where noted.
>  \item[VIRTIO_BLK_F_SCSI (7)] Device supports scsi packet commands.
>  
>  \item[VIRTIO_BLK_F_FLUSH (9)] Cache flush command support.
> -
> -\item[VIRTIO_BLK_F_CONFIG_WCE (11)] Device can toggle its cache between writeback
> -    and writethrough modes.
>  \end{description}
>  
>  VIRTIO_BLK_F_FLUSH was also called VIRTIO_BLK_F_WCE: Legacy drivers
> @@ -3746,9 +3746,9 @@ VIRTIO_BLK_T_FLUSH commands.
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Block Device / Device configuration layout}
>  
> -The \field{capacity} of the device (expressed in 512-byte sectors) is always
> -present. The availability of the others all depend on various feature
> -bits as indicated above.
> +The \field{capacity} of the device (expressed in 512-byte sectors) and
> +caching mode (\field{writeback}) are always present. The availability
> +of the others all depend on various feature bits as indicated above.
>  
>  \begin{lstlisting}
>  struct virtio_blk_config {

I'm afraid we can't make this change at this point - it's clearly
incompatible with devices that implement cs01 unless they
also implement the WCE bit.


If you see value in having a RO writeback field,
just add a feature bit for that.


> @@ -3771,7 +3771,7 @@ struct virtio_blk_config {
>                  // optimal (suggested maximum) I/O size in blocks
>                  le32 opt_io_size;
>          } topology;
> -        u8 reserved;
> +        u8 writeback;
>  };
>  \end{lstlisting}
>  
> @@ -3801,15 +3801,20 @@ according to the native endian of the guest rather than
>    \field{topology} struct can be read to determine the physical block size and optimal
>    I/O lengths for the driver to use. This also does not affect the units
>    in the protocol, only performance.
> +
> +\item The cache mode can be read from \field{writeback}.  0 corresponds
> +  to a writethrough cache, 1 to a writeback cache\footnote{Consistent
> +    with \ref{sec:Device Types / Block Device / Device Operation},
> +    a writethrough cache can be defined broadly as a cache that commits
> +    writes to non-volatile storage before reporting their completion.
> +    For example, a battery-backed writeback cache actually counts as
> +    writethrough according to this definition.}.
>  \end{enumerate}


Could you add some conformance statements for this as well
please?

>  
>  \subsubsection{Legacy Interface: Device Initialization}\label{sec:Device Types / Block Device / Device Initialization / Legacy Interface: Device Initialization}
>  
> -The \field{reserved} field used to be called \field{writeback}.  If the
> -VIRTIO_BLK_F_CONFIG_WCE feature is offered, the cache mode can be
> -read from \field{writeback}; the
> -driver can also write to the field in order to toggle the cache
> -between writethrough (0) and writeback (1) mode.  If the feature is
> +For legacy devices, the \field{writeback} field is only available if
> +the VIRTIO_BLK_F_CONFIG_WCE feature is offered.  If the feature is
>  not available, the driver can instead look at the result of
>  negotiating VIRTIO_BLK_F_FLUSH: the cache will be in writeback mode
>  after reset if and only if VIRTIO_BLK_F_FLUSH is negotiated.
> @@ -3871,9 +3876,19 @@ A device MUST set the \field{status} byte to VIRTIO_BLK_S_IOERR
>  for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT
>  write any data.
>  
> -Upon receipt of a VIRTIO_BLK_T_FLUSH request, the driver SHOULD ensure
> +Upon receipt of a VIRTIO_BLK_T_FLUSH request, the device SHOULD ensure
>  that any writes which were completed are committed to non-volatile storage.
>  
> +If the \field{writeback} field in configuration space is 0, the device
> +SHOULD ensure that all writes are committed to non-volatile storage before
> +reporting completion.
> +
> +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},
> +the driver MUST NOT assume that writes submitted while the field was 1 have
> +been committed to non-volatile storage.
> +
>  \subsubsection{Legacy Interface: Device Operation}\label{sec:Device Types / Block Device / Device Operation / Legacy Interface: Device Operation}
>  When using the legacy interface, transitional devices and drivers
>  MUST format the fields in struct virtio_blk_req
> @@ -3884,6 +3899,10 @@ The \field{reserved} field was previously called \field{ioprio}.  \field{ioprio}
>  is a hint about the relative priorities of requests to the device:
>  higher numbers indicate more important requests.
>  
> +If the \field{VIRTIO_BLK_F_FLUSH} feature is not negotiated, the device
> +SHOULD ensure that all writes are committed to non-volatile storage before
> +reporting completion.
> +

Maybe clarify what happens if it doesn't "Failure to do so might
lead to data loss"?

>  \begin{lstlisting}
>  #define VIRTIO_BLK_T_FLUSH_OUT    5
>  \end{lstlisting}
> -- 
> 2.4.3
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that 
> generates this mail.  Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php