Re: [Qemu-devel] [PATCH 01/12] VMDK: Introduced VmdkExtent

[Fam Zheng <famcool@gmail.com>]

On Thu, Jun 9, 2011 at 3:58 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sat, Jun 04, 2011 at 08:40:22AM +0800, Fam Zheng wrote:
>>   fail:
>> -    qemu_free(s->l1_backup_table);
>> -    qemu_free(s->l1_table);
>> -    qemu_free(s->l2_cache);
>> +    if(s->extents) {
>> +        qemu_free(s->extents[0].l1_backup_table);
>> +        qemu_free(s->extents[0].l1_table);
>> +        qemu_free(s->extents[0].l2_cache);
>> +    }
>
> for (i = 0; i < s->num_extents; i++) {
>    qemu_free(s->extents[i].l1_backup_table);
>    qemu_free(s->extents[i].l1_table);
>    qemu_free(s->extents[i].l2_cache);
> }
> qemu_free(s->extents);
>

This looks better, although num_extents is 1 at most here.

>> +static int find_extent(BDRVVmdkState *s, int64_t sector_num, int start_idx)
>> +{
>> +    int ext_idx = start_idx;
>> +    while (ext_idx < s->num_extents
>> +            && sector_num >= s->extents[ext_idx].sectors) {
>> +        sector_num -= s->extents[ext_idx].sectors;
>> +        ext_idx++;
>> +    }
>> +    if (ext_idx == s->num_extents) return -1;
>> +    return ext_idx;
>> +}
>
> Callers don't really need the index, they just want the extent and an
> optimized way of repeated calls to avoid O(N^2) find_extent() times:
>
> static VmdkExtent *find_extent(BDRVVmdkState *s, int64_t sector_num,
>                               VmdkExtent *start_extent)
> {
>    VmdkExtent *extent = start_extent;
>
>    if (!start_extent) {
>        extent = &s->extent[0];
>    }
>
>    while (extent < &s->extents[s->num_extents]) {
>        if (sector_num < extent->end) {
>            return extent;
>        }
>        extent++;
>    }
>    return NULL;
> }
>
> I added the VmdkExtent.end field so that this function can be called
> repeatedly for an increasing series of sector_num values.  It seems like
> your code would fail when called with a non-0 index since sector_num -=
> s->extents[ext_idx].sectors is incorrect when starting from an arbitrary
> extent_idx.
>

Parameter sector_num which I meant was relative to start_idx, so
caller  passes a relative sector_num when calling with a non-0 index.

>> +
>>  static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_num,
>>                               int nb_sectors, int *pnum)
>>  {
>>      BDRVVmdkState *s = bs->opaque;
>> -    int index_in_cluster, n;
>> -    uint64_t cluster_offset;
>>
>> -    cluster_offset = get_cluster_offset(bs, NULL, sector_num << 9, 0);
>> -    index_in_cluster = sector_num % s->cluster_sectors;
>> -    n = s->cluster_sectors - index_in_cluster;
>> +    int index_in_cluster, n, ret;
>> +    uint64_t offset;
>> +    VmdkExtent *extent;
>> +    int ext_idx;
>> +
>> +    ext_idx = find_extent(s, sector_num, 0);
>> +    if (ext_idx == -1) return 0;
>> +    extent = &s->extents[ext_idx];
>> +    if (s->extents[ext_idx].flat) {
>> +        n = extent->sectors - sector_num;
>
> If I have two flat extents:
> Extent A: 0     - 1.5MB
> Extent B: 1.5MB - 2MB
>
> And I call vmdk_is_allocated(sector_num=1.5MB), then n = 512KB - 1.5MB
> which is negative!  Also n is an int but it should be an int64_t (or
> uint64_t) which can hold sector units.

You are right. I did this because I wasn't considering multi extent
situations yet in this patch, and when introducing multi extents
support this problem is fixed.


-- 
Best regards!
Fam Zheng