On Thu 01 Feb 2018 04:23:09 PM CET, Anton Nefedov wrote:
>>> However, I'm wondering whether this is the best approach. The old
>>> L2 table is probably not going to be used after this function, so
>>> we're basically polluting the cache here. That was bad enough so
>>> far, but now that actually means wasting multiple cache entries on
>>> it.
>>>
>>> Sure, the code is simpler this way. But maybe it would still be
>>> better to manually copy the data over from the old offset... (As
>>> long as it's not much more complicated.)
>>
>> You mean bypassing the cache altogether?
>>
>> qcow2_cache_flush(bs, s->l2_table_cache);
>> new_table = g_malloc(s->cluster_size);
>> if (old_l2_offset & L1E_OFFSET_MASK) {
>> bdrv_pread(bs->file, old_l2_offset, new_table, s->cluster_size);
>> } else {
>> memset(new_table, 0, s->cluster_size);
>> }
>> bdrv_pwrite(bs->file, new_l2_offset, new_table, s->cluster_size);
>> g_free(new_table);
>>
>> ??
>
> (I know it's a draft so you probably just skipped that but just in
> case) It seems ok to bypass the cache read - perhaps even a flush is
> not necessary: old_l2_offset must be read-only and flushed at this
> point; I believe new_l2_offset might be cached too, so it needs to be
> updated.
One problem I see with this is that while we wouldn't pollute the cache
we'd always be reading the table twice from disk in all cases:
1) Read old table
2) Write new table
3) Read new table (after l2_allocate(), using the cache this time)
We can of course improve it by reading the old table from disk but
directly in the cache -so we'd spare step (3)-, but we'd still have to
read at least once from disk.
With the old code (especially if slice_size == cluster_size) we don't
need to read anything if the L2 table is already cached:
1) Get empty table from the cache
2) memcpy() the old data
3) Get new table from the cache (after l2_allocate()).
Berto
