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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-11 at 22:05, Eric Blake wrote:
> 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 <mreitz@redhat.com>
>> ---
>>   block/qcow2.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 75 insertions(+), 1 deletion(-)
> Getting trickier to review.

Yes, and 18 is the worst.

>> 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).

Right.

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

Right, will fix.

>> +{
>> +    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)?

No, the driver does not have to call the status CB; qemu-img amend will 
mind that case by first printing 0 %, then invoking the amend operation, 
and then printing 100 %. Will add an assertion.

>> +
>> +    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)

No, because the amend CB does not have to be called on completion; 
img_amend() in qemu-img.c takes care of that case.

>> +
>> +        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...

Ah, right, I'll change the variable name, maybe to operation_work_size 
or something like that.

>> +
>> +    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?

Yes, we won't.

> (because if we do,
> you'd have a computation of final_size * (1 - 1 - 1) / (1 + 1) which
> looks weird).  Worth an assert()?

assert()s are always worth it.

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

Yes, it's not pretty but it's the best we can do without either 
hard-coding some estimations or trying some kind of dry-run to determine 
each operation's work size beforehand.

>> +}
>> +
>>   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 <eblake@redhat.com>

Thanks!

Max