On Thu, Aug 13, 2015 at 12:13:57PM +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. > > 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> I think this looks sane. I don't want to start voting now, too many people are traveling. Could you prepare a patch for virtio scsi based on this? Thanks! > --- > v5->v6: > - clarify caching mode after reset > - improved and more consistent wording > - make descriptions less PCIish/MMIOish > > conformance.tex 2 + > content.tex 145 +++++++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 125 insertions(+), 22 deletions(-) > > diff --git a/conformance.tex b/conformance.tex > index 7b7df32..f59e360 100644 > --- a/conformance.tex > +++ b/conformance.tex > @@ -105,6 +105,7 @@ A network driver MUST conform to the following normative statements: > A block driver MUST conform to the following normative statements: > > egin{itemize} > +item
ef{drivernormative:Device Types / Block Device / Device Initialization} > item
ef{drivernormative:Device Types / Block Device / Device Operation} > end{itemize} > > @@ -224,6 +225,7 @@ A network device MUST conform to the following normative statements: > A block device MUST conform to the following normative statements: > > egin{itemize} > +item
ef{devicenormative:Device Types / Block Device / Device Initialization} > item
ef{devicenormative:Device Types / Block Device / Device Operation} > end{itemize} > > diff --git a/content.tex b/content.tex > index 3672588..9e72c19 100644 > --- a/content.tex > +++ b/content.tex > @@ -4041,8 +4041,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} > @@ -4051,16 +4056,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. > +egin{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} > > @@ -4089,7 +4090,7 @@ struct virtio_blk_config { > // optimal (suggested maximum) I/O size in blocks > le32 opt_io_size; > } topology; > - u8 reserved; > + u8 writeback; > }; > end{lstlisting} > > @@ -4119,21 +4120,63 @@ 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 cachefootnote{Consistent with > +
ef{devicenormative:Device Types / Block Device / Device Operation}, > + a writethrough cache can be defined broadly as a cache that commits > + writes to persistent device backend 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 can be either writeback or writethrough. The actual > + mode can be determined by reading field{writeback} after feature > + negotiation. > 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 neither VIRTIO_BLK_F_CONFIG_WCE nor VIRTIO_BLK_F_FLUSH are > +negotiated, the driver MAY deduce the presence of a writethrough cache. > +If VIRTIO_BLK_F_CONFIG_WCE was not negotiated but VIRTIO_BLK_F_FLUSH was, > +the driver SHOULD assume presence of a writeback cache. > + > +The driver MUST NOT read field{writeback} before setting > +the FEATURES_OK field{status} bit. > + > +devicenormative{subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization} > + > +Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it > +if they offer VIRTIO_BLK_F_CONFIG_WCE. > + > +If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH > +is not, the device MUST initialize field{writeback} to 0. > + > 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, transitional devices > +MUST implement slightly different behavior around feature negotiation > +when used through the legacy interface. In particular, when using the > +legacy interface: > + > +egin{itemize} > +item the driver MAY read or write field{writeback} before setting > + the DRIVER or DRIVER_OK field{status} bit > + > +item the device MUST NOT modify the cache mode (and field{writeback}) > + as a result of a driver setting a status bit, unless > + the DRIVER_OK bit is being set and the driver has not set the > + VIRTIO_BLK_F_CONFIG_WCE driver feature bit. > + > +item the device MUST NOT modify the cache mode (and field{writeback}) > + as a result of a driver modifying the driver feature bits, for example > + if the driver sets the VIRTIO_BLK_F_CONFIG_WCE driver feature bit but > + does not set the VIRTIO_BLK_F_FLUSH bit. > +end{itemize} > > -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} > > @@ -4183,14 +4226,66 @@ 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 persistent device backend 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 write are undefined in persistent device > +backend storage until the write becomes stable. A write becomes stable > +once it is completed and one or more of the following conditions is true: > + > +egin{enumerate} > +itemlabel{item:flush1} neither VIRTIO_BLK_F_CONFIG_WCE nor > + VIRTIO_BLK_F_FLUSH feature were negotiated, but VIRTIO_BLK_F_FLUSH was > + offered by the device; > + > +itemlabel{item:flush2} the VIRTIO_BLK_F_CONFIG_WCE feature was negotiated and the > + field{writeback} field in configuration space was 0 extbf{all the time between > + the submission of the write and its completion}; > + > +itemlabel{item:flush3} a VIRTIO_BLK_T_FLUSH request is sent extbf{after the write is > + completed} and is completed itself. > +end{enumerate} > + > +If the device is backed by persistent storage, the device MUST ensure that > +stable writes are committed to it, before reporting completion of the write > +(cases~
ef{item:flush1} and~
ef{item:flush2}) or the flush > +(case~
ef{item:flush3}). Failure to do so can cause data loss > +in case of a crash. > + > +If the driver changes field{writeback} between the submission of the write > +and its completion, the write could be either volatile or stable when > +its completion is reported; in other words, the exact behavior is undefined. > + > +% According to the device requirements for device initialization: > +% Offer(CONFIG_WCE) => Offer(FLUSH). > +% > +% After reversing the implication: > +% not Offer(FLUSH) => not Offer(CONFIG_WCE). > + > +If VIRTIO_BLK_F_FLUSH was not offered by the > + devicefootnote{Note that in this case, according to > +
ef{devicenormative:Device Types / Block Device / Device Initialization}, > + the device will not have offered VIRTIO_BLK_F_CONFIG_WCE either.}, the > +device MAY also commit writes to persistent device backend storage before > +reporting their completion. Unlike case~
ef{item:flush1}, however, this > +is not an absolute requirement of the specification. > + > +egin{note} > + An implementation that does not offer VIRTIO_BLK_F_FLUSH and does not commit > + completed writes will not be resilient to data loss in case of crashes. > + Not offering 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 > @@ -4233,6 +4328,12 @@ 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 persistent > +device backend storage when VIRTIO_BLK_F_FLUSH was offered but not > +negotiated. In order to work around this, the driver MAY set the > +field{writeback} to 0 (if available) or it MAY send an explicit flush > +request after every completed write. > + > If the device has VIRTIO_BLK_F_SCSI feature, it can also support > scsi packet command requests, each of these requests is of form: > > @@ -5577,7 +5678,7 @@ contents of field{event}. The following events are defined: > end{lstlisting} > > By sending this event, the device signals a change in the configuration parameters > - of a logical unit, for example the capacity or caching mode. > + of a logical unit, for example the capacity or cache mode. > field{event} is set to VIRTIO_SCSI_T_PARAM_CHANGE. > field{lun} addresses a logical unit in the SCSI host. > > -- > 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