[Qemu-devel] [PATCH 0/3] qcow2: Write complete sectors

[Qemu-devel] [PATCH 0/3] qcow2: Write complete sectors

[Kevin Wolf]

Once upon a time, there was a bdrv_pwrite that actually wrote single bytes to
the file... However, today it is emulated by a read-modify-write cycle which
aligns the request to sector size. This is slow. And we don't need it: qcow2
often has the complete sector in memory, we don't need to read it from the disk
again.

These patches change the writes to L1 tables, L2 tables and refcount blocks to
write complete sectors instead of single entries.

This series depends on the qcow2 split to apply cleanly.

Kevin Wolf (3):
  l2_allocate: Write complete sectors
  alloc_cluster_link_l2: Write complete sectors
  update_refcount: Write complete sectors

 block/qcow2-cluster.c  |   61 +++++++++++++++++++++++++++++++++++++++++------
 block/qcow2-refcount.c |   34 ++++++++++++++++++++------
 2 files changed, 79 insertions(+), 16 deletions(-)

[Qemu-devel] [PATCH 1/3] l2_allocate: Write complete sectors

[Kevin Wolf]

When modifying the L1 table, l2_allocate() needs to write complete sectors
instead of single entries. The L1 table is already in memory, reading it from
disk in the block layer to align the request is wasted performance.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 99215fa..1c70693 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -171,6 +171,31 @@ static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
 }
 
 /*
+ * Writes one sector of the L1 table to the disk (can't update single entries
+ * and we really don't want bdrv_pread to perform a read-modify-write)
+ */
+#define L1_ENTRIES_PER_SECTOR (512 / 8)
+static int write_l1_entry(BDRVQcowState *s, int l1_index)
+{
+    uint64_t buf[L1_ENTRIES_PER_SECTOR];
+    int l1_start_index;
+    int i;
+
+    l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);
+    for (i = 0; i < L1_ENTRIES_PER_SECTOR; i++) {
+        buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
+    }
+
+    if (bdrv_pwrite(s->hd, s->l1_table_offset + 8 * l1_start_index,
+        buf, sizeof(buf)) != sizeof(buf))
+    {
+        return -1;
+    }
+
+    return 0;
+}
+
+/*
  * l2_allocate
  *
  * Allocate a new l2 entry in the file. If l1_index points to an already
@@ -184,7 +209,7 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
 {
     BDRVQcowState *s = bs->opaque;
     int min_index;
-    uint64_t old_l2_offset, tmp;
+    uint64_t old_l2_offset;
     uint64_t *l2_table, l2_offset;
 
     old_l2_offset = s->l1_table[l1_index];
@@ -196,11 +221,9 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
     /* update the L1 entry */
 
     s->l1_table[l1_index] = l2_offset | QCOW_OFLAG_COPIED;
-
-    tmp = cpu_to_be64(l2_offset | QCOW_OFLAG_COPIED);
-    if (bdrv_pwrite(s->hd, s->l1_table_offset + l1_index * sizeof(tmp),
-                    &tmp, sizeof(tmp)) != sizeof(tmp))
+    if (write_l1_entry(s, l1_index) < 0) {
         return NULL;
+    }
 
     /* allocate a new entry in the l2 cache */
 
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/3] alloc_cluster_link_l2: Write complete sectors

[Kevin Wolf]

When updating the L2 tables in alloc_cluster_link_l2(), write complete
sectors instead of updating single entries.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1c70693..d349655 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -578,6 +578,28 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
     return cluster_offset;
 }
 
+/*
+ * Write L2 table updates to disk, writing whole sectors to avoid a
+ * read-modify-write in bdrv_pwrite
+ */
+#define L2_ENTRIES_PER_SECTOR (512 / 8)
+static int write_l2_entries(BDRVQcowState *s, uint64_t *l2_table,
+    uint64_t l2_offset, int l2_index, int num)
+{
+    int l2_start_index = l2_index & ~(L1_ENTRIES_PER_SECTOR - 1);
+    int start_offset = (8 * l2_index) & ~511;
+    int end_offset = (8 * (l2_index + num) + 511) & ~511;
+    size_t len = end_offset - start_offset;
+
+    if (bdrv_pwrite(s->hd, l2_offset + start_offset, &l2_table[l2_start_index],
+        len) != len)
+    {
+        return -1;
+    }
+
+    return 0;
+}
+
 int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
     QCowL2Meta *m)
 {
@@ -625,10 +647,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
                     (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
      }
 
-    if (bdrv_pwrite(s->hd, l2_offset + l2_index * sizeof(uint64_t),
-                l2_table + l2_index, m->nb_clusters * sizeof(uint64_t)) !=
-            m->nb_clusters * sizeof(uint64_t))
+    if (write_l2_entries(s, l2_table, l2_offset, l2_index, m->nb_clusters) < 0) {
+        ret = -1;
         goto err;
+    }
 
     for (i = 0; i < j; i++)
         qcow2_free_any_clusters(bs,
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/3] update_refcount: Write complete sectors

[Kevin Wolf]

When updating the refcount blocks in update_refcount(), write complete sectors
instead of updating single entries.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b38390c..dd6e293 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -209,6 +209,27 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
     return refcount_block_offset;
 }
 
+#define REFCOUNTS_PER_SECTOR (512 >> REFCOUNT_SHIFT)
+static int write_refcount_block_entries(BDRVQcowState *s,
+    int64_t refcount_block_offset, int first_index, int last_index)
+{
+    size_t size;
+
+    first_index &= ~(REFCOUNTS_PER_SECTOR - 1);
+    last_index = (last_index + REFCOUNTS_PER_SECTOR)
+        & ~(REFCOUNTS_PER_SECTOR - 1);
+
+    size = (last_index - first_index) << REFCOUNT_SHIFT;
+    if (bdrv_pwrite(s->hd,
+        refcount_block_offset + (first_index << REFCOUNT_SHIFT),
+        &s->refcount_block_cache[first_index], size) != size)
+    {
+        return -EIO;
+    }
+
+    return 0;
+}
+
 /* XXX: cache several refcount block clusters ? */
 static int update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
@@ -238,10 +259,9 @@ static int update_refcount(BlockDriverState *bs,
         old_table_index = table_index;
         table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
         if ((old_table_index >= 0) && (table_index != old_table_index)) {
-            size_t size = (last_index - first_index + 1) << REFCOUNT_SHIFT;
-            if (bdrv_pwrite(s->hd,
-                refcount_block_offset + (first_index << REFCOUNT_SHIFT),
-                &s->refcount_block_cache[first_index], size) != size)
+
+            if (write_refcount_block_entries(s, refcount_block_offset,
+                first_index, last_index) < 0)
             {
                 return -EIO;
             }
@@ -278,10 +298,8 @@ static int update_refcount(BlockDriverState *bs,
 
     /* Write last changed block to disk */
     if (refcount_block_offset != 0) {
-        size_t size = (last_index - first_index + 1) << REFCOUNT_SHIFT;
-        if (bdrv_pwrite(s->hd,
-            refcount_block_offset + (first_index << REFCOUNT_SHIFT),
-            &s->refcount_block_cache[first_index], size) != size)
+        if (write_refcount_block_entries(s, refcount_block_offset,
+            first_index, last_index) < 0)
         {
             return -EIO;
         }
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH 0/3] qcow2: Write complete sectors

[Dor Laor]

On 06/16/2009 12:31 PM, Kevin Wolf wrote:
> Once upon a time, there was a bdrv_pwrite that actually wrote single bytes to
> the file... However, today it is emulated by a read-modify-write cycle which
> aligns the request to sector size. This is slow. And we don't need it: qcow2
> often has the complete sector in memory, we don't need to read it from the disk
> again.
>
> These patches change the writes to L1 tables, L2 tables and refcount blocks to
> write complete sectors instead of single entries.
>
> This series depends on the qcow2 split to apply cleanly.
>
>    

If it's not an RFC, you better prepare a git tree for Anthony to pull from
and described how did you test it. Maybe committing the qemu-io scripts
would also be a move in the right direction.
Cheers,
Dor

> Kevin Wolf (3):
>    l2_allocate: Write complete sectors
>    alloc_cluster_link_l2: Write complete sectors
>    update_refcount: Write complete sectors
>
>   block/qcow2-cluster.c  |   61 +++++++++++++++++++++++++++++++++++++++++------
>   block/qcow2-refcount.c |   34 ++++++++++++++++++++------
>   2 files changed, 79 insertions(+), 16 deletions(-)
>
>
>
>    

Re: [Qemu-devel] [PATCH 0/3] qcow2: Write complete sectors

[Avi Kivity]

On 06/16/2009 12:31 PM, Kevin Wolf wrote:
> Once upon a time, there was a bdrv_pwrite that actually wrote single bytes to
> the file... However, today it is emulated by a read-modify-write cycle which
> aligns the request to sector size. This is slow. And we don't need it: qcow2
> often has the complete sector in memory, we don't need to read it from the disk
> again.
>
> These patches change the writes to L1 tables, L2 tables and refcount blocks to
> write complete sectors instead of single entries.
>
> This series depends on the qcow2 split to apply cleanly.
>
>    

Patches look good to me.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/3] qcow2: Write complete sectors

[Kevin Wolf]

Dor Laor schrieb:
> On 06/16/2009 12:31 PM, Kevin Wolf wrote:
>> Once upon a time, there was a bdrv_pwrite that actually wrote single bytes to
>> the file... However, today it is emulated by a read-modify-write cycle which
>> aligns the request to sector size. This is slow. And we don't need it: qcow2
>> often has the complete sector in memory, we don't need to read it from the disk
>> again.
>>
>> These patches change the writes to L1 tables, L2 tables and refcount blocks to
>> write complete sectors instead of single entries.
>>
>> This series depends on the qcow2 split to apply cleanly.
>>
>>    
> 
> If it's not an RFC, you better prepare a git tree for Anthony to pull from

I can do that, but the patches need to be on the list anyway.

> and described how did you test it. Maybe committing the qemu-io scripts
> would also be a move in the right direction.

Anthony is promising for a while now that he will push his tests to the
tree. Once he has done so, I'll consider integrating my script.

Kevin