[Qemu-devel] [PATCH] qcow2: Drop synchronous qcow_write()

[Qemu-devel] [PATCH] qcow2: Drop synchronous qcow_write()

[Kevin Wolf]

There is only one (internal) user left and it can be switched to the normal
emulation provided in block.c. Leaving it as it is would mean that creating
compressed images uses a different codepath than everything else, so there
would be a risk of a breakage that wouldn't be covered by much testing.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block-qcow2.c |   42 +-----------------------------------------
 1 files changed, 1 insertions(+), 41 deletions(-)

diff --git a/block-qcow2.c b/block-qcow2.c
index 9a49777..d674501 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1226,46 +1226,6 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static int qcow_write(BlockDriverState *bs, int64_t sector_num,
-                     const uint8_t *buf, int nb_sectors)
-{
-    BDRVQcowState *s = bs->opaque;
-    int ret, index_in_cluster, n;
-    uint64_t cluster_offset;
-    int n_end;
-    QCowL2Meta l2meta;
-
-    while (nb_sectors > 0) {
-        index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n_end = index_in_cluster + nb_sectors;
-        if (s->crypt_method &&
-            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
-            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
-        cluster_offset = alloc_cluster_offset(bs, sector_num << 9,
-                                              index_in_cluster,
-                                              n_end, &n, &l2meta);
-        if (!cluster_offset)
-            return -1;
-        if (s->crypt_method) {
-            encrypt_sectors(s, sector_num, s->cluster_data, buf, n, 1,
-                            &s->aes_encrypt_key);
-            ret = bdrv_pwrite(s->hd, cluster_offset + index_in_cluster * 512,
-                              s->cluster_data, n * 512);
-        } else {
-            ret = bdrv_pwrite(s->hd, cluster_offset + index_in_cluster * 512, buf, n * 512);
-        }
-        if (ret != n * 512 || alloc_cluster_link_l2(bs, cluster_offset, &l2meta) < 0) {
-            free_any_clusters(bs, cluster_offset, l2meta.nb_clusters);
-            return -1;
-        }
-        nb_sectors -= n;
-        sector_num += n;
-        buf += n * 512;
-    }
-    s->cluster_cache_offset = -1; /* disable compressed cache */
-    return 0;
-}
-
 typedef struct QCowAIOCB {
     BlockDriverAIOCB common;
     int64_t sector_num;
@@ -1774,7 +1734,7 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
-        qcow_write(bs, sector_num, buf, s->cluster_sectors);
+        bdrv_write(bs, sector_num, buf, s->cluster_sectors);
     } else {
         cluster_offset = alloc_compressed_cluster_offset(bs, sector_num << 9,
                                               out_len);
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qcow2: Drop synchronous qcow_write()

[Christoph Hellwig]

On Wed, May 13, 2009 at 12:46:33PM +0200, Kevin Wolf wrote:
> There is only one (internal) user left and it can be switched to the normal
> emulation provided in block.c. Leaving it as it is would mean that creating
> compressed images uses a different codepath than everything else, so there
> would be a risk of a breakage that wouldn't be covered by much testing.

Looks good to me, that strary qcow2_write irked me, too.  Do you plan
to kill qcow_read, too?

Also the same patch should into qcow, too.

Re: [Qemu-devel] [PATCH] qcow2: Drop synchronous qcow_write()

[Kevin Wolf]

Christoph Hellwig schrieb:
> Looks good to me, that strary qcow2_write irked me, too.  Do you plan
> to kill qcow_read, too?

Hm, haven't planned that because qcow_read() is used in a much more
common code path, but in theory the very same approach should work there
(and someone seems to have done this already for qcow1?).

Don't know how the emulation behaves performance wise though,
qcow_read() is used for each cluster allocation. I guess the difference
is negligible?

> Also the same patch should into qcow, too.

Right, this looks the same.

Kevin

Re: [Qemu-devel] [PATCH] qcow2: Drop synchronous qcow_write()

[Christoph Hellwig]

On Wed, May 13, 2009 at 01:08:38PM +0200, Kevin Wolf wrote:
> Christoph Hellwig schrieb:
> > Looks good to me, that strary qcow2_write irked me, too.  Do you plan
> > to kill qcow_read, too?
> 
> Hm, haven't planned that because qcow_read() is used in a much more
> common code path, but in theory the very same approach should work there
> (and someone seems to have done this already for qcow1?).
> 
> Don't know how the emulation behaves performance wise though,
> qcow_read() is used for each cluster allocation. I guess the difference
> is negligible?

Good question.  Probably needs some benchmarking first.