Re: [virtio] Re: [virtio-dev] Re: [virtio-comment] Problems with VIRTIO-4 and writeback only disks

[Rusty Russell <rusty@au1.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 03/10/2013 07:05, Rusty Russell ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> Il 02/10/2013 02:50, Rusty Russell ha scritto:
>>>>>> I would make this even stronger, like "It is undefined whether a disk
>>>>>> operates in writeback or writethrough mode after a guest announces lack
>>>>>> of support for VIRTIO_BLK_F_FLUSH".
>>>> At this point it's documenting historical behavior, rather than
>>>> proscribing it.  How about we simply note:
>>>>
>>>> Some older legacy devices did not operate in writethrough mode even
>>>> after a guest announced lack of support for VIRTIO_BLK_F_FLUSH.
>>>
>>> That's fine.
>>>
>>> Paolo
>> 
>> Here's the final patch then:
>> 
>> commit ff7486a8699c06743473c0078e9b511275d65306
>> Author: Rusty Russell <rusty@au1.ibm.com>
>> Date:   Tue Oct 1 14:53:17 2013 +0930
>> 
>>     virtio_blk: deprecate flush.
>>     
>>     This is partially a port of Paolo Bonzini <pbonzini@redhat.com>'s
>>     WCE spec changes, but puts them in the legacy section.  It reserves
>>     the writeback field for compatibility.
>>     
>>     Flush is now required.
>>     
>>     See: https://lists.oasis-open.org/archives/virtio-comment/201308/msg00078.html
>>     
>>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
>> 
>> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
>> index 5efeba7..a9c0cce 100644
>> --- a/virtio-v1.0-wd01-part1-specification.txt
>> +++ b/virtio-v1.0-wd01-part1-specification.txt
>> @@ -1817,8 +1817,6 @@ device except where noted.
>>  
>>    VIRTIO_BLK_F_BLK_SIZE (6) Block size of disk is in “blk_size”.
>>  
>> -  VIRTIO_BLK_F_FLUSH (9) Cache flush command support.
>> -
>>    VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O
>>      alignment.
>>  
>> @@ -1843,6 +1841,7 @@ device except where noted.
>>  			u16 min_io_size;
>>  			u32 opt_io_size;
>>  		} topology;
>> +		u8 reserved;
>>  	};
>>  
>>  2.4.2.3.1 Legacy Interface: Feature bits
>> @@ -1851,6 +1850,15 @@ device except where noted.
>>  
>>    VIRTIO_BLK_F_SCSI (7) Device supports scsi packet commands.
>>  
>> +  VIRTIO_BLK_F_FLUSH (9) Cache flush command support.
>> +
>> +  VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback
>> +    and writethrough modes.
>> +
>> +VIRTIO_BLK_F_FLUSH was also called VIRTIO_BLK_F_WCE: Legacy drivers
>> +should only negotiate this feature if they are capable of sending
>> +VIRTIO_BLK_T_FLUSH commands.
>> +
>>  2.4.2.4. Device Initialization
>>  -----------------------------
>>  
>> @@ -1872,6 +1880,21 @@ device except where noted.
>>    I/O lengths for the driver to use. This also does not affect the units
>>    in the protocol, only performance.
>>  
>> +2.4.2.4.1. Legacy Interface: Device Initialization
>> +-----------------------------
>> +
>> +The reserved field used to be called writeback.  If the
>> +VIRTIO_BLK_F_CONFIG_FLUSH feature is offered, the cache mode should be
>
> CONFIG_WCE?

Oops, oversed.  Thanks.

>> +read from the writeback field of the configuration if available; 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.
>> +
>> +Some older legacy devices did not operate in writethrough mode even
>> +after a guest announced lack of support for VIRTIO_BLK_F_FLUSH.
>> +
>>  2.4.2.5. Device Operation
>>  ------------------------
>>  
>> @@ -1907,6 +1930,9 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by host:
>>  	#define VIRTIO_BLK_S_IOERR     1
>>  	#define VIRTIO_BLK_S_UNSUPP    2
>>  
>> +Any writes completed before the submission of the flush command should
>> +be committed to permanent storage by the device.
>
> s/permanent/non-volatile/
>
> Putting them in a non-volatile cache is fine.

Thanks, fixed.

Cheers,
Rusty.