qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: "Max Reitz" <mreitz@redhat.com>,
	"Benoît Canet" <benoit.canet@irqsave.net>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH alt 4/7] block/qcow2: Implement status CB for amend
Date: Fri, 01 Aug 2014 14:38:20 -0600	[thread overview]
Message-ID: <53DBFABC.7030207@redhat.com> (raw)
In-Reply-To: <53DBF629.6000703@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2480 bytes --]

On 08/01/2014 02:18 PM, Max Reitz wrote:

>>> +        if (status_cb) {
>>> +            status_cb(bs, *visited_l1_entries << (s->l2_bits +
>>> s->cluster_bits),
>>> +                      l1_entries << (s->l2_bits + s->cluster_bits));
>> Shifting is a multiplication so it keep proportionality intact.
>> So do we really need these shifts ?
> 
> As of patch 1, the variables are defined as "offset" and "working_size"
> which I meant to be byte ranges. We could indeed leave the unit for
> BlockDriverAmendStatusCB's parameters undefined (and leave it up to the
> block driver, only specifying that offset / working_size has to be the
> progress ratio), but then we could just as well just give a floating
> point percentage to that function.

As it is, block jobs are already documented as merely exposing
completion percentage, and NOT tied to the size of the underlying block
device.  Your own pending patch is proof of this:

https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg00960.html

When doing drive-mirror, we WANT to have the total size grow according
to how many dirty blocks are encountered through each pass, and the
current offset grow in rough proportion to how fast we are converging to
a mirrored state (or even having the percentage go backwards, when we
are diverging from too much I/O pressure, as a sign that some throttling
is needed).  Artificially trying to constrain that progress reporting to
the size in bytes of the block device does not help matters.

> 
> Bytes as a unit seemed safe to me, however, since all of qemu's code
> assumes byte ranges to always fit in int64_t; and the reason I preferred
> them over a percentage is that block jobs use bytes as well.
> 
> A real reason not to use bytes would be that some driver is unable to
> give a "byte" representation of its workload during the amend progress;
> however, this seems unlikely to me (every large workload which should be
> part of the progress report should be somehow representable as bytes).

I don't think we can promise anything more than two relative numbers,
with no bearing on what they are measuring under the hood, so scaling
both numbers just to produce progress output buys nothing.  There may
eventually be a device where we can't report progress in any more
granularity than 0 (still working) or 1 (done).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

  reply	other threads:[~2014-08-01 20:38 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-26 19:22 [Qemu-devel] [PATCH alt 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 1/7] block: Add status callback to bdrv_amend_options() Max Reitz
2014-07-31  7:51   ` Benoît Canet
2014-07-31  8:07     ` Benoît Canet
2014-08-01 20:06     ` Max Reitz
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 2/7] qemu-img: Add progress output for amend Max Reitz
2014-07-31  7:56   ` Benoît Canet
2014-08-01 20:09     ` Max Reitz
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 3/7] qemu-img: Fix insignifcant memleak Max Reitz
2014-07-30 14:58   ` Eric Blake
2014-07-30 20:08     ` Max Reitz
2014-07-31  7:59   ` Benoît Canet
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 4/7] block/qcow2: Implement status CB for amend Max Reitz
2014-07-30 16:28   ` Eric Blake
2014-07-31  8:06   ` Benoît Canet
2014-08-01 20:18     ` Max Reitz
2014-08-01 20:38       ` Eric Blake [this message]
2014-08-01 20:48         ` Max Reitz
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 5/7] block/qcow2: Make get_refcount() global Max Reitz
2014-07-31  8:09   ` Benoît Canet
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 6/7] block/qcow2: Simplify shared L2 handling in amend Max Reitz
2014-07-31  8:24   ` Benoît Canet
2014-08-01 20:51     ` Max Reitz
2014-07-26 19:22 ` [Qemu-devel] [PATCH alt 7/7] iotests: Expand test 061 Max Reitz
2014-07-31  8:30   ` Benoît Canet
2014-08-01 20:34     ` Max Reitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53DBFABC.7030207@redhat.com \
    --to=eblake@redhat.com \
    --cc=benoit.canet@irqsave.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).