| Message ID | 1397329060-23599-5-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: > Implement progress output for the commit command by querying the > progress of the block job. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > qemu-img.texi | 2 +- > 3 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > index d029609..8bc55cd 100644 > --- a/qemu-img-cmds.hx > +++ b/qemu-img-cmds.hx > @@ -22,9 +22,9 @@ STEXI > ETEXI > > DEF("commit", img_commit, > - "commit [-q] [-f fmt] [-t cache] filename") > + "commit [-q] [-f fmt] [-t cache] [-p] filename") > STEXI > -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} > +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} > ETEXI > > DEF("compare", img_compare, > diff --git a/qemu-img.c b/qemu-img.c > index 9fe6384..50d7519 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -686,6 +686,9 @@ fail: > struct CommonBlockJobCBInfo { > Error **errp; > bool done; > + /* If the blockjob works on several sectors at once, this field has to > + * contain that granularity in bytes */ > + uint64_t granularity; > }; > > static void common_block_job_cb(void *opaque, int ret) > @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, int ret) > static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) > { > BlockJobInfo *info; > + uint64_t mod_offset = 0; > > do { > qemu_aio_wait(); > > info = block_job_query(job); > > + if (info->offset) { > + if (!mod_offset) { > + /* Some block jobs (at least "commit") will only work on a > + * subset of the image file and therefore basically skip many > + * sectors at the start (processing them apparently > + * instantaneously). These sectors should be ignored when > + * calculating the progress. > + * If the blockjob processes several sectors at once, the first > + * progress report is likely to come after one such sector group > + * (whose size is cbi->granularity), therefore subtract it from > + * the current offset in order to obtain a more probable > + * starting offset. */ > + mod_offset = info->offset > cbi->granularity > + ? info->offset - cbi->granularity > + : 0; granularity != buf_size. And you still can't distinguish whether the first chunk was skipped or copied. In the v2 review I suggested changing mirror_run() to update s->common.len. There you wouldn't have to make any assumptions, but would know exactly where the bitmap initialisation is done. And it would improve traditional block job progress output, too. Am I missing anything that makes this approach harder than it seems? Kevin
On 16.04.2014 17:00, Kevin Wolf wrote: > Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: >> Implement progress output for the commit command by querying the >> progress of the block job. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- >> qemu-img.texi | 2 +- >> 3 files changed, 45 insertions(+), 5 deletions(-) >> >> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx >> index d029609..8bc55cd 100644 >> --- a/qemu-img-cmds.hx >> +++ b/qemu-img-cmds.hx >> @@ -22,9 +22,9 @@ STEXI >> ETEXI >> >> DEF("commit", img_commit, >> - "commit [-q] [-f fmt] [-t cache] filename") >> + "commit [-q] [-f fmt] [-t cache] [-p] filename") >> STEXI >> -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} >> +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} >> ETEXI >> >> DEF("compare", img_compare, >> diff --git a/qemu-img.c b/qemu-img.c >> index 9fe6384..50d7519 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -686,6 +686,9 @@ fail: >> struct CommonBlockJobCBInfo { >> Error **errp; >> bool done; >> + /* If the blockjob works on several sectors at once, this field has to >> + * contain that granularity in bytes */ >> + uint64_t granularity; >> }; >> >> static void common_block_job_cb(void *opaque, int ret) >> @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, int ret) >> static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) >> { >> BlockJobInfo *info; >> + uint64_t mod_offset = 0; >> >> do { >> qemu_aio_wait(); >> >> info = block_job_query(job); >> >> + if (info->offset) { >> + if (!mod_offset) { >> + /* Some block jobs (at least "commit") will only work on a >> + * subset of the image file and therefore basically skip many >> + * sectors at the start (processing them apparently >> + * instantaneously). These sectors should be ignored when >> + * calculating the progress. >> + * If the blockjob processes several sectors at once, the first >> + * progress report is likely to come after one such sector group >> + * (whose size is cbi->granularity), therefore subtract it from >> + * the current offset in order to obtain a more probable >> + * starting offset. */ >> + mod_offset = info->offset > cbi->granularity >> + ? info->offset - cbi->granularity >> + : 0; > granularity != buf_size. And you still can't distinguish whether the > first chunk was skipped or copied. > > In the v2 review I suggested changing mirror_run() to update > s->common.len. There you wouldn't have to make any assumptions, but > would know exactly where the bitmap initialisation is done. And it would > improve traditional block job progress output, too. > > Am I missing anything that makes this approach harder than it seems? I just don't like it, somehow, that's all. But I'll see what I can do. Max
On 16.04.2014 23:48, Max Reitz wrote: > On 16.04.2014 17:00, Kevin Wolf wrote: >> Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: >>> Implement progress output for the commit command by querying the >>> progress of the block job. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> qemu-img-cmds.hx | 4 ++-- >>> qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- >>> qemu-img.texi | 2 +- >>> 3 files changed, 45 insertions(+), 5 deletions(-) >>> >>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx >>> index d029609..8bc55cd 100644 >>> --- a/qemu-img-cmds.hx >>> +++ b/qemu-img-cmds.hx >>> @@ -22,9 +22,9 @@ STEXI >>> ETEXI >>> DEF("commit", img_commit, >>> - "commit [-q] [-f fmt] [-t cache] filename") >>> + "commit [-q] [-f fmt] [-t cache] [-p] filename") >>> STEXI >>> -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} >>> +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} >>> ETEXI >>> DEF("compare", img_compare, >>> diff --git a/qemu-img.c b/qemu-img.c >>> index 9fe6384..50d7519 100644 >>> --- a/qemu-img.c >>> +++ b/qemu-img.c >>> @@ -686,6 +686,9 @@ fail: >>> struct CommonBlockJobCBInfo { >>> Error **errp; >>> bool done; >>> + /* If the blockjob works on several sectors at once, this field >>> has to >>> + * contain that granularity in bytes */ >>> + uint64_t granularity; >>> }; >>> static void common_block_job_cb(void *opaque, int ret) >>> @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, >>> int ret) >>> static void run_block_job(BlockJob *job, struct >>> CommonBlockJobCBInfo *cbi) >>> { >>> BlockJobInfo *info; >>> + uint64_t mod_offset = 0; >>> do { >>> qemu_aio_wait(); >>> info = block_job_query(job); >>> + if (info->offset) { >>> + if (!mod_offset) { >>> + /* Some block jobs (at least "commit") will only >>> work on a >>> + * subset of the image file and therefore basically >>> skip many >>> + * sectors at the start (processing them apparently >>> + * instantaneously). These sectors should be >>> ignored when >>> + * calculating the progress. >>> + * If the blockjob processes several sectors at >>> once, the first >>> + * progress report is likely to come after one such >>> sector group >>> + * (whose size is cbi->granularity), therefore >>> subtract it from >>> + * the current offset in order to obtain a more >>> probable >>> + * starting offset. */ >>> + mod_offset = info->offset > cbi->granularity >>> + ? info->offset - cbi->granularity >>> + : 0; >> granularity != buf_size. And you still can't distinguish whether the >> first chunk was skipped or copied. >> >> In the v2 review I suggested changing mirror_run() to update >> s->common.len. There you wouldn't have to make any assumptions, but >> would know exactly where the bitmap initialisation is done. And it would >> improve traditional block job progress output, too. >> >> Am I missing anything that makes this approach harder than it seems? > > I just don't like it, somehow, that's all. But I'll see what I can do. Okay, now I have a better reason than "Meh, I don't like it" (which is always a very bad reason, of course), being the following: As mirror_run() is actually made for mirroring from an active block device, some sectors may be marked dirty during the block job which the initialization loop did not consider dirty. This will not occur in the case of qemu-img commit, obviously, as the block device is not really used. However, mirror_run() isn't made for use by qemu-img only. If new sectors are marked dirty during the block job's operation, we'd have to increase the length of the block job live which seems very crude to me. Of course, I'd love to be proven wrong, as I do see that the above solution (taking the granularity into account) is pretty crude as well. Max
Am 17.04.2014 um 00:53 hat Max Reitz geschrieben: > On 16.04.2014 23:48, Max Reitz wrote: > >On 16.04.2014 17:00, Kevin Wolf wrote: > >>Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: > >>>Implement progress output for the commit command by querying the > >>>progress of the block job. > >>> > >>>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>>--- > >>> qemu-img-cmds.hx | 4 ++-- > >>> qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > >>> qemu-img.texi | 2 +- > >>> 3 files changed, 45 insertions(+), 5 deletions(-) > >>> > >>>diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > >>>index d029609..8bc55cd 100644 > >>>--- a/qemu-img-cmds.hx > >>>+++ b/qemu-img-cmds.hx > >>>@@ -22,9 +22,9 @@ STEXI > >>> ETEXI > >>> DEF("commit", img_commit, > >>>- "commit [-q] [-f fmt] [-t cache] filename") > >>>+ "commit [-q] [-f fmt] [-t cache] [-p] filename") > >>> STEXI > >>>-@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} > >>>+@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} > >>> ETEXI > >>> DEF("compare", img_compare, > >>>diff --git a/qemu-img.c b/qemu-img.c > >>>index 9fe6384..50d7519 100644 > >>>--- a/qemu-img.c > >>>+++ b/qemu-img.c > >>>@@ -686,6 +686,9 @@ fail: > >>> struct CommonBlockJobCBInfo { > >>> Error **errp; > >>> bool done; > >>>+ /* If the blockjob works on several sectors at once, this > >>>field has to > >>>+ * contain that granularity in bytes */ > >>>+ uint64_t granularity; > >>> }; > >>> static void common_block_job_cb(void *opaque, int ret) > >>>@@ -702,12 +705,34 @@ static void common_block_job_cb(void > >>>*opaque, int ret) > >>> static void run_block_job(BlockJob *job, struct > >>>CommonBlockJobCBInfo *cbi) > >>> { > >>> BlockJobInfo *info; > >>>+ uint64_t mod_offset = 0; > >>> do { > >>> qemu_aio_wait(); > >>> info = block_job_query(job); > >>> + if (info->offset) { > >>>+ if (!mod_offset) { > >>>+ /* Some block jobs (at least "commit") will > >>>only work on a > >>>+ * subset of the image file and therefore > >>>basically skip many > >>>+ * sectors at the start (processing them apparently > >>>+ * instantaneously). These sectors should be > >>>ignored when > >>>+ * calculating the progress. > >>>+ * If the blockjob processes several sectors > >>>at once, the first > >>>+ * progress report is likely to come after > >>>one such sector group > >>>+ * (whose size is cbi->granularity), > >>>therefore subtract it from > >>>+ * the current offset in order to obtain a > >>>more probable > >>>+ * starting offset. */ > >>>+ mod_offset = info->offset > cbi->granularity > >>>+ ? info->offset - cbi->granularity > >>>+ : 0; > >>granularity != buf_size. And you still can't distinguish whether the > >>first chunk was skipped or copied. > >> > >>In the v2 review I suggested changing mirror_run() to update > >>s->common.len. There you wouldn't have to make any assumptions, but > >>would know exactly where the bitmap initialisation is done. And it would > >>improve traditional block job progress output, too. > >> > >>Am I missing anything that makes this approach harder than it seems? > > > >I just don't like it, somehow, that's all. But I'll see what I can do. > > Okay, now I have a better reason than "Meh, I don't like it" (which > is always a very bad reason, of course), being the following: As > mirror_run() is actually made for mirroring from an active block > device, some sectors may be marked dirty during the block job which > the initialization loop did not consider dirty. This will not occur > in the case of qemu-img commit, obviously, as the block device is > not really used. However, mirror_run() isn't made for use by > qemu-img only. If new sectors are marked dirty during the block > job's operation, we'd have to increase the length of the block job > live which seems very crude to me. > > Of course, I'd love to be proven wrong, as I do see that the above > solution (taking the granularity into account) is pretty crude as > well. I'm not sure if it really matters. The progress is only for the initial bulk copy anyway. It's possible that you copied a cluster and then the guest dirties it again, and the progress won't show that. So you already have some inaccuracy of that kind today. In this light, I would consider it reasonable enough to treat only the initially allocated clusters as part of that first pass. But you're right in that we need to be careful to cap the progress at the maximum we're advertising. Kevin
On Thu, 04/17 09:29, Kevin Wolf wrote: > Am 17.04.2014 um 00:53 hat Max Reitz geschrieben: > > On 16.04.2014 23:48, Max Reitz wrote: > > >On 16.04.2014 17:00, Kevin Wolf wrote: > > >>Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: > > >>>Implement progress output for the commit command by querying the > > >>>progress of the block job. > > >>> > > >>>Signed-off-by: Max Reitz <mreitz@redhat.com> > > >>>--- > > >>> qemu-img-cmds.hx | 4 ++-- > > >>> qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > > >>> qemu-img.texi | 2 +- > > >>> 3 files changed, 45 insertions(+), 5 deletions(-) > > >>> > > >>>diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > > >>>index d029609..8bc55cd 100644 > > >>>--- a/qemu-img-cmds.hx > > >>>+++ b/qemu-img-cmds.hx > > >>>@@ -22,9 +22,9 @@ STEXI > > >>> ETEXI > > >>> DEF("commit", img_commit, > > >>>- "commit [-q] [-f fmt] [-t cache] filename") > > >>>+ "commit [-q] [-f fmt] [-t cache] [-p] filename") > > >>> STEXI > > >>>-@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} > > >>>+@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} > > >>> ETEXI > > >>> DEF("compare", img_compare, > > >>>diff --git a/qemu-img.c b/qemu-img.c > > >>>index 9fe6384..50d7519 100644 > > >>>--- a/qemu-img.c > > >>>+++ b/qemu-img.c > > >>>@@ -686,6 +686,9 @@ fail: > > >>> struct CommonBlockJobCBInfo { > > >>> Error **errp; > > >>> bool done; > > >>>+ /* If the blockjob works on several sectors at once, this > > >>>field has to > > >>>+ * contain that granularity in bytes */ > > >>>+ uint64_t granularity; > > >>> }; > > >>> static void common_block_job_cb(void *opaque, int ret) > > >>>@@ -702,12 +705,34 @@ static void common_block_job_cb(void > > >>>*opaque, int ret) > > >>> static void run_block_job(BlockJob *job, struct > > >>>CommonBlockJobCBInfo *cbi) > > >>> { > > >>> BlockJobInfo *info; > > >>>+ uint64_t mod_offset = 0; > > >>> do { > > >>> qemu_aio_wait(); > > >>> info = block_job_query(job); > > >>> + if (info->offset) { > > >>>+ if (!mod_offset) { > > >>>+ /* Some block jobs (at least "commit") will > > >>>only work on a > > >>>+ * subset of the image file and therefore > > >>>basically skip many > > >>>+ * sectors at the start (processing them apparently > > >>>+ * instantaneously). These sectors should be > > >>>ignored when > > >>>+ * calculating the progress. > > >>>+ * If the blockjob processes several sectors > > >>>at once, the first > > >>>+ * progress report is likely to come after > > >>>one such sector group > > >>>+ * (whose size is cbi->granularity), > > >>>therefore subtract it from > > >>>+ * the current offset in order to obtain a > > >>>more probable > > >>>+ * starting offset. */ Don't really understand why we don't like this, the first sectors are as well part of the commit progress, isn't it? > > >>>+ mod_offset = info->offset > cbi->granularity > > >>>+ ? info->offset - cbi->granularity > > >>>+ : 0; > > >>granularity != buf_size. And you still can't distinguish whether the > > >>first chunk was skipped or copied. > > >> > > >>In the v2 review I suggested changing mirror_run() to update > > >>s->common.len. There you wouldn't have to make any assumptions, but > > >>would know exactly where the bitmap initialisation is done. And it would > > >>improve traditional block job progress output, too. > > >> > > >>Am I missing anything that makes this approach harder than it seems? > > > > > >I just don't like it, somehow, that's all. But I'll see what I can do. > > > > Okay, now I have a better reason than "Meh, I don't like it" (which > > is always a very bad reason, of course), being the following: As > > mirror_run() is actually made for mirroring from an active block > > device, some sectors may be marked dirty during the block job which > > the initialization loop did not consider dirty. This will not occur > > in the case of qemu-img commit, obviously, as the block device is > > not really used. However, mirror_run() isn't made for use by > > qemu-img only. If new sectors are marked dirty during the block > > job's operation, we'd have to increase the length of the block job > > live which seems very crude to me. > > > > Of course, I'd love to be proven wrong, as I do see that the above > > solution (taking the granularity into account) is pretty crude as > > well. > > I'm not sure if it really matters. The progress is only for the initial > bulk copy anyway. It's possible that you copied a cluster and then the > guest dirties it again, and the progress won't show that. So you already > have some inaccuracy of that kind today. In this light, I would consider > it reasonable enough to treat only the initially allocated clusters as > part of that first pass. > > But you're right in that we need to be careful to cap the progress at > the maximum we're advertising. Today mirror_run maintains job offset as (getlength() - dirty_cnt), so it seems that it's not guaranteed always increase, should we give that a consideration? For qemu-img it doesn't matter though. Fam
On 04/17/2014 01:29 AM, Kevin Wolf wrote: >> Okay, now I have a better reason than "Meh, I don't like it" (which >> is always a very bad reason, of course), being the following: As >> mirror_run() is actually made for mirroring from an active block >> device, some sectors may be marked dirty during the block job which >> the initialization loop did not consider dirty. This will not occur >> in the case of qemu-img commit, obviously, as the block device is >> not really used. However, mirror_run() isn't made for use by >> qemu-img only. If new sectors are marked dirty during the block >> job's operation, we'd have to increase the length of the block job >> live which seems very crude to me. >> >> Of course, I'd love to be proven wrong, as I do see that the above >> solution (taking the granularity into account) is pretty crude as >> well. > > I'm not sure if it really matters. The progress is only for the initial > bulk copy anyway. It's possible that you copied a cluster and then the > guest dirties it again, and the progress won't show that. So you already > have some inaccuracy of that kind today. In this light, I would consider > it reasonable enough to treat only the initially allocated clusters as > part of that first pass. > > But you're right in that we need to be careful to cap the progress at > the maximum we're advertising. Libvirt has documented for its job interfaces that the "maximum" number is NOT fixed, and need not be the same as the size of the disk it is tracking. An operation is entitled to raise the maximum appropriately as it accounts for blocks dirtied by the guest after the host had already passed the block on one iteration. For the sake of progress reporting, raising the maximum value as we go DOES give better results, as long as one thing holds true: when the operation is complete, the amount remaining converges to 0 because the amount complete converges to the maximum. Yes, this can mean that progress sometimes travels backwards (the maximum increases by more than the completion across one pass of the disk, if the guest is dirtying the disk faster than the job was copying from the disk during that iteration) - but that's something you WANT to see, as an indication that the job is not going to converge if you don't throttle the guest.
On 17.04.2014 14:08, Eric Blake wrote: > On 04/17/2014 01:29 AM, Kevin Wolf wrote: > >>> Okay, now I have a better reason than "Meh, I don't like it" (which >>> is always a very bad reason, of course), being the following: As >>> mirror_run() is actually made for mirroring from an active block >>> device, some sectors may be marked dirty during the block job which >>> the initialization loop did not consider dirty. This will not occur >>> in the case of qemu-img commit, obviously, as the block device is >>> not really used. However, mirror_run() isn't made for use by >>> qemu-img only. If new sectors are marked dirty during the block >>> job's operation, we'd have to increase the length of the block job >>> live which seems very crude to me. >>> >>> Of course, I'd love to be proven wrong, as I do see that the above >>> solution (taking the granularity into account) is pretty crude as >>> well. >> I'm not sure if it really matters. The progress is only for the initial >> bulk copy anyway. It's possible that you copied a cluster and then the >> guest dirties it again, and the progress won't show that. So you already >> have some inaccuracy of that kind today. In this light, I would consider >> it reasonable enough to treat only the initially allocated clusters as >> part of that first pass. >> >> But you're right in that we need to be careful to cap the progress at >> the maximum we're advertising. > Libvirt has documented for its job interfaces that the "maximum" number > is NOT fixed, and need not be the same as the size of the disk it is > tracking. An operation is entitled to raise the maximum appropriately > as it accounts for blocks dirtied by the guest after the host had > already passed the block on one iteration. For the sake of progress > reporting, raising the maximum value as we go DOES give better results, > as long as one thing holds true: when the operation is complete, the > amount remaining converges to 0 because the amount complete converges to > the maximum. Yes, this can mean that progress sometimes travels > backwards (the maximum increases by more than the completion across one > pass of the disk, if the guest is dirtying the disk faster than the job > was copying from the disk during that iteration) - but that's something > you WANT to see, as an indication that the job is not going to converge > if you don't throttle the guest. Great, then I'll implement that in v5. Max
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index d029609..8bc55cd 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,9 +22,9 @@ STEXI ETEXI DEF("commit", img_commit, - "commit [-q] [-f fmt] [-t cache] filename") + "commit [-q] [-f fmt] [-t cache] [-p] filename") STEXI -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} ETEXI DEF("compare", img_compare, diff --git a/qemu-img.c b/qemu-img.c index 9fe6384..50d7519 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -686,6 +686,9 @@ fail: struct CommonBlockJobCBInfo { Error **errp; bool done; + /* If the blockjob works on several sectors at once, this field has to + * contain that granularity in bytes */ + uint64_t granularity; }; static void common_block_job_cb(void *opaque, int ret) @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, int ret) static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) { BlockJobInfo *info; + uint64_t mod_offset = 0; do { qemu_aio_wait(); info = block_job_query(job); + if (info->offset) { + if (!mod_offset) { + /* Some block jobs (at least "commit") will only work on a + * subset of the image file and therefore basically skip many + * sectors at the start (processing them apparently + * instantaneously). These sectors should be ignored when + * calculating the progress. + * If the blockjob processes several sectors at once, the first + * progress report is likely to come after one such sector group + * (whose size is cbi->granularity), therefore subtract it from + * the current offset in order to obtain a more probable + * starting offset. */ + mod_offset = info->offset > cbi->granularity + ? info->offset - cbi->granularity + : 0; + } + + qemu_progress_print((float)(info->offset - mod_offset) / + (info->len - mod_offset) * 100.f, 0); + } + if (!info->busy && info->offset < info->len) { block_job_resume(job); } @@ -724,13 +749,13 @@ static int img_commit(int argc, char **argv) int c, ret, flags; const char *filename, *fmt, *cache; BlockDriverState *bs, *base_bs; - bool quiet = false; + bool progress = false, quiet = false; Error *local_err = NULL; fmt = NULL; cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:ht:q"); + c = getopt(argc, argv, "f:ht:qp"); if (c == -1) { break; } @@ -745,11 +770,20 @@ static int img_commit(int argc, char **argv) case 't': cache = optarg; break; + case 'p': + progress = true; + break; case 'q': quiet = true; break; } } + + /* Progress is not shown in Quiet mode */ + if (quiet) { + progress = false; + } + if (optind != argc - 1) { help(); } @@ -767,6 +801,9 @@ static int img_commit(int argc, char **argv) return 1; } + qemu_progress_init(progress, 1.f); + qemu_progress_print(0.f, 100); + /* This is different from QMP, which by default uses the deepest file in the * backing chain (i.e., the very base); however, the traditional behavior of * qemu-img commit is using the immediate backing file. */ @@ -778,6 +815,7 @@ static int img_commit(int argc, char **argv) struct CommonBlockJobCBInfo cbi = { .errp = &local_err, + .granularity = COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS, }; commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS, @@ -790,6 +828,8 @@ static int img_commit(int argc, char **argv) run_block_job(bs->job, &cbi); done: + qemu_progress_end(); + bdrv_unref(bs); if (local_err) { diff --git a/qemu-img.texi b/qemu-img.texi index f84590e..1a9c08f 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -140,7 +140,7 @@ this case. @var{backing_file} will never be modified unless you use the The size can also be specified using the @var{size} option with @code{-o}, it doesn't need to be specified separately in this case. -@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} Commit the changes recorded in @var{filename} in its base image or backing file. If the backing file is smaller than the snapshot, then the backing file will be
Implement progress output for the commit command by querying the progress of the block job. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- qemu-img.texi | 2 +- 3 files changed, 45 insertions(+), 5 deletions(-)