Re: [Qemu-devel] [PATCH v7 03/10] block: Parse "backing" option to reference existing BDS

[Markus Armbruster <armbru@redhat.com>]

Fam Zheng <famz@redhat.com> writes:

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 109 +++++++++++++++++++++++++++++-----------------
>  block/mirror.c            |   2 +-
>  include/block/block.h     |   3 +-
>  include/block/block_int.h |   3 ++
>  4 files changed, 76 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index c1a3576..41562fd 100644
> --- a/block.c
> +++ b/block.c
> @@ -961,63 +961,87 @@ fail:
>  /*
>   * Opens the backing file for a BlockDriverState if not yet open
>   *
> + * If backing_bs is not NULL or empty, find the BDS by name and reference it as
> + * the backing_hd, in this case options is ignored.
> + *

The name "backing_bs" suggests this is a BlockDriverState, which it's
not.  What about calling it "name", just like bdrv_find()'s parameter?
Or "device_name", like the BlockDriverState member?

>   * options is a QDict of options to pass to the block drivers, or NULL for an
>   * empty set of options. The reference to the QDict is transferred to this
>   * function (even on failure), so if the caller intends to reuse the dictionary,
>   * it needs to use QINCREF() before calling bdrv_file_open.
>   */
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> +                           QDict *options, Error **errp)

This function is now too overloaded for my taste.

Possibly cleaner:

* Have a new function to set backing_hd.  Takes a device name argument,
  or maybe a BDS.  I'm leaning towards the latter.

* The new function and old bdrv_open_backing_file() have a common tail.
  Factor it out into a helper.

>  {
>      char backing_filename[PATH_MAX];
>      int back_flags, ret;
>      BlockDriver *back_drv = NULL;
>      Error *local_err = NULL;
>  
> -    if (bs->backing_hd != NULL) {
> -        QDECREF(options);
> -        return 0;
> -    }
> +    if (backing_bs && *backing_bs != '\0') {
> +        bs->backing_hd = bdrv_find(backing_bs);

What if bs->backing_hd is already set?

> +        if (!bs->backing_hd) {
> +            error_setg(errp, "Backing device not found: %s", backing_bs);
> +            return -ENOENT;
> +        }
> +        bdrv_ref(bs->backing_hd);
> +    } else {
> +        if (bs->backing_hd != NULL) {
> +            QDECREF(options);
> +            return 0;
> +        }
>  
> -    /* NULL means an empty set of options */
> -    if (options == NULL) {
> -        options = qdict_new();
> -    }
> +        /* NULL means an empty set of options */
> +        if (options == NULL) {
> +            options = qdict_new();
> +        }
>  
> -    bs->open_flags &= ~BDRV_O_NO_BACKING;
> -    if (qdict_haskey(options, "file.filename")) {
> -        backing_filename[0] = '\0';
> -    } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> -        QDECREF(options);
> -        return 0;
> -    } else {
> -        bdrv_get_full_backing_filename(bs, backing_filename,
> -                                       sizeof(backing_filename));
> -    }
> +        bs->open_flags &= ~BDRV_O_NO_BACKING;
> +        if (qdict_haskey(options, "file.filename")) {
> +            backing_filename[0] = '\0';
> +        } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> +            QDECREF(options);
> +            return 0;
> +        } else {
> +            bdrv_get_full_backing_filename(bs, backing_filename,
> +                                           sizeof(backing_filename));
> +        }
>  
> -    bs->backing_hd = bdrv_new("");
> +        bs->backing_hd = bdrv_new("");
>  
> -    if (bs->backing_format[0] != '\0') {
> -        back_drv = bdrv_find_format(bs->backing_format);
> -    }
> +        if (bs->backing_format[0] != '\0') {
> +            back_drv = bdrv_find_format(bs->backing_format);
> +        }
>  
> -    /* backing files always opened read-only */
> -    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> -                                    BDRV_O_COPY_ON_READ);
> +        /* backing files always opened read-only */
> +        back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> +                                        BDRV_O_COPY_ON_READ);
>  
> -    ret = bdrv_open(bs->backing_hd,
> -                    *backing_filename ? backing_filename : NULL, options,
> -                    back_flags, back_drv, &local_err);
> -    if (ret < 0) {
> -        bdrv_unref(bs->backing_hd);
> -        bs->backing_hd = NULL;
> -        bs->open_flags |= BDRV_O_NO_BACKING;
> -        error_setg(errp, "Could not open backing file: %s",
> -                   error_get_pretty(local_err));
> -        error_free(local_err);
> -        return ret;
> +        ret = bdrv_open(bs->backing_hd,
> +                        *backing_filename ? backing_filename : NULL, options,
> +                        back_flags, back_drv, &local_err);
> +        if (ret < 0) {
> +            bdrv_unref(bs->backing_hd);
> +            bs->backing_hd = NULL;
> +            bs->open_flags |= BDRV_O_NO_BACKING;
> +            error_setg(errp, "Could not open backing file: %s",
> +                       error_get_pretty(local_err));
> +            error_free(local_err);
> +            return ret;
> +        }
>      }
> +    assert(bs->backing_hd);
> +    assert(!bs->backing_blocker);
> +    error_setg(&bs->backing_blocker,
> +               "device is used as backing hd of '%s'",
> +               bs->device_name);
> +    bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> +    /* Otherwise we won't be able to commit due to check in bdrv_commit */
> +    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> +                    bs->backing_blocker);
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>              bs->backing_hd->file->filename);
> +    pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> +            bs->backing_hd->drv->format_name);
>      return 0;
>  }
>  

This patch hunk is hard to review, because you had to indent the old
code.  Same with whitespace differences ignored:

  @@ -961,18 +961,30 @@
   /*
    * Opens the backing file for a BlockDriverState if not yet open
    *
  + * If backing_bs is not NULL or empty, find the BDS by name and reference it as
  + * the backing_hd, in this case options is ignored.
  + *
    * options is a QDict of options to pass to the block drivers, or NULL for an
    * empty set of options. The reference to the QDict is transferred to this
    * function (even on failure), so if the caller intends to reuse the dictionary,
    * it needs to use QINCREF() before calling bdrv_file_open.
    */
  -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
  +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
  +                           QDict *options, Error **errp)
   {
       char backing_filename[PATH_MAX];
       int back_flags, ret;
       BlockDriver *back_drv = NULL;
       Error *local_err = NULL;

  +    if (backing_bs && *backing_bs != '\0') {
  +        bs->backing_hd = bdrv_find(backing_bs);
  +        if (!bs->backing_hd) {
  +            error_setg(errp, "Backing device not found: %s", backing_bs);
  +            return -ENOENT;
  +        }
  +        bdrv_ref(bs->backing_hd);
  +    } else {
       if (bs->backing_hd != NULL) {
           QDECREF(options);
           return 0;
  @@ -1016,8 +1028,20 @@
           error_free(local_err);
           return ret;
       }
  +    }
  +    assert(bs->backing_hd);
  +    assert(!bs->backing_blocker);
  +    error_setg(&bs->backing_blocker,
  +               "device is used as backing hd of '%s'",
  +               bs->device_name);
  +    bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
  +    /* Otherwise we won't be able to commit due to check in bdrv_commit */
  +    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
  +                    bs->backing_blocker);
       pstrcpy(bs->backing_file, sizeof(bs->backing_file),
               bs->backing_hd->file->filename);
  +    pstrcpy(bs->backing_format, sizeof(bs->backing_format),
  +            bs->backing_hd->drv->format_name);
       return 0;
   }

Looks like you do more than the stated subject in this patch: you also
add a blocker!  Separate patch, please.

"Block all" followed by "unblock specific" is a bit awkward.  A function
taking a bit set would let you do the masking on bits rather than an
array of lists.  Suggestion, not a request, and it can be done in a
followup patch.

> @@ -1164,9 +1188,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
>          qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +        ret = bdrv_open_backing_file(bs, qdict_get_try_str(options, "backing"),
> +                                     backing_options, &local_err);
> +
> +        qdict_del(options, "backing");

Why do you have to delete "backing"?

>          if (ret < 0) {
>              goto close_and_fail;
>          }
> @@ -1459,9 +1485,14 @@ void bdrv_close(BlockDriverState *bs)
>  
>      if (bs->drv) {
>          if (bs->backing_hd) {
> +            assert(bs->backing_blocker);
> +            bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
>              bdrv_unref(bs->backing_hd);
>              bs->backing_hd = NULL;
>          }
> +        if (bs->backing_blocker) {
> +            error_free(bs->backing_blocker);
> +        }

Can bs->backing_blocker legitimately be non-null when bs->backing_hd is
null?

>          bs->drv->bdrv_close(bs);
>          g_free(bs->opaque);
>  #ifdef _WIN32
> diff --git a/block/mirror.c b/block/mirror.c
> index 6dc27ad..b1596d3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -511,7 +511,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_open_backing_file(s->target, NULL, &local_err);
> +    ret = bdrv_open_backing_file(s->target, NULL, NULL, &local_err);
>      if (ret < 0) {
>          char backing_filename[PATH_MAX];
>          bdrv_get_full_backing_filename(s->target, backing_filename,
> diff --git a/include/block/block.h b/include/block/block.h
> index 18397e0..24d06f9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -185,7 +185,8 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_parse_discard_flags(const char *mode, int *flags);
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>                     QDict *options, int flags, Error **errp);
> -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs,
> +                           QDict *options, Error **errp);
>  int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>                int flags, BlockDriver *drv, Error **errp);
>  BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 458acd6..83dcf7d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -342,6 +342,9 @@ struct BlockDriverState {
>      BlockJob *job;
>  
>      QDict *options;
> +
> +    /* For backing reference, block the operations of named backing device */
> +    Error *backing_blocker;
>  };
>  
>  int get_tmp_filename(char *filename, int size);

I find the comment confusing :)

What about:

/* The error object in use for blocking operations on backing_hd */