Re: [virtio] [PATCH v2] virtio-blk: restore VIRTIO_BLK_F_FLUSH and VIRTIO_BLK_F_CONFIG_WCE

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

On Thu, Jul 02, 2015 at 03:01:55PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 12:51, Michael S. Tsirkin wrote:
> >>>> +\item If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, 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.}.
> >>>
> >>> This seems a bit too much text for a footnote, I'd still prefer it
> >>> as a simple text note.
> >>
> >> I can't do this inside an {enumerate}.  It is long-ish, but it's just
> >> documenting what existing hardware does.
> > 
> > Why not? I just tried this: note nests fine within enumerate.
> 
> I get an error from LaTeX.

Do you mean XeTex? Also - how did you write it?


> >>>> +
> >>>> +\item If the VIRTIO_BLK_F_CONFIG_WCE feature is not available, the driver
> >>>> +  can deduct the cache mode by looking at the result of negotiating
> >>>> +  VIRTIO_BLK_F_FLUSH: the cache will be in writeback mode if and only if
> >>>> +  VIRTIO_BLK_F_FLUSH is negotiated.
> >>>> +\end{enumerate}
> >>>>  
> >>>> -Some older legacy devices did not operate in writethrough mode even
> >>>> -after a driver announced lack of support for VIRTIO_BLK_F_FLUSH.
> >>>> +If the VIRTIO_BLK_F_FLUSH feature is not negotiated while
> >>>> +VIRTIO_BLK_F_CONFIG_WCE is, \field{writeback} MUST read 0 immediately
> >>>> +after feature negotiation.
> >>>
> >>> This is confusing.
> >>> Previously you simply said writeback is only valid if
> >>> VIRTIO_BLK_F_CONFIG_WCE is negotiated.
> >>
> >> This paragraph says "If the VIRTIO_BLK_F_FLUSH feature is not negotiated
> >> *while VIRTIO_BLK_F_CONFIG_WCE is*".
> > 
> > Would be clearer to revert the order then.
> 
> Sure, I can do that.
> 
> > Is this logic something that legacy devices implemented?
> > I don't see anything like this in QEMU.
> 
> It wasn't possible because of the lack of a FEATURES_OK state.  I'll
> have to document legacy behavior better.
> 
> >>> Also, what's the reset value if it is negotiated?
> >>
> >> It is not specified.  The device can make it persistent or reset it to a
> >> user-specified value.
> > 
> > Does driver have to read it on init then?
> 
> Read it or set it.  Linux does the latter.

Is there value in specifying a non zero value on reset?
Maybe we can make this 0 unconditionally?


> > I think this text could benefit from separating
> > between two concepts:
> > - device writenback mode
> > - writeback field
> > 
> > operation should refer to current mode, separately
> > field might or might not make it discoverable.
> > 
> > I see this in QEMU:
> > 
> >     /* A guest that supports VIRTIO_BLK_F_CONFIG_WCE must be able to send
> >      * cache flushes.  Thus, the "auto writethrough" behavior is never
> >      * necessary for guests that support the VIRTIO_BLK_F_CONFIG_WCE feature.
> >      * Leaving it enabled would break the following sequence:
> >      *
> >      *     Guest started with "-drive cache=writethrough"
> >      *     Guest sets status to 0
> >      *     Guest sets DRIVER bit in status field
> >      *     Guest reads host features (WCE=0, CONFIG_WCE=1)
> >      *     Guest writes guest features (WCE=0, CONFIG_WCE=1)
> >      *     Guest writes 1 to the WCE configuration field (writeback mode)
> >      *     Guest sets DRIVER_OK bit in status field
> >      *
> >      * s->blk would erroneously be placed in writethrough mode.
> >      */
> >     if (!virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) {
> >         aio_context_acquire(blk_get_aio_context(s->blk));
> >         blk_set_enable_write_cache(s->blk,
> >                                    virtio_has_feature(vdev, VIRTIO_BLK_F_WCE));
> >         aio_context_release(blk_get_aio_context(s->blk));
> >     }
> > 
> > 
> > is this what this text is trying to say?
> 
> This is part of the behavior that I want to clarify.

Can't say I see how this maps into the text.
you don't document what happens without VIRTIO_BLK_F_CONFIG_WCE
at all, do you?


> > 
> > 
> > 
> > 
> >>> We can't have a mandatory behaviour but also
> >>> make it optional. And driver does not propose features, it negotiates
> >>> them.
> >>
> >> Will fix.
> >>
> >>> Or maybe device must fail negotiation if it meets some criteria?
> >>
> >> Yes, but just "MAY".
> > 
> > Devices may fail negotiation if any dependencies are not met.
> > Is this the case?
> > See Feature bit requirements that we have for the net device,
> > I think that's formulated in a clearer way.
> 
> Good.  But I don't want to make it mandatory.  I'll see what I can come
> up with in v3.
> 
> > 
> >>>>  \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Operation}
> >>>>  
> >>>> @@ -3871,8 +3882,25 @@ 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.
> >>>> +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.
> >>>
> >>> The 1st one is a SHOULD because not everyone cares about data loss.
> >>> But why is the second one a MUST? Same logic seems to apply.
> >>
> >> Because then the driver can _know_ that it is safe from data losses, by
> >> looking at proposed features.
> > 
> > I still don't see a difference. the point of the 1st
> > SHOULD is that maybe it's used with e.g. -snapshot
> > so data will be discarded on qemu exit anyway.
> 
> No, that's not the point of the 1st SHOULD.  From the point of view of
> the guest, if you use "-snapshot" it is totally irrelevant whether the
> host commits to non-volatile storage.  As you wrote, data will be
> discarded so there is no separation between volatile and non-volatile
> storage.
> 
> The point of the first SHOULD is simply to document reasonable behavior
> for the case of simple drivers that do not care about data loss.

Hmm why do such *drivers* exist, or should be allowed?


> Devices that do not support VIRTIO_BLK_F_FLUSH probably would not obey
> that SHOULD.  But devices that do support VIRTIO_BLK_F_FLUSH need to do
> better, hence the MUST.
> 
> Do you think the MUST prevents the trick with "-snapshot"?  I think it
> doesn't, because the guest cannot see the difference.


Well MUST means if you don't do it you violate spec.

>  (And I think even
> "MUST"s can be overridden by a user's "I know what I am doing" option
> such as QEMU's cache.no-flush=on).

I think that's exactly the SHOULD. SHOULD means "do this always unless
you really know what you are doing". Check out the RFC.


> > Maybe we should specify that data loss is in case of a crash.
> 
> Yes.
> 
> > This seems to apply to all cases that might lead to data loss.
> > 
> > IOW driver doesn't really know it's safe from losses since
> > backend can still lose data. All driver cares about is
> > what is required from driver to avoid losses.
> 
> That's the point: do not propose VIRTIO_BLK_T_FLUSH if you can lose
> unflushed data from a crash.
> 
> >>>> +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.
> >
> > If writeback is enabled, device can cache requests,
> > and driver must issue explicit flushes, correct?
> > If writeback is disabled, driver may assume data
> > is committed after request completes.
> > 
> > Above should be conformance statements for the driver.
> 
> I'll add something, but I don't think there should be conformance
> statement for the drivers about this.  Every "MUST" for the driver can
> be rephrased to a "MAY assume" on the driver and vice versa.

True but this is an important topic so it bears repetition I think.

> > 
> >>>> +
> >>>> +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
> >>>
> >>> since the last VIRTIO_BLK_F_FLUSH completed, correct?
> >>
> >> More precisely "submitted while the field was 1 and completed after the
> >> last VIRTIO_BLK_F_FLUSH was submitted".  Does it have to be specified
> >> explicitly?
> > 
> > If we want it understood correctly, I guess yes.
> 
> Ok.  I have reworked the whole description of when a write becomes
> stable.  It is longer but I think it leaves no space for interpretation.
> 
> Paolo