Message ID | 20190523154444.28783-3-shmuel.eiderman@oracle.com |
---|---|
State | New |
Headers | show |
Series | qemu-img: rebase: Improve/optimize rebase operation | expand |
On 23.05.19 17:44, Sam Eiderman wrote: > In the following case: > > (base) A <- B <- C (tip) > > when running: > > qemu-img rebase -b A C > > QEMU would read all sectors not allocated in the file being rebased (C) > and compare them to the new base image (A), regardless of whether they > were changed or even allocated anywhere along the chain between the new > base and the top image (B). This causes many unneeded reads when > rebasing an image which represents a small diff of a large disk, as it > would read most of the disk's sectors. > > Instead, use bdrv_is_allocated_above() to reduce the number of > unnecessary reads. > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com> > Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com> > --- > qemu-img.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 9bd0bb1e47..e6fd8e1a98 100644 > --- a/qemu-img.c > +++ b/qemu-img.c [...] > @@ -3437,6 +3443,23 @@ static int img_rebase(int argc, char **argv) > continue; > } > > + if (prefix_chain_bs) { > + /* > + * If cluster wasn't changed since prefix_chain, we don't need > + * to take action > + */ > + ret = bdrv_is_allocated_above(bs, prefix_chain_bs, > + offset, n, &n); I think you forgot to update the patch...? Max > + if (ret < 0) { > + error_report("error while reading image metadata: %s", > + strerror(-ret)); > + goto out; > + } > + if (!ret) { > + continue; > + } > + } > + > /* > * Read old and new backing file and take into consideration that > * backing files may be smaller than the COW image. >
Yes, my bad, resending as v5. Sam > On 23 May 2019, at 19:14, Max Reitz <mreitz@redhat.com> wrote: > > On 23.05.19 17:44, Sam Eiderman wrote: >> In the following case: >> >> (base) A <- B <- C (tip) >> >> when running: >> >> qemu-img rebase -b A C >> >> QEMU would read all sectors not allocated in the file being rebased (C) >> and compare them to the new base image (A), regardless of whether they >> were changed or even allocated anywhere along the chain between the new >> base and the top image (B). This causes many unneeded reads when >> rebasing an image which represents a small diff of a large disk, as it >> would read most of the disk's sectors. >> >> Instead, use bdrv_is_allocated_above() to reduce the number of >> unnecessary reads. >> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >> Signed-off-by: Sam Eiderman <shmuel.eiderman@oracle.com> >> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com> >> --- >> qemu-img.c | 25 ++++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 9bd0bb1e47..e6fd8e1a98 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c > > [...] > >> @@ -3437,6 +3443,23 @@ static int img_rebase(int argc, char **argv) >> continue; >> } >> >> + if (prefix_chain_bs) { >> + /* >> + * If cluster wasn't changed since prefix_chain, we don't need >> + * to take action >> + */ >> + ret = bdrv_is_allocated_above(bs, prefix_chain_bs, >> + offset, n, &n); > > I think you forgot to update the patch...? > > Max > >> + if (ret < 0) { >> + error_report("error while reading image metadata: %s", >> + strerror(-ret)); >> + goto out; >> + } >> + if (!ret) { >> + continue; >> + } >> + } >> + >> /* >> * Read old and new backing file and take into consideration that >> * backing files may be smaller than the COW image.
diff --git a/qemu-img.c b/qemu-img.c index 9bd0bb1e47..e6fd8e1a98 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3164,7 +3164,7 @@ static int img_rebase(int argc, char **argv) BlockBackend *blk = NULL, *blk_old_backing = NULL, *blk_new_backing = NULL; uint8_t *buf_old = NULL; uint8_t *buf_new = NULL; - BlockDriverState *bs = NULL; + BlockDriverState *bs = NULL, *prefix_chain_bs = NULL; char *filename; const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg; int c, flags, src_flags, ret; @@ -3353,6 +3353,12 @@ static int img_rebase(int argc, char **argv) goto out; } + /* + * Find out whether we rebase an image on top of a previous image + * in its chain. + */ + prefix_chain_bs = bdrv_find_backing_image(bs, out_real_path); + blk_new_backing = blk_new_open(out_real_path, NULL, options, src_flags, &local_err); g_free(out_real_path); @@ -3437,6 +3443,23 @@ static int img_rebase(int argc, char **argv) continue; } + if (prefix_chain_bs) { + /* + * If cluster wasn't changed since prefix_chain, we don't need + * to take action + */ + ret = bdrv_is_allocated_above(bs, prefix_chain_bs, + offset, n, &n); + if (ret < 0) { + error_report("error while reading image metadata: %s", + strerror(-ret)); + goto out; + } + if (!ret) { + continue; + } + } + /* * Read old and new backing file and take into consideration that * backing files may be smaller than the COW image.