Re: [Qemu-devel] [PATCH 17/21] qcow2: Use intermediate helper CB for amend

[Eric Blake]

On 11/10/2014 06:45 AM, Max Reitz wrote:
> If there is more than one time-consuming operation to be performed for
> qcow2_amend_options(), we need an intermediate CB which coordinates the
> progress of the individual operations and passes the result to the
> original status callback.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2.c | 76 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 1 deletion(-)

Getting trickier to review.

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index eaef251..e6b93d1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2655,6 +2655,71 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
> target_version,
>      return 0;
>  }
>  
> +typedef enum Qcow2AmendOperation {
> +    /* This is the value Qcow2AmendHelperCBInfo::last_operation will be
> +     * statically initialized to so that the helper CB can discern the first
> +     * invocation from an operation change */
> +    QCOW2_NO_OPERATION = 0,
> +
> +    QCOW2_DOWNGRADING,
> +} Qcow2AmendOperation;

So for this patch, you still have just one operation, but later in the
series, you add a second (and the goal of THIS patch is that it will
work even if there are 3 or more operations, even though this series
doesn't add that many).


> +static void qcow2_amend_helper_cb(BlockDriverState *bs, int64_t offset,
> +                                 int64_t total_work_size, void *opaque)

indentation looks off

> +{
> +    Qcow2AmendHelperCBInfo *info = opaque;
> +    int64_t current_work_size;
> +    int64_t projected_work_size;

Worth asserting that info->total_operations is non-zero?  Or is there
ever a valid case for calling the callback even when there are no
sub-operations, and therefore we are automatically complete (offset ==
total_work_size)?

> +
> +    if (info->current_operation != info->last_operation) {
> +        if (info->last_operation != QCOW2_NO_OPERATION) {
> +            info->offset_completed += info->last_work_size;
> +            info->operations_completed++;
> +        }

Would it be any easier to guarantee that we come to 100% completion by
requiring the coordinator to pass a final completion callback? [1]
 info->current_operation = QCOW2_NO_OPERATION;
 cb(bs, 0, 0, info)

> +
> +        info->last_operation = info->current_operation;
> +    }
> +
> +    info->last_work_size = total_work_size;

Took me a while to realize that total_work_size is the incoming
(estimated) total size for the current sub-operation, and not the total
over the combination of all sub-operations...

> +
> +    current_work_size = info->offset_completed + total_work_size;
> +
> +    /* current_work_size is the total work size for (operations_completed + 
> 1)

but this comment helped.

> +     * operations (which includes this one), so multiply it by the number of
> +     * operations not covered and divide it by the number of operations
> +     * covered to get a projection for the operations not covered */
> +    projected_work_size = current_work_size * (info->total_operations -
> +                                               info->operations_completed - 
> 1)
> +                                            / (info->operations_completed + 
> 1);

So, when there is just one sub-operation (which is the case until later
patches add a second), this results in the following calculation for ALL
calls during the intermediate steps of the sub-operation:

projected_work_size = total_work_size * (1 - 0 - 1) / (0 + 1)

that is, we are projecting 0 additional work because we have zero
additional stages to complete.  Am I correct that we will never enter
the callback in a state where
info->operations_completed==info->total_operations?  (because if we do,
you'd have a computation of final_size * (1 - 1 - 1) / (1 + 1) which
looks weird).  Worth an assert()?  Then again, my proposal above [1] to
guarantee a 100% completion by use of a final cleanup callback would
indeed reach the point where operations_completed==total_operations.

> +
> +    info->original_status_cb(bs, info->offset_completed + offset,
> +                             current_work_size + projected_work_size,
> +                             info->original_cb_opaque);

So, as long as we don't add a second phase, this is strictly equivalent
to calling the original callback with the original offset (since
info->offset_completed remains 0) and original work size (since
projected_work_size remains 0).  That part works fine.

Let's see what happens if we had three phases.  To make it more
interesting, let's pick some numbers - how about the first phase
progresses from 0-10, the second from 0-100, and the third from 0-10,
and where none of the sub-operations change predicted total_work_size.
The caller would first set info->current_operation to 1, then call the
callback a few times; how about twice with 5/10 and 10/10.  For both
calls, current_work_size is 0+10, then projected_work_size is
10*(3-0-1)/(0+1) == 20, and we call the original callback with
(0+5)/(10+20) and (0+10)/(10+20).  Pretty good (5/30 and 10/30 are right
on if the first sub-command is exactly one-third of the time of the
overall command; and even if it is not, it still shows reasonable progress).

Then we move on to the second sub-command where the coordinator updates
info->current_operation to 2 before triggering several callbacks; let's
suppose it reports at 0/100, 30/100, 60/100, and 100/100.  The first
call updates info to track that we've detected a change in sub-command
(offset_completed is now 10, operations_completed is now 1).  Then for
all four calls, current_work_size is 10+100, and projected_work_size is
110*(3-1-1)/(1+1) == 55.  So we call the original callback with
(10+0)/(110+55), (10+30)/(110+55), (10+60)/(110+55), (10+100)/(110+55).
 The first report of 10/165 looks like we jumped backwards (much smaller
progress than our previous report of 10/30), but that's merely a
representation that this phase is estimating a larger total_work count,
and we have no way of correlating whether 1 unit of work count in each
phase is equivalent to an equal amount of time.  But by the end, we
report 110/165, which is spot on for being two-thirds complete.

Another assignment to info->current_operation, and a couple more
callbacks; let's again use 5/10 and 10/10.  The first callback updates
info (offset_completed is now 110, operations_completed is now 2).  For
each call, current_work_size is 110+10, and projected_work_size is
120*(3-2-1/(2+1) == 0.  We call the original callback with
(120+5)/(120+10) and (120+10)/(120+10).  We've done a very rapid jump
from 2/3 to 125/130, but end the overall operation with the two values
equal.  So the function is not very smooth, but at least it is as good
an estimate as possible along each stage of the operation, and we never
violate the premise of reporting equal values until all sub-commands are
complete.

> +}
> +
>  static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>                                 BlockDriverAmendStatusCB *status_cb,
>                                 void *cb_opaque)
> @@ -2669,6 +2734,7 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>      bool encrypt;
>      int ret;
>      QemuOptDesc *desc = opts->list->desc;
> +    Qcow2AmendHelperCBInfo helper_cb_info;
>  
>      while (desc && desc->name) {
>          if (!qemu_opt_find(opts, desc->name)) {
> @@ -2726,6 +2792,12 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>          desc++;
>      }
>  
> +    helper_cb_info = (Qcow2AmendHelperCBInfo){
> +        .original_status_cb = status_cb,
> +        .original_cb_opaque = cb_opaque,
> +        .total_operations = (new_version < old_version)
> +    };

Slick.

> +
>      /* Upgrade first (some features may require compat=1.1) */
>      if (new_version > old_version) {
>          s->qcow_version = new_version;
> @@ -2784,7 +2856,9 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>  
>      /* Downgrade last (so unsupported features can be removed before) */
>      if (new_version < old_version) {
> -        ret = qcow2_downgrade(bs, new_version, status_cb, cb_opaque);
> +        helper_cb_info.current_operation = QCOW2_DOWNGRADING;
> +        ret = qcow2_downgrade(bs, new_version, &qcow2_amend_helper_cb,
> +                              &helper_cb_info);

Looks correct to me. Other than the indentation issue and possible
addition of some asserts, this is good to go.

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature