Re: [Qemu-devel] [PATCH] block: fix streaming/closing race

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Mar 29, 2012 at 06:08:40PM +0200, Paolo Bonzini wrote:
> Streaming can issue I/O while qcow2_close is running.  This causes the
> L2 caches to become very confused or, alternatively, could cause a
> segfault when the streaming coroutine is reentered after closing its
> block device.  The fix is to cancel streaming jobs when closing their
> underlying device.  The cancellation must be synchronous, so add a flag
> saying whether streaming has in-flight I/O.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block.c        |   14 ++++++++++++++
>  block/stream.c |    4 +++-
>  block_int.h    |    2 ++
>  3 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b88ee90..1aab4bf 100644
> --- a/block.c
> +++ b/block.c
> @@ -813,6 +813,9 @@ unlink_and_fail:
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> +        if (bs->job) {
> +            block_job_cancel_sync(bs->job);
> +        }
>          if (bs == bs_snapshots) {
>              bs_snapshots = NULL;
>          }
> @@ -4069,3 +4072,14 @@ bool block_job_is_cancelled(BlockJob *job)
>  {
>      return job->cancelled;
>  }
> +
> +void block_job_cancel_sync(BlockJob *job)
> +{
> +    BlockDriverState *bs = job->bs;
> +
> +    assert(bs->job == job);
> +    block_job_cancel(job);
> +    while (bs->job != NULL && bs->job->busy) {

It's not clear to me why we have a busy flag.  Is canceling and waiting
for bs->job == NULL not enough?  Perhaps you're trying to take a
shortcut and not wait until the job is fully stopped - it's not worth it
since the streaming block job does no significant work after detecting
cancelation.

Stefan