Re: [Qemu-devel] [PATCH 1/3 v9] add-cow file format

[Stefan Hajnoczi]

On Wed, May 30, 2012 at 2:50 AM, Dong Xu Wang
<address@hidden> wrote:
> On Tue, May 29, 2012 at 11:50 PM, Stefan Hajnoczi <address@hidden> wrote:

I thought a bit more about locking.  Because the metadata is simple
not much locking is necessary except when fetching new bitmap clusters
from the image file into the cache and when populating untouched
sectors during data cluster allocation.  Those are the two cases where
parallel requests could put the block driver or image file into a bad
state if allowed to run without any locking.

Another way of describing the consequences of parallelism:
1. Coroutines must not duplicate the same add-cow bitmap cluster into
the cache if they run at the same time.
2. Coroutines must not hold bitmap tables across blocking operations
since the cache entry has no reference count and might be evicted from
the cache.
3. Coroutines must not allocate the same data cluster simultaneously
because untouched head/tail sectors must never race with guest writes.

>>> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t size)
>>> +{
>>> +    BDRVAddCowState *s = bs->opaque;
>>> +    int sector_per_byte = SECTORS_PER_CLUSTER * 8;
>>> +    int ret;
>>> +    int64_t old_image_sector = s->image_hd->total_sectors;
>>> +    int64_t bitmap_size =
>>> +        (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte;
>>> +
>>> +    ret = bdrv_truncate(bs->file,
>>> +        sizeof(AddCowHeader) + bitmap_size);
>>> +    if (ret < 0) {
>>> +        bdrv_truncate(s->image_hd, old_image_sector * BDRV_SECTOR_SIZE);
>>
>> Why truncate image_hd on failure?  We never touch the image_hd size on
>> success either.  I think we can just leave it alone.
>
> That means whether we truncate add-cow fails or not ,we should not never touch
> image_hd size?

I thought about this more and I think we should truncate image_hd in
the success case only.  In order to resize the image we need to resize
the cow bitmap and then resize image_hd.  If resizing the add-cow file
failed, then we haven't changed the cow bitmap and we don't need to
truncate image_hd.  Do you agree with this or have I missed something?

>>> @@ -828,6 +832,41 @@ static int img_convert(int argc, char **argv)
>>>     }
>>>
>>>     /* Create the new image */
>>> +
>>> +    if (0 == strcmp(out_fmt, "add-cow")) {
>>> +        image_drv = bdrv_find_format("raw");
>>> +        if (!drv) {
>>> +            ret = -1;
>>> +            goto out;
>>> +        }
>>> +        snprintf(image_filename, sizeof(image_filename),
>>> +            "%s"".ct.raw", out_filename);
>>> +        ret = bdrv_create(image_drv, image_filename, image_param);
>>> +        if (ret < 0) {
>>> +            error_report("%s: error while creating image_file: %s",
>>> +                     image_filename, strerror(-ret));
>>> +            goto out;
>>> +        }
>>> +        set_option_parameter(param, BLOCK_OPT_IMAGE_FILE, image_filename);
>>> +
>>> +        if (!out_baseimg) {
>>> +            backing_drv = bdrv_find_format("qcow2");
>>> +            if (!drv) {
>>> +                ret = -1;
>>> +                goto out;
>>> +            }
>>> +            snprintf(backing_filename, sizeof(backing_filename),
>>> +                "%s"".ct.qcow2", out_filename);
>>> +            ret = bdrv_create(backing_drv, backing_filename, image_param);
>>> +            if (ret < 0) {
>>> +                error_report("%s: error while creating backing_file: %s",
>>> +                         backing_filename, strerror(-ret));
>>> +                goto out;
>>> +            }
>>> +            set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
>>> +                backing_filename);
>>> +        }
>>> +    }
>>
>> If this diff hunk is dropped then the user needs to manually create
>> the raw file before running qemu-img convert?
>>
>> qemu-img convert -O add-cow seems like a very rare case.  I'm not sure
>> we should add special user-friend hacks for this.
>>
>> I'm not sure I understand why you create a qcow2 file either.
>>
> Yes, if we use "qemu-img convert -O add-cow", we should create 2 other files,
> raw file and qcow2(I just picked  up qcow2, other formats is also okay) file,
> as image_file and backing_file, without the two files, .add-cow file can not
> work properly.
>
> Although it will occour in very rare cases, I wish to pass all qemu-iotests
> cases, so I added these code.
>
> Do you think these are not necessary? And some qemu-iotests cases are
> using "convert" operation, If I do not write previous code, these cases will
> fail. Can I let these cases do not support add-cow?

If a test uses qemu-img convert then it's probably not that
interesting for add-cow.  Converting is not a useful operation because
add-cow is an "add-on" block driver that adds a feature on top of raw,
rather than a format like vmdk or qcow2 which is used to share disk
images.  I see why you did this to make qemu-iotests work, but
personally I would drop this special case code and skip those tests.

Stefan