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

[James Bottomley <jbottomley@odin.com>]

On Thu, 2015-07-02 at 13:40 +0200, Michael S. Tsirkin wrote:
> On Thu, Jul 02, 2015 at 11:21:05AM +0000, James Bottomley wrote:
> > On Thu, 2015-07-02 at 10:51 +0200, Paolo Bonzini wrote:
> > > 
> > > On 02/07/2015 10:48, Michael S. Tsirkin wrote:
> > > >> > 
> > > >> > The above footnote is a consequence of the conformance statements in the
> > > >> > "Device Operation" section.  (Hence putting it in a footnote only).
> > > > Hmm I don't get it.
> > > > 
> > > > The only non-legacy text that mention writethrough is this:
> > > > 	VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between
> > > > 	writeback and writethrough modes.
> > > > 
> > > > so this is the only definition of these terms.
> > > 
> > > After the patch (quoting from v2):
> > > 
> > > +If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated and the
> > > +\field{writeback} field in configuration space is 0, the device MUST
> > > +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.
> > > 
> > > > Also, isn't the flush command special in the writeback mode?
> > > > maybe mention this...
> > > 
> > > Again quoting from v2:
> > > 
> > > +If the \field{VIRTIO_BLK_F_FLUSH} feature is negotiated, upon receipt
> > > +of a VIRTIO_BLK_T_FLUSH request, the device MUST ensure that any writes
> > > +which were completed are committed to non-volatile storage.
> > > 
> > > > 
> > > > Also
> > > > 	Some older legacy devices did not operate in writethrough mode
> > > > 	even after a driver announced lack of
> > > > 	support for VIRTIO_BLK_F_FLUSH.
> > > > 
> > > > What does this mean? let's just say in which mode they did operate.
> > > > Also what should driver do in this case?
> > > 
> > > In v2:
> > > 
> > > +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.  It MUST do so if it proposed
> > > +\field{VIRTIO_BLK_F_FLUSH}.  Failure to do so can cause data loss.
> > 
> > Not SHOULD, MUST.  Please don't leave spec based doors open to bad
> > implementations.  An implementation that allows data corruption MUST NOT
> > have a spec interpretation that supports it.
> > 
> > James
> > 
> > 
> 
> I'm not sure I agree.  Personally I create disks that discard data all
> the time. I *don't* want these writes committed anywhere.

There's a difference between you actively configuring a data destroying
device and a spec supporting a device implementation that can never be
operated with guaranteed data integrity.

If you want to damage integrity for speed, don't flush in the guest.

Perhaps I'm partly on this hobby horse because I'm getting demands over
in SCSI land that we clean up behind a range of USB devices with a
writeback cache and no flush command.  Personally I don't believe the
WCE feature is separable from the FLUSH one in a well run
implementation.

James