Re: [RFC PATCH v2 5/8] block.c: wrlock in bdrv_replace_child_noperm

[Stefan Hajnoczi]

On Tue, Apr 26, 2022 at 04:51:11AM -0400, Emanuele Giuseppe Esposito wrote:
> The only problem here is ->attach and ->detach callbacks
> could call bdrv_{un}apply_subtree_drain(), which itself
> will use a rdlock to navigate through all nodes.
> To avoid deadlocks, take the lock only outside the drains,
> and if we need to both attach and detach, do it in a single
> critical section.
> 
> Therefore change ->attach and ->detach to return true if they
> are modifying the lock, so that we don't take it twice or release
> temporarly.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block.c                          | 31 +++++++++++++++++++++++++++----
>  block/block-backend.c            |  6 ++++--
>  include/block/block_int-common.h |  8 ++++++--
>  3 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b2eb679abb..6cd87e8dd3 100644
> --- a/block.c
> +++ b/block.c
> @@ -1434,21 +1434,26 @@ static void bdrv_inherited_options(BdrvChildRole 
> role, bool parent_is_format,
>      *child_flags = flags;
>  }
>  
> -static void bdrv_child_cb_attach(BdrvChild *child)
> +static bool bdrv_child_cb_attach(BdrvChild *child)
>  {
>      BlockDriverState *bs = child->opaque;
>  
>      assert_bdrv_graph_writable(bs);
>      QLIST_INSERT_HEAD(&bs->children, child, next);
>  
> +    /* Paired with bdrv_graph_wrlock() in bdrv_replace_child_noperm */
> +    bdrv_graph_wrunlock();
> +
>      if (child->role & BDRV_CHILD_COW) {
>          bdrv_backing_attach(child);
>      }
>  
>      bdrv_apply_subtree_drain(child, bs);
> +
> +    return true;
>  }
>  
> -static void bdrv_child_cb_detach(BdrvChild *child)
> +static bool bdrv_child_cb_detach(BdrvChild *child)
>  {
>      BlockDriverState *bs = child->opaque;
>  
> @@ -1458,8 +1463,13 @@ static void bdrv_child_cb_detach(BdrvChild *child)
>  
>      bdrv_unapply_subtree_drain(child, bs);
>  
> +    /* Paired with bdrv_graph_wrunlock() in bdrv_replace_child_noperm */
> +    bdrv_graph_wrlock();
> +
>      assert_bdrv_graph_writable(bs);
>      QLIST_REMOVE(child, next);
> +
> +    return true;
>  }
>  
>  static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState 
> *base,
> @@ -2842,6 +2852,7 @@ static void bdrv_replace_child_noperm(BdrvChild 
> **childp,
>      BlockDriverState *old_bs = child->bs;
>      int new_bs_quiesce_counter;
>      int drain_saldo;
> +    bool locked = false;
>  
>      assert(!child->frozen);
>      assert(old_bs != new_bs);
> @@ -2868,8 +2879,12 @@ static void bdrv_replace_child_noperm(BdrvChild 
> **childp,
>           * are already gone and we only end the drain sections that came from
>           * elsewhere. */
>          if (child->klass->detach) {
> -            child->klass->detach(child);
> +            locked = child->klass->detach(child);
> +        }
> +        if (!locked) {
> +            bdrv_graph_wrlock();
>          }
> +        locked = true;
>          assert_bdrv_graph_writable(old_bs);
>          QLIST_REMOVE(child, next_parent);
>      }
> @@ -2880,6 +2895,10 @@ static void bdrv_replace_child_noperm(BdrvChild 
> **childp,
>      }
>  
>      if (new_bs) {
> +        if (!locked) {
> +            bdrv_graph_wrlock();
> +            locked = true;
> +        }
>          assert_bdrv_graph_writable(new_bs);
>          QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
>  
> @@ -2896,10 +2915,14 @@ static void bdrv_replace_child_noperm(BdrvChild 
> **childp,
>           * drain sections coming from @child don't get an extra 
> .drained_begin
>           * callback. */
>          if (child->klass->attach) {
> -            child->klass->attach(child);
> +            locked = !(child->klass->attach(child));

O_O I don't understand what the return value of ->attach() means. It has
the opposite meaning to the return value of ->detach()?

>          }
>      }
>  
> +    if (locked) {
> +        bdrv_graph_wrunlock();
> +    }
> +
>      /*
>       * If the old child node was drained but the new one is not, allow
>       * requests to come in only after the new node has been attached.
> diff --git a/block/block-backend.c b/block/block-backend.c
> index e0e1aff4b1..5dbd9fceae 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -282,7 +282,7 @@ static int blk_root_inactivate(BdrvChild *child)
>      return 0;
>  }
>  
> -static void blk_root_attach(BdrvChild *child)
> +static bool blk_root_attach(BdrvChild *child)
>  {
>      BlockBackend *blk = child->opaque;
>      BlockBackendAioNotifier *notifier;
> @@ -295,9 +295,10 @@ static void blk_root_attach(BdrvChild *child)
>                  notifier->detach_aio_context,
>                  notifier->opaque);
>      }
> +    return false;
>  }
>  
> -static void blk_root_detach(BdrvChild *child)
> +static bool blk_root_detach(BdrvChild *child)
>  {
>      BlockBackend *blk = child->opaque;
>      BlockBackendAioNotifier *notifier;
> @@ -310,6 +311,7 @@ static void blk_root_detach(BdrvChild *child)
>                  notifier->detach_aio_context,
>                  notifier->opaque);
>      }
> +    return false;
>  }
>  
>  static AioContext *blk_root_get_parent_aio_context(BdrvChild *c)
> diff --git a/include/block/block_int-common.h 
> b/include/block/block_int-common.h
> index 5a04c778e4..dd058c1fd8 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -857,8 +857,12 @@ struct BdrvChildClass {
>      void (*activate)(BdrvChild *child, Error **errp);
>      int (*inactivate)(BdrvChild *child);
>  
> -    void (*attach)(BdrvChild *child);
> -    void (*detach)(BdrvChild *child);
> +    /*
> +     * Return true if the graph wrlock is taken/released,

What does "taken/released" mean? Does it mean released by attach and
taken by detach?

Also, please document which locks are held when these callbacks are
invoked.

> +     * false if the wrlock state is not changed.
> +     */
> +    bool (*attach)(BdrvChild *child);
> +    bool (*detach)(BdrvChild *child);
>  
>      /*
>       * Notifies the parent that the filename of its child has changed (e.g.
> -- 
> 2.31.1
>

signature.asc
Description: PGP signature