From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56279) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTwY-0003N1-HL for qemu-devel@nongnu.org; Wed, 12 Nov 2014 04:10:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoTwS-0007Z6-B6 for qemu-devel@nongnu.org; Wed, 12 Nov 2014 04:10:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45268) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTwS-0007Yu-2n for qemu-devel@nongnu.org; Wed, 12 Nov 2014 04:10:20 -0500 Message-ID: <546323F3.8030209@redhat.com> Date: Wed, 12 Nov 2014 10:10:11 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-18-git-send-email-mreitz@redhat.com> <546279FD.80507@redhat.com> In-Reply-To: <546279FD.80507@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 17/21] qcow2: Use intermediate helper CB for amend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi 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 >> --- >> 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 Thanks! Max