| Message ID | 20190523154444.28783-3-shmuel.eiderman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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.
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(-)