Re: [Qemu-devel] [PATCH v3 17/39] qcow2: Update l2_allocate() to support L2 slices

[Alberto Garcia]

On Thu 01 Feb 2018 07:22:16 PM CET, Max Reitz wrote:
> On 2018-02-01 16:43, Alberto Garcia wrote:
>> 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()).
>
> Well, then scratch the bdrv_pwrite() for the new table and keep using
> the cache for that (because that actually sounds useful).
>
> On second thought, though, it's rather probable the old L2 table is
> already in the cache...  Before the guest does a write to a location,
> it is reasonable to assume it has read from there before.
>
> So I guess we could think about adding a parameter to qcow2_cache_put()
> or something to reset the LRU counter because we probably won't need
> that entry anymore.  But not something for this series, of course.

That actually doesn't sound like a bad idea, there are maybe more cases
in which we know we're unlikely to need a cache entry soon, but as you
say let's take a look at it after this series.

Berto