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 <address@hidden> 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 <address@hidden>
> ---
>  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
>
>
>