Re: [Qemu-devel] [RFC 18/24] qcow2: add qcow2_map

[Kevin Wolf <kwolf@redhat.com>]

[ Adding back qemu-devel to CC ]

Am 02.08.2011 06:27, schrieb Devin Nakamura:
> On Mon, Aug 1, 2011 at 10:32 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 29.07.2011 06:49, schrieb Devin Nakamura:
>>> Signed-off-by: Devin Nakamura <devin122@gmail.com>
>>> ---
>>>  block/qcow2-cluster.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/qcow2.c         |    1 +
>>>  block/qcow2.h         |    3 +++
>>>  3 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index ca56918..848f2ee 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -977,3 +977,52 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>>>
>>>      return 0;
>>>  }
>>> +
>>> +int qcow2_map(BlockDriverState *bs, uint64_t guest_offset,
>>> +    uint64_t host_offset, uint64_t contiguous_bytes)
>>> +{
>>> +    BDRVQcowState *s = bs->opaque;
>>> +    unsigned int nelms;
>>> +
>>> +
>>> +    if ((s->cluster_size - 1) & guest_offset) {
>>
>> Usually you would have guest_offset first.
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (contiguous_bytes % s->cluster_size) {
>>> +        return -EINVAL;
>>> +    }
>>
>> Any reason why the two checks are different? I don't really care if they
>> use & or %, but they should use the same thing.
> Hmm, I have no idea how that got there. Its especially weird, since I
> cant see myself writing the & check. The % one is much more my style.

The & version is a bit more efficient because it doesn't have to do a
division. But compared to I/O it doesn't make a big difference.

>>> +        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
>>> +
>>> +        for (; l2_index < nelms && contiguous_bytes > 0;  l2_index++) {
>>> +            l2_table[l2_index] = cpu_to_be64(host_offset | QCOW_OFLAG_COPIED);
>>
>> You should increase the refcount for host_offset and set
>> QCOW_OFLAG_COPIED only if it becomes 1. Or maybe we should just fail
>> bdrv_map if the old refcount is != 0.
> The problem with that is the refcounts are all 1. During
> open_conversion_target() the refcounts for all the clusters that are
> less than or equal to the file size are set to 1. This is to avoid
> allocating a cluster and overwriting original image data.

Hm, good point.

Then just check that the refcount is 1? If at some point we wanted to
support snapshots, how would we do that?

In any case, I think we need to update the comment of bdrv_map to say
something about the refcount of the clusters that are mapped (currently
something like "doesn't change the refcount, be sure to increase it
beforehand").

Kevin