Re: [Qemu-block] [RFC PATCH COLO v2 09/13] block: Parse "backing_reference" option to reference existing BDS

[Fam Zheng]

On Wed, 03/25 17:36, Wen Congyang wrote:
> Usage:
> -drive file=xxx,id=Y, \
> -drive 
> file=xxxx,id=X,backing_reference.drive_id=Y,backing_reference.hidden-disk.*
> 
> It will create such backing chain:
>                {virtio-blk dev 'Y'}                                      
> {virtio-blk dev 'X'}
>                          |                                                    
>       |
>                          |                                                    
>       |
>                          v                                                    
>       v
> 
>     [base] <- [mid] <- ( Y )  <----------------- (hidden target) 
> <--------------- ( X )
> 
>                          v                              ^
>                          v                              ^
>                          v                              ^
>                          v                              ^
>                          >>>> drive-backup sync=none >>>>
> 
> X's backing file is hidden-disk, and hidden-disk's backing file is Y.
> Disk Y may be opened or reopened in read-write mode, so A block backup
> job is automatically created: source is Y and target is hidden-disk.
> 
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  block.c                    | 145 
> ++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block.h      |   1 +
>  include/block/block_int.h  |   1 +
>  tests/qemu-iotests/051     |  13 ++++
>  tests/qemu-iotests/051.out |  13 ++++
>  5 files changed, 170 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b4d629e..bd7fa9c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1351,6 +1351,113 @@ free_exit:
>      return ret;
>  }
>  
> +static void backing_reference_completed(void *opaque, int ret)
> +{
> +    BlockDriverState *hidden_disk = opaque;
> +
> +    assert(!hidden_disk->backing_reference);
> +}
> +
> +static int bdrv_open_backing_reference_file(BlockDriverState *bs,
> +                                            QDict *options, Error **errp)
> +{
> +    const char *backing_name;
> +    QDict *hidden_disk_options = NULL;
> +    BlockDriverState *backing_hd, *hidden_disk;
> +    BlockBackend *backing_blk;
> +    Error *local_err = NULL;
> +    int ret = 0;
> +
> +    backing_name = qdict_get_try_str(options, "drive_id");
> +    if (!backing_name) {
> +        error_setg(errp, "Backing reference needs option drive_id");
> +        ret = -EINVAL;
> +        goto free_exit;
> +    }
> +    qdict_del(options, "drive_id");
> +
> +    qdict_extract_subqdict(options, &hidden_disk_options, "hidden-disk.");
> +    if (!qdict_size(hidden_disk_options)) {
> +        error_setg(errp, "Backing reference needs option hidden-disk.*");
> +        ret = -EINVAL;
> +        goto free_exit;
> +    }
> +
> +    if (qdict_size(options)) {
> +        const QDictEntry *entry = qdict_first(options);
> +        error_setg(errp, "Backing reference used by '%s' doesn't support "
> +                   "the option '%s'", bdrv_get_device_name(bs), entry->key);
> +        ret = -EINVAL;
> +        goto free_exit;
> +    }
> +
> +    backing_blk = blk_by_name(backing_name);
> +    if (!backing_blk) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, backing_name);
> +        ret = -ENOENT;
> +        goto free_exit;
> +    }
> +
> +    backing_hd = blk_bs(backing_blk);
> +    /* Backing reference itself? */
> +    if (backing_hd == bs || bdrv_find_overlay(backing_hd, bs)) {
> +        error_setg(errp, "Backing reference itself");
> +        ret = -EINVAL;
> +        goto free_exit;
> +    }
> +
> +    if (bdrv_op_is_blocked(backing_hd, BLOCK_OP_TYPE_BACKING_REFERENCE,
> +                           errp)) {
> +        ret = -EBUSY;
> +        goto free_exit;
> +    }
> +
> +    /* hidden-disk is bs's backing file */
> +    ret = bdrv_open_backing_file(bs, hidden_disk_options, errp);
> +    hidden_disk_options = NULL;
> +    if (ret < 0) {
> +        goto free_exit;
> +    }
> +
> +    hidden_disk = bs->backing_hd;
> +    if (!hidden_disk->drv || !hidden_disk->drv->supports_backing) {
> +        ret = -EINVAL;
> +        error_setg(errp, "Hidden disk's driver doesn't support backing 
> files");
> +        goto free_exit;
> +    }
> +
> +    bdrv_set_backing_hd(hidden_disk, backing_hd);
> +    bdrv_ref(backing_hd);
> +
> +    /*
> +     * backing hd may be opened or reopened in read-write mode, so we
> +     * should backup backing hd to hidden disk
> +     */
> +    bdrv_op_unblock(hidden_disk, BLOCK_OP_TYPE_BACKUP_TARGET,
> +                    bs->backing_blocker);
> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
> +                    hidden_disk->backing_blocker);
> +
> +    bdrv_ref(hidden_disk);
> +    backup_start(backing_hd, hidden_disk, 0, MIRROR_SYNC_MODE_NONE,
> +                 BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
> +                 backing_reference_completed, hidden_disk, &local_err);

We need to be careful, the backing_hd may not be safe to access, depending on
which AioContext it is running on. At least, we should make sure bs,
hidden_disk and backing_hd are all on the same AioContext.

Here, before accessing backing_hd, we need to call aio_context_acquire, like in
qmp_drive_backup.

Fam

> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        bdrv_unref(hidden_disk);
> +        /* FIXME, use which errno? */
> +        ret = -EIO;
> +        goto free_exit;
> +    }
> +
> +    bs->backing_reference = true;
> +
> +free_exit:
> +    QDECREF(hidden_disk_options);
> +    QDECREF(options);
> +    return ret;
> +}
> +
>  /*
>   * Opens a disk image whose options are given as BlockdevRef in another block
>   * device's options.
> @@ -1604,13 +1711,37 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>  
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
> -        QDict *backing_options;
> +        QDict *backing_options, *backing_reference_options;
>  
> +        qdict_extract_subqdict(options, &backing_reference_options,
> +                               "backing_reference.");
>          qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> +
> +        if (qdict_size(backing_reference_options) &&
> +            qdict_size(backing_options)) {
> +            error_setg(&local_err,
> +                       "Option \"backing_reference.*\" and \"backing.*\""
> +                       " cannot be used together");
> +            ret = -EINVAL;
> +            QDECREF(backing_reference_options);
> +            QDECREF(backing_options);
>              goto close_and_fail;
>          }
> +        if (qdict_size(backing_reference_options)) {
> +            QDECREF(backing_options);
> +            ret = bdrv_open_backing_reference_file(bs,
> +                                                   backing_reference_options,
> +                                                   &local_err);
> +            if (ret) {
> +                goto close_and_fail;
> +            }
> +        } else {
> +            QDECREF(backing_reference_options);
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
> +        }
>      }
>  
>      bdrv_refresh_filename(bs);
> @@ -1941,6 +2072,14 @@ void bdrv_close(BlockDriverState *bs)
>      if (bs->drv) {
>          if (bs->backing_hd) {
>              BlockDriverState *backing_hd = bs->backing_hd;
> +            if (bs->backing_reference) {
> +                assert(backing_hd->backing_hd);
> +                if (backing_hd->backing_hd->job) {
> +                    block_job_cancel(backing_hd->backing_hd->job);
> +                }
> +                bdrv_set_backing_hd(backing_hd, NULL);
> +                bdrv_unref(backing_hd->backing_hd);
> +            }
>              bdrv_set_backing_hd(bs, NULL);
>              bdrv_unref(backing_hd);
>          }
> diff --git a/include/block/block.h b/include/block/block.h
> index 68f3b1a..7138e90 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -159,6 +159,7 @@ typedef enum BlockOpType {
>      BLOCK_OP_TYPE_RESIZE,
>      BLOCK_OP_TYPE_STREAM,
>      BLOCK_OP_TYPE_REPLACE,
> +    BLOCK_OP_TYPE_BACKING_REFERENCE,
>      BLOCK_OP_TYPE_MAX,
>  } BlockOpType;
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 08dd8ba..624945d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -375,6 +375,7 @@ struct BlockDriverState {
>      QDict *full_open_options;
>      char exact_filename[PATH_MAX];
>  
> +    bool backing_reference;
>      BlockDriverState *backing_hd;
>      BlockDriverState *file;
>  
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 0360f37..fd67f40 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -116,6 +116,19 @@ run_qemu -drive 
> file="$TEST_IMG",file.backing.driver=file,file.backing.filename=
>  run_qemu -drive 
> file="$TEST_IMG",file.backing.driver=qcow2,file.backing.file.filename="$TEST_IMG.orig"
>  
>  echo
> +echo === Backing file reference ===
> +echo
> +
> +run_qemu -drive file="$TEST_IMG",if=none,id=drive0 \
> +    -drive 
> file="$TEST_IMG",driver=qcow2,backing_reference.drive_id=drive0,backing_reference.hidden-disk.filename="$TEST_IMG.hidden"
> +
> +run_qemu -drive file="$TEST_IMG",if=none,id=drive0 \
> +    -drive 
> file="$TEST_IMG",driver=qcow2,backing_reference.drive_id=drive0,backing_reference.hidden-disk.filename="$TEST_IMG.hidden",backing.file.filename="$TEST_IMG.orig"
> +
> +run_qemu -drive file="$TEST_IMG",if=none,id=drive0 \
> +    -drive 
> file="$TEST_IMG",driver=qcow2,file.backing_reference.drive_id=drive0,file.backing_reference.hidden-disk.filename="$TEST_IMG.hidden",file.backing.file.filename="$TEST_IMG.orig"
> +
> +echo
>  echo === Enable and disable lazy refcounting on the command line, plus some 
> invalid values ===
>  echo
>  
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index 2890eac..cb8340b 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -75,6 +75,19 @@ Testing: -drive 
> file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.fil
>  QEMU_PROG: -drive 
> file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.file.filename=TEST_DIR/t.qcow2.orig:
>  Driver doesn't support backing files
>  
>  
> +=== Backing file reference ===
> +
> +Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0 -drive 
> file=TEST_DIR/t.qcow2,driver=qcow2,backing_reference.drive_id=drive0,backing_reference.hidden-disk.filename=TEST_DIR/t.qcow2.hidden
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) qququiquit
> +
> +Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0 -drive 
> file=TEST_DIR/t.qcow2,driver=qcow2,backing_reference.drive_id=drive0,backing_reference.hidden-disk.filename=TEST_DIR/t.qcow2.hidden,backing.file.filename=TEST_DIR/t.qcow2.orig
> +QEMU_PROG: -drive 
> file=TEST_DIR/t.qcow2,driver=qcow2,backing=drive0,backing.file.filename=TEST_DIR/t.qcow2.orig:
>  Option "backing_reference.*" and "backing.*" cannot be used together
> +
> +Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0 -drive 
> file=TEST_DIR/t.qcow2,driver=qcow2,file.backing_reference.drive_id=drive0,file.backing_reference.hidden-disk.filename=TEST_DIR/t.qcow2.hidden,file.backing.file.filename=TEST_DIR/t.qcow2.orig
> +QEMU_PROG: -drive 
> file=TEST_DIR/t.qcow2,driver=qcow2,file.backing=drive0,file.backing.file.filename=TEST_DIR/t.qcow2.orig:
>  Option "backing_reference.*" and "backing.*" cannot be used together
> +
> +
>  === Enable and disable lazy refcounting on the command line, plus some 
> invalid values ===
>  
>  Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,lazy-refcounts=on
> -- 
> 2.1.0
>