Re: [Qemu-devel] [PATCH 08/16] block: Manage backing file references in bdrv_set_backing_hd()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This simplifies the code somewhat, especially when dropping whole
> backing file subchains.
> 
> The exception is the mirroring code that does adventurous things with
> bdrv_swap() and in order to keep it working, I had to duplicate most of
> bdrv_set_backing_hd() locally. We'll get rid again of this ugliness
> shortly.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c               | 35 ++++++++++++++---------------------
>  block/mirror.c        | 17 ++++++++++++-----
>  block/stream.c        | 21 ---------------------
>  block/vvfat.c         |  6 +++++-
>  include/block/block.h |  1 +
>  5 files changed, 32 insertions(+), 48 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fb94567..743f82e 100644
> --- a/block.c
> +++ b/block.c
> @@ -1094,7 +1094,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState 
> *parent_bs,
>      return child;
>  }
>  
> -static void bdrv_detach_child(BdrvChild *child)
> +void bdrv_detach_child(BdrvChild *child)
>  {
>      QLIST_REMOVE(child, next);
>      g_free(child);
> @@ -1112,13 +1112,20 @@ void bdrv_unref_child(BlockDriverState *parent, 
> BdrvChild *child)
>      bdrv_unref(child_bs);
>  }
>  
> +/*
> + * Sets the backing file link of a BDS. A new reference is created; callers
> + * which don't need their own reference any more must call bdrv_unref().
> + */
>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>  {
> +    if (backing_hd) {
> +        bdrv_ref(backing_hd);
> +    }
>  
>      if (bs->backing) {
>          assert(bs->backing_blocker);
>          bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker);
> -        bdrv_detach_child(bs->backing);
> +        bdrv_unref_child(bs, bs->backing);
>      } else if (backing_hd) {
>          error_setg(&bs->backing_blocker,
>                     "node is used as backing hd of '%s'",
> @@ -1214,7 +1221,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
> *options, Error **errp)
>          goto free_exit;
>      }
>  
> +    /* Hook up the backing file link; drop our reference, bs owns the
> +     * backing_hd reference now */
>      bdrv_set_backing_hd(bs, backing_hd);
> +    bdrv_unref(backing_hd);
>  
>  free_exit:
>      g_free(backing_filename);
> @@ -1884,11 +1894,7 @@ void bdrv_close(BlockDriverState *bs)
>  
>          bs->drv->bdrv_close(bs);
>  
> -        if (bs->backing) {
> -            BlockDriverState *backing_hd = bs->backing->bs;
> -            bdrv_set_backing_hd(bs, NULL);
> -            bdrv_unref(backing_hd);
> -        }
> +        bdrv_set_backing_hd(bs, NULL);
>  
>          if (bs->file != NULL) {
>              bdrv_unref(bs->file->bs);
> @@ -2427,12 +2433,9 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
> BlockDriverState *top,
>      BlockDriverState *intermediate;
>      BlockDriverState *base_bs = NULL;
>      BlockDriverState *new_top_bs = NULL;
> -    BlkIntermediateStates *intermediate_state, *next;
> +    BlkIntermediateStates *intermediate_state;
>      int ret = -EIO;
>  
> -    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
> -    QSIMPLEQ_INIT(&states_to_delete);
> -
>      if (!top->drv || !base->drv) {
>          goto exit;
>      }
> @@ -2459,7 +2462,6 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
> BlockDriverState *top,
>      while (intermediate) {
>          intermediate_state = g_new0(BlkIntermediateStates, 1);
>          intermediate_state->bs = intermediate;
> -        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
>  
>          if (backing_bs(intermediate) == base) {
>              base_bs = backing_bs(intermediate);
> @@ -2482,17 +2484,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
> BlockDriverState *top,
>      }
>      bdrv_set_backing_hd(new_top_bs, base_bs);
>  
> -    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, 
> next) {
> -        /* so that bdrv_close() does not recursively close the chain */
> -        bdrv_set_backing_hd(intermediate_state->bs, NULL);
> -        bdrv_unref(intermediate_state->bs);
> -    }
>      ret = 0;
> -
>  exit:
> -    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, 
> next) {
> -        g_free(intermediate_state);
> -    }
>      return ret;
>  }
>  

To me that begs the question why before we didn't simply
bdrv_ref(base_bs), and then bdrv_unref(top). Well, now it's even
simpler, so that's good.

> diff --git a/block/mirror.c b/block/mirror.c
> index 259e11a..571da27 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -370,11 +370,18 @@ static void mirror_exit(BlockJob *job, void *opaque)
>          bdrv_swap(s->target, to_replace);
>          if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
>              /* drop the bs loop chain formed by the swap: break the loop then
> -             * trigger the unref from the top one */
> -            BlockDriverState *p = s->base->backing
> -                                ? s->base->backing->bs : NULL;
> -            bdrv_set_backing_hd(s->base, NULL);
> -            bdrv_unref(p);
> +             * trigger the unref */
> +            /* FIXME This duplicates bdrv_set_backing_hd(), except for the
> +             * actual detach/unref so that the loop can be broken. When
> +             * bdrv_swap() gets replaced, this will become sane again. */
> +            BlockDriverState *backing = s->base->backing->bs;
> +            assert(s->base->backing_blocker);
> +            bdrv_op_unblock_all(s->base->backing->bs, 
> s->base->backing_blocker);

I know it's dropped later on anwyay, but I can't stop my fingers from
suggesting s/s->base->backing->bs/backing/.

Anyway, with or without that:

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature