Re: [libvirt] [PATCH] virsh: improve waiting for block job readiness
On Mon, 4 Jan 2016, Andrea Bolognani wrote:
On Thu, 2015-12-31 at 16:34 +1100, Michael Chapman wrote:
> Wait for a block job event after the job has reached 100% only if
> exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
> registered.
>
> If neither callback was registered, then no event will ever be received.
> If both were registered, then any user-supplied path is guaranteed to
> match one of the events.
>
> Signed-off-by: Michael Chapman <mike(a)very.puzzling.org>
> ---
>
> I have found that even a 2.5 second timeout isn't always sufficiently
> long for QEMU to flush a disk at the end of a block job.
>
> I hope I've understood the code properly here, because as far as I can
> tell the comment I'm removing in this commit isn't right. The path the
> user supplies *has* to be either the <source file='name'/> or
<target
> dev='name'/> exactly in order for the disk to be matched, and these are
> precisely the two strings used by the two events.
>
> tools/virsh-domain.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index edbbc34..60de9ba 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
> goto cleanup;
> }
>
> - /* since virsh can't guarantee that the path provided by the user will
> - * later be matched in the event we will need to keep the fallback
> - * approach and claim success if the block job finishes or vanishes. */
> - if (result == 0)
> - break;
> + /* if either event could not be registered we can't guarantee that the
> + * path provided by the user will be matched, so keep the fallback
> + * approach and claim success if the block job finishes or vanishes */
> + if (data->cb_id2 < 0 || data->cb_id2 < 0) {
I'm not going to comment on the rest of the patch, but the condition
above doesn't look like it would make any sense written that way...
Oops, good catch. I had meant to use cb_id and cb_id2 respectively in the
two conditions (and in the following test as well).
> + if (result == 0)
> + break;
>
> - /* for two-phase jobs we will try to wait in the synchronized phase
> - * for event arrival since 100% completion doesn't necessarily mean
that
> - * the block job has finished and can be terminated with success */
> - if (info.end == info.cur && --retries == 0) {
> - ret = VIR_DOMAIN_BLOCK_JOB_READY;
> - goto cleanup;
> + /* only wait in the synchronized phase for event arrival if
> + * either callback was registered */
> + if (info.end == info.cur &&
> + ((data->cb_id2 < 0 && data->cb_id2 < 0) ||
--retries == 0)) {
... and neither does this one.
Am I missing something?
Cheers.
> + ret = VIR_DOMAIN_BLOCK_JOB_READY;
> + goto cleanup;
> + }
> }
>
> if (data->verbose)
--
Andrea Bolognani
Software Engineer - Virtualization Team
- Michael