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

[Fam Zheng]

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 <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. */

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