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 11:46:48AM +0000, James Bottomley wrote:
> 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.

Users want to configure this on the host, not on 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

Well this is not the case here.  We do say "Failure to do so can cause
data loss." Maybe "MUST unless data loss in case of a crash is
acceptable"?


-- 
MST