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 01/10/2013 07:22, Rusty Russell ha scritto:
>> Rusty Russell <rusty@au1.ibm.com> writes:
>>> Thinking about this some more: why not make WCE the only option?
>>>
>>> (1) It's simple.
>>> (2) We say flush SHOULD hit the disk.
>> 
>> OK, here's the actual patch.  It applies most of Paulo's patch,
>> but to the legacy section, and remove VIRTIO_BLK_F_FLUSH.  It also
>> documents what FLUSH should do (is more clarity required?)
>
> Actually that's a pretty smart thing to do. :)  Just a couple comments
> below.
>
>> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
>> index 5efeba7..9b5c29a 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.
>
> Do you need to document where the wce flag is in the configuration?

Yes, it's the 'u8 reserved' field above, and referred to below:

>> +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,22 @@ 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

Here.

>> +VIRTIO_BLK_F_CONFIG_WCE feature is offered, the cache mode should be
>> +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_WCE: the cache will be in writeback mode
>> +after reset if and only if VIRTIO_BLK_F_WCE is negotiated.

> s/VIRTIO_BLK_F_WCE/VIRTIO_BLK_F_FLUSH/ everywhere

Yep, thanks.

>> +Note that buggy legacy devices are common, which always operate in
>> +write back mode even if VIRTIO_BLK_F_WCE isn't accepted, or writeback
>> +is turned off.
>
> I would remove this paragraph.  You found bugs, true, but it's not that
> the affected versions of QEMU are common---they are neither in Debian
> nor in RHEL/CentOS, and frankly using anything else in production would
> be crazy (also if you do I hope you'll update fast, and those hosts will
> not be "common" anymore by the time standardization is finished).

I was also thinking of this:

[30] Until version 1.1, QEMU remained in writeback mode even after a
     guest announced lack of support for VIRTIO_BLK_F_FLUSH.

Cheers,
Rusty.