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

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/07/2015 13:10, Michael S. Tsirkin wrote:
> On Wed, Jul 08, 2015 at 01:04:40PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 08/07/2015 12:53, Michael S. Tsirkin wrote:
>>>>> Below 2 paragraphs tell you why :)
>>>>> Because it does not always *have* non-volatile storage.
>>>>
>>>> So are we still discussing what MUST means?
>>>
>>> I'm discussing what non-volatile means.
>>
>> Since we're down to just 1 or 2 contentious points, I'm sending v4.
>> Let's reboot the discussion on top of that version.  Just a couple
>> comments below:
>>
>>>> If the guest cannot see the
>>>> difference, QEMU is not violating anything.
>>>
>>> Of course it can see the difference after reset.
>>
>> Not after reset, only after a cold restart (there was a
>> crash---restoring from migration).  A lot more changes could happen
>> between shutdown (doesn't matter if clean or crashy) and the next cold
>> restart, and the guest can detect them the same way.
>>
>> E.g. in one case the guest could detect that the kernel was updated via
>> libguestfs.  In another the guest could detect that the storage
>> (formerly in tmpfs, though the guest didn't know that) is now all zero.
> 
> Absolutely. I think this means we can't say "spec tells device MUST NOT
> lose data, but qemu loses it and that is ok since no one ever can
> observe that". It's observable, and if it's sometimes okay,
> appropriate statement is SHOULD NOT lose data.

I think you're putting yourself out of the spec by using tmpfs, because
there is no "permanent device backend storage" to talk of.  The user
option to eliminate flushes is the same thing: the user is telling you
to treat device backend storage as transient (not permanent).

As long as there is permanent device backend storage, QEMU is fine.

Perhaps you can propose a patch on top of v4 that adds the "device
backend storage is transient" case?  I don't think it's necessary, but
perhaps it's a small change.

>>>> If they don't, they've already done something not recommended, and a MAY
>>>> is just fine in this paragraph.  This paragraph only applies to
>>>> implementation that have already disregarded that other "SHOULD".  It
>>>> makes little sense to do yet another recommendation that they would
>>>> disregard.
>>>>
>>>> This "MAY" is here to explicitly allow the behavior.
>>>
>>> Well ignoring that SHOULD leads to bad performance, ignoring
>>> this one leads to data loss. So you need to pause and think
>>> at both points ...
>>
>> I see your point.  However, whoever ignores that SHOULD is very likely
>> not caring about data safety anyway.  This MAY allows them to keep the
>> data safe at the price of bad performance, in the unlikely case that
>> they do care about data safety.
> 
> Looks like I was unclear.  What I was suggesting is reversing the
> case. Asking that data SHOULD be safe.

I understood perfectly :) but I think it's pointless.  If
VIRTIO_BLK_F_FLUSH is not even proposed, you can assume that the
implementor doesn't care about data safety.  The spec has already asked
that data SHOULD be safe (by proposing the feature), and that point was
disregarded.

Note that device and driver are asymmetrical here anyway:

       |                    VIRTIO_BLK_F_FLUSH
       +--------------------------------+-------------------------------
       |             yes                |            no
-------+--------------------------------+-------------------------------
device | MUST care about data safety    | MAY care about data safety*
-------+--------------------------------+-------------------------------
driver | SHOULD care about data safety  | cares about data safety
       | but perhaps won't send flushes | but delegates it to the device

         * with your proposal: SHOULD

Paolo