Re: [Qemu-devel] [PATCH v4 4/6] qemu-img: Enable progress output for commit

[Kevin Wolf]

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 <address@hidden>
> >>>---
> >>>  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
> >>>address@hidden commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename}
> >>>address@hidden 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