Re: [Qemu-devel] [PATCH v12 03/14] qcow2: Optimize bdrv_make_empty()

[Max Reitz <mreitz@redhat.com>]

Am 10.10.2014 um 14:32 schrieb Eric Blake:
> On 08/26/2014 03:36 PM, Max Reitz wrote:
>> bdrv_make_empty() is currently only called if the current image
>> represents an external snapshot that has been committed to its base
>> image; it is therefore unlikely to have internal snapshots. In this
>> case, bdrv_make_empty() can be greatly sped up by emptying the L1 and
>> refcount table (while having the dirty flag set) and creating a trivial
>> refcount structure.
>>
>> If there are snapshots, fall back to the simple implementation (discard
>> all clusters).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkdebug.c      |   2 +
>>   block/qcow2.c         | 137 ++++++++++++++++++++++++++++++++++++++++++++------
>>   include/block/block.h |   2 +
>>   3 files changed, 126 insertions(+), 15 deletions(-)
>>
>> +    } else {
>> +        int l1_clusters;
>> +        int64_t offset;
>> +        uint64_t *new_reftable;
>> +        uint8_t l1_ofs_rt_ofs_cls[20]; /* L1 offset; RT offset and clusters */
>> +        uint64_t rt_entry;
>> +
>> +        ret = qcow2_cache_empty(bs, s->l2_table_cache);
>>           if (ret < 0) {
>> -            break;
>> +            return ret;
>> +        }
>> +
>> +        ret = qcow2_cache_empty(bs, s->refcount_block_cache);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +
>> +        /* Refcounts will be broken utterly */
>> +        ret = qcow2_mark_dirty(bs);
> qcow2_mark_dirty does assert(s->qcow_version >= 3).  But this code can
> be reached when committing a qcow2 0.10 compat-level file, right?

You're right, this code doesn't work for compat=0.10. As you proposed in 
your own answer to this, I'll just use the fallback code in that case 
(in v13)

>> +        /* "Create" an empty reftable (one cluster) directly after the image
>> +         * header and an empty L1 table three clusters after the image header;
>> +         * the cluster between those two will be used as the first refblock */
>> +        cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 0], 3 * s->cluster_size);
>> +        cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 8],     s->cluster_size);
>> +        cpu_to_be32w((uint32_t *)&l1_ofs_rt_ofs_cls[16], 1);
> The offsets here feel a bit magic.

Well, such byte offsets are everywhere in the qcow2 code which does such 
small writes to the image header. Making them non-magic would mean 
offsetof(...) - offsetof(...). Can and will do, but I probably won't 
find it much more readable. ;-)

Max