[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v3 17/39] qcow2: Update l2_allocate

From: Alberto Garcia
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 17/39] qcow2: Update l2_allocate() to support L2 slices
Date: Thu, 01 Feb 2018 16:43:45 +0100
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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()).


reply via email to

[Prev in Thread] Current Thread [Next in Thread]