Re: [Qemu-devel] Re: [PATCH 2/2] qcow2: Implement bdrv_truncate() for growing images

[Kevin Wolf]

Am 26.04.2010 16:01, schrieb Stefan Hajnoczi:
> From qcow_open():
> 
> /* read the level 1 table */
> s->l1_size = header.l1_size;
> shift = s->cluster_bits + s->l2_bits;
> s->l1_vm_state_index = (header.size + (1LL << shift) - 1) >> shift;
> /* the L1 table must contain at least enough entries to put
>    header.size bytes */
> if (s->l1_size < s->l1_vm_state_index)
>     goto fail;
> 
> Images where L1 size is not at least l1_vm_state_index cannot be
> opened by QEMU.  Right now the special case exists and perhaps some
> qcow2 code already relies on this assumption.

Hm, yes. You win.

>>> @@ -1095,6 +1094,40 @@ static int qcow_make_empty(BlockDriverState *bs)
>>>      return 0;
>>>  }
>>>
>>> +static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>> +{
>>> +    BDRVQcowState *s = bs->opaque;
>>> +    int ret, new_l1_size;
>>> +
>>> +    if (offset & 511) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* cannot proceed if image has snapshots */
>>> +    if (s->nb_snapshots) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    /* shrinking is currently not supported */
>>> +    if (offset < bs->total_sectors << BDRV_SECTOR_BITS) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    new_l1_size = size_to_l1(s, offset);
>>> +    ret = qcow2_grow_l1_image_data(bs, new_l1_size);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>
>> Instead of the confusing changes above you could just increase the L1
>> table size using the old function and keep the data/vmstate thing local
>> to qcow2_truncate (or maybe better a qcow2_move_vmstate(bs, offset)
>> which internally grows the L1 table as needed)
>>
>> Actually, I think this is not that different from your patch (you called
>> the almost same function qcow2_grow_l1_image_data and avoided the normal
>> calculation of the next L1 table size for some reason), but probably a
>> lot less confusing.
> 
> I like the qcow2_move_vmstate() name.  It is clearer than
> qcow2_grow_l1_image_data().
> 
> If I understand correctly, the next L1 table size calculation tries to
> grow the table in steps large enough so that the grow operation does
> not need to be performed frequently.  This makes sense when appending
> arbitrary vm state data, but the truncate operation knows the exact
> new size of the image and doesn't need to speculatively allocate more
> L1 table space.

Agreed. I just doubt that saving a few bytes is worth splitting up
qcow2_grow_l1_table when rounding up doesn't really hurt.

Maybe most of my real problem was really just naming, though. It made
everything look more complicated than it actually is. And the second
thing is probably that grow_l1_table started to do more than just
growing the L1 table.

>>> +
>>> +    /* write updated header.size */
>>> +    offset = cpu_to_be64(offset);
>>> +    if (bdrv_pwrite(bs->file, offsetof(QCowHeader, size), &offset,
>>> +                    sizeof(uint64_t)) != sizeof(uint64_t)) {
>>> +        return -EIO;
>>> +    }
>>
>> The vmstate_offset field needs to be updated somewhere, too. In my
>> suggestion this would be in qcow2_move_vmstate.
> 
> Which field (I can't find a vmstate_offset field)?  The patch updates
> s->l1_vm_state_index in qcow2_grow_l1_table_common(), is that the one
> you mean?

I meant in the header. And you can't find it because it doesn't exist,
s->l1_vm_state_index is calculated in qcow_open. Never let me review
patches when I'm tired. :-)

Kevin