[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 16/32] qcow2: Update l2_allocate
From: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 16/32] qcow2: Update l2_allocate() to support L2 slices |
Date: |
Wed, 17 Jan 2018 16:42:56 +0100 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
On Tue 16 Jan 2018 05:52:36 PM CET, Anton Nefedov wrote:
>> @@ -299,42 +300,50 @@ static int l2_allocate(BlockDriverState *bs, int
>> l1_index, uint64_t **table)
>>
> [..]>
>> - /* write the l2 table to the file */
>> - BLKDBG_EVENT(bs->file, BLKDBG_L2_ALLOC_WRITE);
>> + trace_qcow2_l2_allocate_write_l2(bs, l1_index);
>> + qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>> + ret = qcow2_cache_flush(bs, s->l2_table_cache);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>>
>
> Might be an overoptimization but do we really have to flush each slice
> separately?
> Otherwise a number of write ops will remain the same but at least we
> would bdrv_flush() just once.
You're completely right, I'll fix that.
>> } else {
>> + uint64_t new_l2_offset;
>> /* First allocate a new L2 table (and do COW if needed) */
>> - ret = l2_allocate(bs, l1_index, &l2_table);
>> + ret = l2_allocate(bs, l1_index);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + /* Get the offset of the newly-allocated l2 table */
>> + new_l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK;
>> + assert(offset_into_cluster(s, new_l2_offset) == 0);
>> + /* Load the l2 table in memory */
>> + ret = l2_load(bs, offset, new_l2_offset, &l2_table);
>
> Cosmetic: maybe this l2_load() better be merged with the copied L2 case?
> e.g.
>
> if (!(l1_table[l1_index] & COPIED))
> l2_allocate();
> l2_load();
I'm not sure about that, since there's also the qcow2_free_clusters()
call afterwards, so the final code might be harder to follow.
Berto