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 <mre...@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

Reply via email to