[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] [PATCH] virtio-blk: restore VIRTIO_BLK_F_FLUSH and VIRTIO_BLK_F_CONFIG_WCE
This is v3. Paolo On 02/07/2015 16:20, 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. > > In addition, even removing VIRTIO_BLK_F_FLUSH was probably not a great > idea, because it simplifies simple drivers (e.g. firmware) that are okay > with a writethrough cache but still need data to persist after power loss. > What really should have been removed is just the possibility that devices > not propose VIRTIO_BLK_F_FLUSH, but even that only deserves a "SHOULD" in > the new world of conformance statements. > > Restore these, with the following changes: > > * clarify and use conformance statements in order to define writeback > and writethrough caching according to what is commonly done by high-end > storage. > > * clarify (with conformance statements) the influence of the > VIRTIO_BLK_F_FLUSH feature on caching and how to proceed if only one of > VIRTIO_BLK_F_FLUSH and VIRTIO_BLK_F_CONFIG_WCE is negotiated. > > * strengthen the requirement for persisting writes to MUST after > a VIRTIO_BLK_T_FLUSH request (and in other cases too involving the > new features). > > The suggested behavior upon feature negotiation is okay for the Linux > implementation of virtio1, even after the implementation is modified to > support the two new features. > > This fixes VIRTIO-144. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > conformance.tex | 2 + > content.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 111 insertions(+), 21 deletions(-) > > diff --git a/conformance.tex b/conformance.tex > index 29c6ba8..8753cd6 100644 > --- a/conformance.tex > +++ b/conformance.tex > @@ -102,6 +102,7 @@ A network driver MUST conform to the following normative statements: > A block driver MUST conform to the following normative statements: > > \begin{itemize} > +\item \ref{drivernormative:Device Types / Block Device / Device Initialization} > \item \ref{drivernormative:Device Types / Block Device / Device Operation} > \end{itemize} > > @@ -205,6 +206,7 @@ A network device MUST conform to the following normative statements: > A block device MUST conform to the following normative statements: > > \begin{itemize} > +\item \ref{devicenormative:Device Types / Block Device / Device Initialization} > \item \ref{devicenormative:Device Types / Block Device / Device Operation} > \end{itemize} > > diff --git a/content.tex b/content.tex > index 3b12263..e58a0bd 100644 > --- a/content.tex > +++ b/content.tex > @@ -3723,8 +3723,13 @@ device except where noted. > > \item[VIRTIO_BLK_F_BLK_SIZE (6)] Block size of disk is in \field{blk_size}. > > +\item[VIRTIO_BLK_F_FLUSH (9)] Cache flush command support. > + > \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} > @@ -3733,16 +3738,12 @@ device except where noted. > \item[VIRTIO_BLK_F_BARRIER (0)] Device supports request barriers. > > \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 > -MUST only negotiate this feature if they are capable of sending > -VIRTIO_BLK_T_FLUSH commands. > +\begin{note} > + In the legacy interface, VIRTIO_BLK_F_FLUSH was also > + called VIRTIO_BLK_F_WCE. > +\end{note} > > \subsection{Device configuration layout}\label{sec:Device Types / Block Device / Device configuration layout} > > @@ -3771,7 +3772,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,21 +3802,52 @@ 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 If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache > + mode can be read or set through the \field{writeback} field. 0 corresponds > + to a writethrough cache, 1 to a writeback cache\footnote{Consistent with > + \ref{devicenormative: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.}. The cache mode after > + reset is undefined. > \end{enumerate} > > +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization} > + > +Drivers SHOULD NOT negotiate VIRTIO_BLK_F_FLUSH if they are incapable of > +sending VIRTIO_BLK_T_FLUSH commands. > + > +If the VIRTIO_BLK_F_CONFIG_WCE feature is not negotiated, but > +VIRTIO_BLK_F_FLUSH was proposed by the device, the driver MAY deduce > +the presence of a writethrough cache. Otherwise, the driver SHOULD > +assume presence of a writeback cache. > + > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization} > + > +Devices SHOULD always propose VIRTIO_BLK_F_FLUSH, and MUST propose it > +if they propose VIRTIO_BLK_F_CONFIG_WCE. > + > +If VIRTIO_BLK_F_CONFIG_WCE is negotiated but the VIRTIO_BLK_F_FLUSH feature > +is not, the device MUST set \field{writeback} to 0 as soon as the driver > +sets the FEATURES_OK status bit. > + > \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 > -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. > +Because legacy devices do not have FEATURES_OK, legacy device behavior > +differs around feature negotiation; legacy devices never modify > +\field{writeback} as a result of a driver's write to the status register. > +In particular, some legacy drivers wrote to \field{writeback} between > +setting the DRIVER status bit and setting the DRIVER_OK status bit. > +It would be a bug for the device to overwrite \field{writeback} when > +the DRIVER_OK status bit is set. > + > +Legacy devices must also never modify \field{writeback} as a result of > +a driver's write to the guest features registers, for example if the > +driver sets the VIRTIO_BLK_F_CONFIG_WCE guest feature bit but does not > +set the VIRTIO_BLK_F_FLUSH bit. > > -Some older legacy devices did not operate in writethrough mode even > -after a driver announced lack of support for VIRTIO_BLK_F_FLUSH. > > \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Operation} > > @@ -3865,14 +3897,67 @@ 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. > > +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 any volatile writes have been committed > +to non-volatile storage. > + > \devicenormative{\subsubsection}{Device Operation}{Device Types / Block Device / Device Operation} > > 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 device SHOULD ensure > -that any writes which were completed are committed to non-volatile storage. > +A write is considered volatile when it is submitted; the contents of > +sectors covered by a volatile are undefined until the write becomes stable. > +A write becomes stable once it is completed and one or more of the > +following conditions is true: > + > +\begin{enumerate} > +\item\label{item:flush1} neither VIRTIO_BLK_F_CONFIG_WCE nor > + VIRTIO_BLK_F_FLUSH feature were negotiated, but VIRTIO_BLK_F_FLUSH was > + proposed by the device; > + > +\item\label{item:flush2} the VIRTIO_BLK_F_CONFIG_WCE feature was negotiated and the > + \field{writeback} field in configuration space was 0 \textbf{all the time between > + the submission of the write and its completion}; > + > +\item\label{item:flush3} a VIRTIO_BLK_T_FLUSH request is sent \textbf{after the write is > + completed} and is completed itself. > +\end{enumerate} > + > +The device MUST ensure that stable writes are committed to > +non-volatile storage, before reporting completion of the write > +(cases~\ref{item:flush1} and~\ref{item:flush2}) or the flush > +(case~\ref{item:flush3}). Failure to do so can cause data loss > +in case of a crash. > + > +% ** Not using MAY/MAY NOT intentionally, this is not optional behavior ** > +If the driver changes \field{writeback} between the submission of the write > +and its completion, the write can be either volatile or stable when > +its completion is reported; the exact behavior is undefined. > + > +% According to the device requirements for device initialization: > +% Propose(CONFIG_WCE) => Propose(FLUSH). > +% > +% After reversing the implication: > +% not Propose(FLUSH) => not Propose(CONFIG_WCE). > + > +If VIRTIO_BLK_F_FLUSH was not proposed by the > + device\footnote{Note that in this case, according to > + \ref{devicenormative:Device Types / Block Device / Device Initialization}, > + the device will not have proposed VIRTIO_BLK_F_CONFIG_WCE either.}, the > +device MAY also commit writes to non-volatile storage before reporting > +their completion. Unlike case~\ref{item:flush1}, however, this is not > +an absolute requirement of the specification. > + > +\begin{note} > + An implementation that does not propose VIRTIO_BLK_F_FLUSH and does not commit > + completed writes will not be resilient to data loss in case of crashes. > + Not proposing VIRTIO_BLK_F_FLUSH is an absolute requirement > + for implementations that do not wish to be safe against such data losses. > +\end{note} > > \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 > @@ -3907,6 +3992,9 @@ serve as data consistency guarantee. Only a VIRTIO_BLK_T_FLUSH request > does that. > \end{note} > > +Some older legacy devices did not commit completed writes to non-volatile > +storage, even if VIRTIO_BLK_F_FLUSH was proposed but not negotiated. > + > If the device has VIRTIO_BLK_F_SCSI feature, it can also support > scsi packet command requests, each of these requests is of form: > >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]