Re: [Qemu-devel] Re: [PATCH v2 6/7] qed: Read/write support

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

On 10/13/2010 10:08 AM, Avi Kivity wrote:
>  On 10/13/2010 04:53 PM, Anthony Liguori wrote:
>> On 10/13/2010 09:16 AM, Avi Kivity wrote:
>>>  On 10/13/2010 04:11 PM, Anthony Liguori wrote:
>>>>> Why would you ever update the header, apart from relocating L1 for 
>>>>> some reason?
>>>>
>>>>
>>>> To update the L1/L2 tables clean bit.  That's what prevents a check 
>>>> in the normal case where you have a clean shutdown.
>>>
>>> I see - so you wouldn't update it every allocation, only when the 
>>> disk has been quiet for a while.
>>
>> Right, the current plan is to flush the header dirty bit on shutdown 
>> or whenever there is an explicit flush of the device.  Current that 
>> is caused by either a guest-initiated flush or a L1 update.
>
> That does add an extra write (and a new write+flush later to mark the 
> header dirty again when you start allocating).  I'd drop it and only 
> use the timer.
>
> in fact, it adds an extra flush too.  The sequence
>
> 1 L1 update
> 2 mark clean
> 3 flush
>
> is unsafe since you can crash between 2 and 3, ad only 2 makes it.  So 
> I'd do something like

You've got the order wrong.

1. L1 update
2. flush()
3. mark clean

If (3) doesn't make it to disk, that's okay.  It just causes an extra scan.

> 1 opportunistic flush (for whatever reason)
> 2 set timer
> 3 no intervening metadata changes
> 4 mark clean
> 5 no intervening metadata changes
> 6 mark dirty
> 7 flush
> 8 metadata changes

Not sure I see why we set the timer in step 2 as opposed to:

0 clear scheduled flush (if necessary)
1 opportunistic flush (for whatever reason)
2 mark clean
3 no intervening metadata changes
4 mark dirty
5 flush
6 schedule flush (in 5 minutes)
7 metadata changes

Which is now recorded at http://wiki.qemu.org/Features/QED/ScanAvoidance 
so we can keep track of this.

Regards,

Anthony Liguori

>
>