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

[Stefan Hajnoczi]

On Thu, Jun 9, 2011 at 11:20 AM, Fam Zheng <address@hidden> wrote:
> On Thu, Jun 9, 2011 at 3:58 PM, Stefan Hajnoczi <address@hidden> 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.

The important thing is to free s->extents.

>>> +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.

How can they calculate a relative sector_num?  It seems to me like
they need to do this manually by traversing the extents again.  I
think this exposes too much of the find_extents() iteration, it would
be simpler to switch to the VmdkExtent *find_extent(BDRVVmdkState *s,
int64_t sector_num, VmdkExtent *start_extent) function above.

Also, feel free to include doc comments to explain what function
arguments and return value do.

>>> +
>>>  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.

Okay, I only looked at the first patch so far.  Please try to
structure patch series so they build on each other and do not
introduce intermediate build failures or bugs.  This is important so
that git-bisect(1) works and to make review straightforward (I can't
keep 12 patches in my head at the same time, I need to review one at a
time :)).

If it is necessary to introduce a temporary bug or weirdness in an
earlier patch, please include a comment or something in the commit
description to explain what is going to happen.

Stefan