| Message ID | 1279297056-13392-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 > > >
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
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);
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(-)