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 <stefa...@gmail.com> wrote:
> On Wed, May 30, 2012 at 2:50 AM, Dong Xu Wang
> <wdon...@linux.vnet.ibm.com> wrote:
>> On Tue, May 29, 2012 at 11:50 PM, Stefan Hajnoczi <stefa...@gmail.com> 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
>
