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

[Max Reitz]

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.

Max