Re: [Qemu-devel] [PATCH 2/8] block: add live block commit functionality

[Jeff Cody <jcody@redhat.com>]

On 09/14/2012 11:45 AM, Eric Blake wrote:
> On 09/14/2012 07:41 AM, Jeff Cody wrote:
>> This adds the live commit coroutine.  This iteration focuses on the
>> commit only below the active layer, and not the active layer itself.
>>
>> The behaviour is similar to block streaming; the sectors are walked
>> through, and anything that exists above 'base' is committed back down
>> into base.  At the end, intermediate images are deleted, and the
>> chain stitched together.  Images are restored to their original open
>> flags upon completion.
>>
> 
>> +
>> +enum {
>> +    /*
>> +     * Size of data buffer for populating the image file.  This should be large
>> +     * enough to process multiple clusters in a single call, so that populating
>> +     * contiguous regions of the image is efficient.
>> +     */
>> +    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
> 
> I'm guessing you will add a followup patch that depends on Paolo's
> series for controlling the granularity of this buffer?  Or is it less
> important for the commit case?

For the version of commit implemented by these patches, I don't think
controlling the granularity of the commit buffer is important.  However,
for the next stage, when we are commiting the active layer, then it may
become more important.  That stage will likely have a lot of code reuse
from Paolo's series, as well.

> 
>> +
>> +static void coroutine_fn commit_run(void *opaque)
>> +{
> 
>> +    ret = base_len = bdrv_getlength(base);
>> +    if (base_len < 0) {
>> +        goto exit_restore_reopen;
>> +    }
>> +
>> +    if (base_len < s->common.len) {
>> +        ret = bdrv_truncate(base, s->common.len);
>> +        if (ret) {
>> +            goto exit_restore_reopen;
>> +        }
>> +    }
> 
> Question: is it valid to have a qcow2 file whose size is smaller than
> it's backing image?

I don't think so... however:

>  Suppose I have base[1M] <- mid[2M] <- top[3M] <-
> active[3M], and request to commit top into base.  This bdrv_truncate()
> means I will now have:
> 
> base[3M] <- mid[2M] <- top[3M] <- active[3M].
> 
> If I then abort the commit operation at this point, then we have the
> situation of 'mid' reporting a smaller size than 'base' - which may make
> 'mid' invalid.  And even if it is valid, what happens if I now request
> to commit 'mid' into 'base', but 'base' already had data written past
> the 2M mark before I aborted the first operation?

Once the commit starts, I don't know if you can safely abort it, and
still count on 'mid' being valid.  Ignoring potential size differences,
how would you ever know that what was written from 'top' into 'base' is
compatible with what is present in 'mid'?

Once you begin a commit, your chain as an entirety can stay safe after
an abort, as long as it is accessed from 'top' or above... but I think
you have to consider the intermediate images between 'base' and 'top' to
be invalid as standalone images.

> 
> I'm worried that you may have to bdrv_truncate() the entire chain to
> keep it consistent, which is more complex because it requires more r/w
> files.
> 

Transactional bdrv_truncate()!  :)