qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Dong Xu Wang
Subject: Re: [Qemu-devel] [PATCH 1/3 v9] add-cow file format
Date: Mon, 4 Jun 2012 16:15:43 +0800

Okay, thanks all of your comments, if no other comments, I will write
next version.


On Wed, May 30, 2012 at 4:20 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]