Re: [Qemu-devel] [PATCH V12 5/6] add-cow file format

[Kevin Wolf]

Am 12.09.2012 09:28, schrieb Dong Xu Wang:
>>> +static bool is_allocated(BlockDriverState *bs, int64_t sector_num)
>>> +{
>>> +    BDRVAddCowState *s  = bs->opaque;
>>> +    BlockCache *c = s->bitmap_cache;
>>> +    int64_t cluster_num = sector_num / SECTORS_PER_CLUSTER;
>>> +    uint8_t *table      = NULL;
>>> +    uint64_t offset = ADD_COW_PAGE_SIZE * s->header.header_pages_size
>>> +        + (offset_in_bitmap(sector_num) & (~(c->entry_size - 1)));
>>> +    int ret = block_cache_get(bs, s->bitmap_cache, offset,
>>> +        (void **)&table, BLOCK_TABLE_BITMAP, ADD_COW_CACHE_ENTRY_SIZE);
>>
>> No matching block_cache_put?
>>
>>> +
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +    return table[cluster_num / 8 % ADD_COW_CACHE_ENTRY_SIZE]
>>> +        & (1 << (cluster_num % 8));
>>> +}
>>> +
>>> +static coroutine_fn int add_cow_is_allocated(BlockDriverState *bs,
>>> +        int64_t sector_num, int nb_sectors, int *num_same)
>>> +{
>>> +    BDRVAddCowState *s = bs->opaque;
>>> +    int changed;
>>> +
>>> +    if (nb_sectors == 0) {
>>> +        *num_same = 0;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (s->header.features & ADD_COW_F_All_ALLOCATED) {
>>> +        *num_same = nb_sectors - 1;
>>
>> Why - 1?
>>
>>> +        return 1;
>>> +    }
>>> +    changed = is_allocated(bs, sector_num);
>>> +
>>> +    for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
>>> +        if (is_allocated(bs, sector_num + *num_same) != changed) {
>>> +            break;
>>> +        }
>>> +    }
>>> +    return changed;
>>> +}

>>> +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;
>>> +    uint32_t bitmap_pos = s->header.header_pages_size * ADD_COW_PAGE_SIZE;
>>> +    int64_t bitmap_size =
>>> +        (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte;
>>> +    bitmap_size = (bitmap_size + ADD_COW_CACHE_ENTRY_SIZE - 1)
>>> +        & (~(ADD_COW_CACHE_ENTRY_SIZE - 1));
>>> +
>>> +    ret = bdrv_truncate(bs->file, bitmap_pos + bitmap_size);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +    return 0;
>>> +}
>>
>> So you don't truncate s->image_file? Does this work?
> 
> s->image_file should be truncated? Image file can have a larger virtual size
> than backing_file, my understanding is we should not truncate image file.

I'm talking about s->image_hd, not bs->backing_hd. You are right that
the backing file should not be changed. But the associated raw image
should be resized, shouldn't it?

>>> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
>>> +{
>>> +    BDRVAddCowState *s = bs->opaque;
>>> +    int ret;
>>> +
>>> +    qemu_co_mutex_lock(&s->lock);
>>> +    ret = block_cache_flush(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP,
>>> +        ADD_COW_CACHE_ENTRY_SIZE);
>>> +    qemu_co_mutex_unlock(&s->lock);
>>> +    return ret;
>>> +}
>>
>> What about flushing s->image_file?

>>> +typedef struct AddCowHeader {
>>> +    uint64_t        magic;
>>> +    uint32_t        version;
>>> +
>>> +    uint32_t        backing_filename_offset;
>>> +    uint32_t        backing_filename_size;
>>> +
>>> +    uint32_t        image_filename_offset;
>>> +    uint32_t        image_filename_size;
>>> +
>>> +    uint64_t        features;
>>> +    uint64_t        optional_features;
>>> +    uint32_t        header_pages_size;
>>> +} QEMU_PACKED AddCowHeader;
>>
>> Why aren't backing/image_file_format part of the header here? They are
>> in the spec. It would also simplify some offset calculation code.
>>
> 
> Anthony said "It's far better to shrink the size of the header and use
> an offset/len
> pointer to the backing file string.  Limiting backing files to 1023 is
> unacceptable"
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04110.html
> 
> So I use offset  and length instead of using string directly.

I'm talking about the format, not the path.

Kevin