| Message ID | 1415627159-15941-18-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > > 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 <eblake@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
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; + +typedef struct Qcow2AmendHelperCBInfo { + /* The code coordinating the amend operations should only modify + * these four fields; the rest will be managed by the CB */ + BlockDriverAmendStatusCB *original_status_cb; + void *original_cb_opaque; + + Qcow2AmendOperation current_operation; + + /* Total number of operations to perform (only set once) */ + int total_operations; + + /* The following fields are managed by the CB */ + + /* Number of operations completed */ + int operations_completed; + + /* Cumulative offset of all completed operations */ + int64_t offset_completed; + + Qcow2AmendOperation last_operation; + int64_t last_work_size; +} Qcow2AmendHelperCBInfo; + +static void qcow2_amend_helper_cb(BlockDriverState *bs, int64_t offset, + int64_t total_work_size, void *opaque) +{ + Qcow2AmendHelperCBInfo *info = opaque; + int64_t current_work_size; + int64_t projected_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++; + } + + info->last_operation = info->current_operation; + } + + info->last_work_size = total_work_size; + + current_work_size = info->offset_completed + total_work_size; + + /* current_work_size is the total work size for (operations_completed + 1) + * 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); + + info->original_status_cb(bs, info->offset_completed + offset, + current_work_size + projected_work_size, + info->original_cb_opaque); +} + 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) + }; + /* 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); if (ret < 0) { return ret; }
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(-)