Re: [Qemu-devel] [PATCH] backup: Fail early if cannot determine cluster size

[John Snow <jsnow@redhat.com>]

On 05/18/2016 01:53 AM, Fam Zheng wrote:
> Otherwise the job is orphaned and block_job_cancel_sync in
> bdrv_close_all() when quiting will hang.
> 

Tch, mea culpa.

> A simple reproducer is running blockdev-backup from null-co:// to
> null-co://.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 491fd14..3ff48c6 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -505,6 +505,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>      int64_t len;
>      BlockDriverInfo bdi;
>      int ret;
> +    int64_t cluster_size;
>  
>      assert(bs);
>      assert(target);
> @@ -568,23 +569,10 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>          goto error;
>      }
>  
> -    BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> -                                           cb, opaque, errp);
> -    if (!job) {
> -        goto error;
> -    }
> -
> -    job->on_source_error = on_source_error;
> -    job->on_target_error = on_target_error;
> -    job->target = target;
> -    job->sync_mode = sync_mode;
> -    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> -                       sync_bitmap : NULL;
> -
>      /* If there is no backing file on the target, we cannot rely on COW if our
>       * backup cluster size is smaller than the target cluster size. Even for
>       * targets with a backing file, try to avoid COW if possible. */
> -    ret = bdrv_get_info(job->target, &bdi);
> +    ret = bdrv_get_info(target, &bdi);
>      if (ret < 0 && !target->backing) {
>          error_setg_errno(errp, -ret,
>              "Couldn't determine the cluster size of the target image, "
> @@ -594,11 +582,25 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>          goto error;
>      } else if (ret < 0 && target->backing) {
>          /* Not fatal; just trudge on ahead. */
> -        job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
> +        cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
>      } else {
> -        job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> +        cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
>      }
>  
> +    BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
> +                                           cb, opaque, errp);
> +    if (!job) {
> +        goto error;
> +    }
> +
> +    job->on_source_error = on_source_error;
> +    job->on_target_error = on_target_error;
> +    job->target = target;
> +    job->sync_mode = sync_mode;
> +    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
> +                       sync_bitmap : NULL;
> +    job->cluster_size = cluster_size;
> +
>      bdrv_op_block_all(target, job->common.blocker);
>      job->common.len = len;
>      job->common.co = qemu_coroutine_create(backup_run);
> 

Thank you.

Reviewed-by: John Snow <jsnow@redhat.com>