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

[Max Reitz]

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.

Max