[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: Anton Nefedov
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 17/39] qcow2: Update l2_allocate() to support L2 slices
Date: Thu, 1 Feb 2018 18:23:09 +0300
User-agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

On 1/2/2018 4:13 PM, Alberto Garcia wrote:
On Wed 31 Jan 2018 09:07:48 PM CET, Max Reitz wrote:
On 2018-01-26 15:59, Alberto Garcia wrote:
This patch updates l2_allocate() to support the qcow2 cache returning
L2 slices instead of full L2 tables.

The old code simply gets an L2 table from the cache and initializes it
with zeroes or with the contents of an existing table. With a cache
that returns slices instead of tables the idea remains the same, but
the code must now iterate over all the slices that are contained in an
L2 table.

Since now we're operating with slices the function can no longer
return the newly-allocated table, so it's up to the caller to retrieve
the appropriate L2 slice after calling l2_allocate() (note that with
this patch the caller is still loading full L2 tables, but we'll deal
with that in a separate patch).

Signed-off-by: Alberto Garcia <address@hidden>
  block/qcow2-cluster.c | 56 +++++++++++++++++++++++++++++++--------------------
  1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 57349928a9..2a53c1dc5f 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -264,11 +264,12 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int 
-static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
+static int l2_allocate(BlockDriverState *bs, int l1_index)
      BDRVQcow2State *s = bs->opaque;
      uint64_t old_l2_offset;
-    uint64_t *l2_table = NULL;
+    uint64_t *l2_slice = NULL;
+    unsigned slice, slice_size2, n_slices;

I'd personally prefer size_t, but oh well.

I don't see any reason not to use int / unsigned. The size of a slice is
always <= cluster_size (an int), and both slice and n_slices are smaller
than that.

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);



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

reply via email to

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