[Qemu-devel] [PATCH] block: Change bdrv_commit to handle multiple sectors at once

[Qemu-devel] [PATCH] block: Change bdrv_commit to handle multiple sectors at once

[Kevin Wolf]

bdrv_commit copies the image to its backing file sector by sector, which
is (surprise!) relatively slow. Let's take a larger buffer and handle more
sectors at once if possible.

With a 1G qcow2 file, this brought the time bdrv_commit takes down from
5:06 min to 1:14 min for me.

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

diff --git a/block.c b/block.c
index 65cf4dc..12dbc43 100644
--- a/block.c
+++ b/block.c
@@ -729,9 +729,9 @@ int bdrv_commit(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
     int64_t i, total_sectors;
-    int n, j, ro, open_flags;
+    int n, ro, open_flags;
     int ret = 0, rw_ret = 0;
-    unsigned char sector[BDRV_SECTOR_SIZE];
+    uint8_t *buf;
     char filename[1024];
     BlockDriverState *bs_rw, *bs_ro;
 
@@ -774,25 +774,26 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+    buf = qemu_malloc(2048 * BDRV_SECTOR_SIZE);
+
     for (i = 0; i < total_sectors;) {
-        if (drv->bdrv_is_allocated(bs, i, 65536, &n)) {
-            for(j = 0; j < n; j++) {
-                if (bdrv_read(bs, i, sector, 1) != 0) {
-                    ret = -EIO;
-                    goto ro_cleanup;
-                }
+        if (drv->bdrv_is_allocated(bs, i, 2048, &n)) {
 
-                if (bdrv_write(bs->backing_hd, i, sector, 1) != 0) {
-                    ret = -EIO;
-                    goto ro_cleanup;
-                }
-                i++;
-	    }
-	} else {
-            i += n;
+            if (bdrv_read(bs, i, buf, n) != 0) {
+                ret = -EIO;
+                goto ro_cleanup;
+            }
+
+            if (bdrv_write(bs->backing_hd, i, buf, n) != 0) {
+                ret = -EIO;
+                goto ro_cleanup;
+            }
         }
+        i += n;
     }
 
+    qemu_free(buf);
+
     if (drv->bdrv_make_empty) {
         ret = drv->bdrv_make_empty(bs);
         bdrv_flush(bs);
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH] block: Change bdrv_commit to handle multiple sectors at once

[Christoph Hellwig]

On Fri, Jul 16, 2010 at 06:17:36PM +0200, Kevin Wolf wrote:
> +    buf = qemu_malloc(2048 * BDRV_SECTOR_SIZE);

Please add a COMMIT_BUF_SIZE #define instead of the hardcoded 2048 in
various places.

>      for (i = 0; i < total_sectors;) {
> +        if (drv->bdrv_is_allocated(bs, i, 2048, &n)) {
>  
> +            if (bdrv_read(bs, i, buf, n) != 0) {
> +                ret = -EIO;
> +                goto ro_cleanup;
> +            }
> +
> +            if (bdrv_write(bs->backing_hd, i, buf, n) != 0) {
> +                ret = -EIO;
> +                goto ro_cleanup;
> +            }
>          }
> +        i += n;

Maybe it's just me, but I'd prefer n getting a more descriptive name
(e.g. sector) and moving the increment of it into the for loop, e.g.

	for (sector = 0; sector < total_sectors; sector += n) {
		if (!drv->bdrv_is_allocated(bs, i, 2048, &n))
			continue;
		...
	}

Otherwise this looks good to me.

Re: [Qemu-devel] [PATCH] block: Change bdrv_commit to handle multiple sectors at once

[Kevin Wolf]

Am 16.07.2010 20:16, schrieb Christoph Hellwig:
> On Fri, Jul 16, 2010 at 06:17:36PM +0200, Kevin Wolf wrote:
>> +    buf = qemu_malloc(2048 * BDRV_SECTOR_SIZE);
> 
> Please add a COMMIT_BUF_SIZE #define instead of the hardcoded 2048 in
> various places.
> 
>>      for (i = 0; i < total_sectors;) {
>> +        if (drv->bdrv_is_allocated(bs, i, 2048, &n)) {
>>  
>> +            if (bdrv_read(bs, i, buf, n) != 0) {
>> +                ret = -EIO;
>> +                goto ro_cleanup;
>> +            }
>> +
>> +            if (bdrv_write(bs->backing_hd, i, buf, n) != 0) {
>> +                ret = -EIO;
>> +                goto ro_cleanup;
>> +            }
>>          }
>> +        i += n;
> 
> Maybe it's just me, but I'd prefer n getting a more descriptive name
> (e.g. sector) and moving the increment of it into the for loop, e.g.
> 
> 	for (sector = 0; sector < total_sectors; sector += n) {
> 		if (!drv->bdrv_is_allocated(bs, i, 2048, &n))
> 			continue;
> 		...
> 	}

So you mean i should be renamed, not n, right? I'll change these points
in v2.

Kevin

Re: [Qemu-devel] [PATCH] block: Change bdrv_commit to handle multiple sectors at once

[Stefan Hajnoczi]

On Fri, Jul 16, 2010 at 5:17 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> bdrv_commit copies the image to its backing file sector by sector, which
> is (surprise!) relatively slow. Let's take a larger buffer and handle more
> sectors at once if possible.
>
> With a 1G qcow2 file, this brought the time bdrv_commit takes down from
> 5:06 min to 1:14 min for me.

Nice performance improvement!

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c |   35 ++++++++++++++++++-----------------
>  1 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/block.c b/block.c
> index 65cf4dc..12dbc43 100644
> --- a/block.c
> +++ b/block.c
> @@ -729,9 +729,9 @@ int bdrv_commit(BlockDriverState *bs)
>  {
>     BlockDriver *drv = bs->drv;
>     int64_t i, total_sectors;
> -    int n, j, ro, open_flags;
> +    int n, ro, open_flags;
>     int ret = 0, rw_ret = 0;
> -    unsigned char sector[BDRV_SECTOR_SIZE];
> +    uint8_t *buf;
>     char filename[1024];
>     BlockDriverState *bs_rw, *bs_ro;
>
> @@ -774,25 +774,26 @@ int bdrv_commit(BlockDriverState *bs)
>     }
>
>     total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +    buf = qemu_malloc(2048 * BDRV_SECTOR_SIZE);
> +
>     for (i = 0; i < total_sectors;) {
> -        if (drv->bdrv_is_allocated(bs, i, 65536, &n)) {
> -            for(j = 0; j < n; j++) {
> -                if (bdrv_read(bs, i, sector, 1) != 0) {
> -                    ret = -EIO;
> -                    goto ro_cleanup;
> -                }
> +        if (drv->bdrv_is_allocated(bs, i, 2048, &n)) {
>
> -                if (bdrv_write(bs->backing_hd, i, sector, 1) != 0) {
> -                    ret = -EIO;
> -                    goto ro_cleanup;
> -                }
> -                i++;
> -           }
> -       } else {
> -            i += n;
> +            if (bdrv_read(bs, i, buf, n) != 0) {
> +                ret = -EIO;
> +                goto ro_cleanup;
> +            }
> +
> +            if (bdrv_write(bs->backing_hd, i, buf, n) != 0) {
> +                ret = -EIO;
> +                goto ro_cleanup;
> +            }
>         }
> +        i += n;
>     }
>
> +    qemu_free(buf);

Is buf being freed in the error case ro_cleanup label?

Stefan

> +
>     if (drv->bdrv_make_empty) {
>         ret = drv->bdrv_make_empty(bs);
>         bdrv_flush(bs);
> --
> 1.7.1.1
>
>
>