Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 11520 bytes --]

Il mar 25 apr 2023, 19:32 Kevin Wolf <kwolf@redhat.com> ha scritto:

> These functions must not be called in coroutine context, because they
> need write access to the graph.
>

With these patches applied vrc is still complaining about calls to
bdrv_unref_child from qcow2_do_open and qcow2_do_close.

Otherwise, the situation looks pretty good.

Paolo

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h          |  3 ++-
>  include/sysemu/block-backend-global-state.h |  5 ++++-
>  block.c                                     |  2 +-
>  block/crypto.c                              |  6 +++---
>  block/parallels.c                           |  6 +++---
>  block/qcow.c                                |  6 +++---
>  block/qcow2.c                               | 14 +++++++-------
>  block/qed.c                                 |  6 +++---
>  block/vdi.c                                 |  6 +++---
>  block/vhdx.c                                |  6 +++---
>  block/vmdk.c                                | 18 +++++++++---------
>  block/vpc.c                                 |  6 +++---
>  12 files changed, 44 insertions(+), 40 deletions(-)
>
> diff --git a/include/block/block-global-state.h
> b/include/block/block-global-state.h
> index 2c312cc774..ec3ddb17a8 100644
> --- a/include/block/block-global-state.h
> +++ b/include/block/block-global-state.h
> @@ -218,7 +218,8 @@ void bdrv_img_create(const char *filename, const char
> *fmt,
>                       bool quiet, Error **errp);
>
>  void bdrv_ref(BlockDriverState *bs);
> -void bdrv_unref(BlockDriverState *bs);
> +void no_coroutine_fn bdrv_unref(BlockDriverState *bs);
> +void coroutine_fn no_co_wrapper bdrv_co_unref(BlockDriverState *bs);
>  void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
>  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                               BlockDriverState *child_bs,
> diff --git a/include/sysemu/block-backend-global-state.h
> b/include/sysemu/block-backend-global-state.h
> index 2b6d27db7c..fa83f9389c 100644
> --- a/include/sysemu/block-backend-global-state.h
> +++ b/include/sysemu/block-backend-global-state.h
> @@ -42,7 +42,10 @@ blk_co_new_open(const char *filename, const char
> *reference, QDict *options,
>
>  int blk_get_refcnt(BlockBackend *blk);
>  void blk_ref(BlockBackend *blk);
> -void blk_unref(BlockBackend *blk);
> +
> +void no_coroutine_fn blk_unref(BlockBackend *blk);
> +void coroutine_fn no_co_wrapper blk_co_unref(BlockBackend *blk);
> +
>  void blk_remove_all_bs(void);
>  BlockBackend *blk_by_name(const char *name);
>  BlockBackend *blk_next(BlockBackend *blk);
> diff --git a/block.c b/block.c
> index 5ec1a3897e..20d5ee0959 100644
> --- a/block.c
> +++ b/block.c
> @@ -680,7 +680,7 @@ int coroutine_fn
> bdrv_co_create_opts_simple(BlockDriver *drv,
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      return ret;
>  }
>
> diff --git a/block/crypto.c b/block/crypto.c
> index ca67289187..8fd3ad0054 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -355,7 +355,7 @@ block_crypto_co_create_generic(BlockDriverState *bs,
> int64_t size,
>      ret = 0;
>   cleanup:
>      qcrypto_block_free(crypto);
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      return ret;
>  }
>
> @@ -661,7 +661,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions
> *create_options, Error **errp)
>
>      ret = 0;
>  fail:
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -730,7 +730,7 @@ fail:
>          bdrv_co_delete_file_noerr(bs);
>      }
>
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_QCryptoBlockCreateOptions(create_opts);
>      qobject_unref(cryptoopts);
>      return ret;
> diff --git a/block/parallels.c b/block/parallels.c
> index 013684801a..b49c35929e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -613,8 +613,8 @@ static int coroutine_fn
> parallels_co_create(BlockdevCreateOptions* opts,
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>
>  exit:
> @@ -691,7 +691,7 @@ parallels_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  done:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qcow.c b/block/qcow.c
> index 490e4f819e..a0c701f578 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -915,8 +915,8 @@ static int coroutine_fn
> qcow_co_create(BlockdevCreateOptions *opts,
>      g_free(tmp);
>      ret = 0;
>  exit:
> -    blk_unref(qcow_blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(qcow_blk);
> +    bdrv_co_unref(bs);
>      qcrypto_block_free(crypto);
>      return ret;
>  }
> @@ -1015,7 +1015,7 @@ qcow_co_create_opts(BlockDriver *drv, const char
> *filename,
>  fail:
>      g_free(backing_fmt);
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 94cf59af8b..01742b3ebe 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3705,7 +3705,7 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>          goto out;
>      }
>
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      blk = NULL;
>
>      /*
> @@ -3785,7 +3785,7 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>          }
>      }
>
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      blk = NULL;
>
>      /* Reopen the image without BDRV_O_NO_FLUSH to flush it before
> returning.
> @@ -3810,9 +3810,9 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> -    bdrv_unref(data_bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
> +    bdrv_co_unref(data_bs);
>      return ret;
>  }
>
> @@ -3943,8 +3943,8 @@ finish:
>      }
>
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> -    bdrv_unref(data_bs);
> +    bdrv_co_unref(bs);
> +    bdrv_co_unref(data_bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qed.c b/block/qed.c
> index 0705a7b4e2..aff2a2076e 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -748,8 +748,8 @@ static int coroutine_fn
> bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      ret = 0; /* success */
>  out:
>      g_free(l1_table);
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -819,7 +819,7 @@ bdrv_qed_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/vdi.c b/block/vdi.c
> index f2434d6153..08331d2dd7 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -886,8 +886,8 @@ static int coroutine_fn
> vdi_co_do_create(BlockdevCreateOptions *create_options,
>
>      ret = 0;
>  exit:
> -    blk_unref(blk);
> -    bdrv_unref(bs_file);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs_file);
>      g_free(bmap);
>      return ret;
>  }
> @@ -975,7 +975,7 @@ vdi_co_create_opts(BlockDriver *drv, const char
> *filename,
>  done:
>      qobject_unref(qdict);
>      qapi_free_BlockdevCreateOptions(create_options);
> -    bdrv_unref(bs_file);
> +    bdrv_co_unref(bs_file);
>      return ret;
>  }
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 81420722a1..00777da91a 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2053,8 +2053,8 @@ static int coroutine_fn
> vhdx_co_create(BlockdevCreateOptions *opts,
>
>      ret = 0;
>  delete_and_exit:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      g_free(creator);
>      return ret;
>  }
> @@ -2144,7 +2144,7 @@ vhdx_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3f8c731e32..11b553ef25 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2306,7 +2306,7 @@ exit:
>          if (pbb) {
>              *pbb = blk;
>          } else {
> -            blk_unref(blk);
> +            blk_co_unref(blk);
>              blk = NULL;
>          }
>      }
> @@ -2516,12 +2516,12 @@ vmdk_co_do_create(int64_t size,
>          if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
>              error_setg(errp, "Invalid backing file format: %s. Must be
> vmdk",
>                         blk_bs(backing)->drv->format_name);
> -            blk_unref(backing);
> +            blk_co_unref(backing);
>              ret = -EINVAL;
>              goto exit;
>          }
>          ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
> -        blk_unref(backing);
> +        blk_co_unref(backing);
>          if (ret) {
>              error_setg(errp, "Failed to read parent CID");
>              goto exit;
> @@ -2542,14 +2542,14 @@ vmdk_co_do_create(int64_t size,
>                               blk_bs(extent_blk)->filename);
>          created_size += cur_size;
>          extent_idx++;
> -        blk_unref(extent_blk);
> +        blk_co_unref(extent_blk);
>      }
>
>      /* Check whether we got excess extents */
>      extent_blk = extent_fn(-1, extent_idx, flat, split, compress,
> zeroed_grain,
>                             opaque, NULL);
>      if (extent_blk) {
> -        blk_unref(extent_blk);
> +        blk_co_unref(extent_blk);
>          error_setg(errp, "List of extents contains unused extents");
>          ret = -EINVAL;
>          goto exit;
> @@ -2590,7 +2590,7 @@ vmdk_co_do_create(int64_t size,
>      ret = 0;
>  exit:
>      if (blk) {
> -        blk_unref(blk);
> +        blk_co_unref(blk);
>      }
>      g_free(desc);
>      g_free(parent_desc_line);
> @@ -2641,7 +2641,7 @@ vmdk_co_create_opts_cb(int64_t size, int idx, bool
> flat, bool split,
>                             errp)) {
>          goto exit;
>      }
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>  exit:
>      g_free(ext_filename);
>      return blk;
> @@ -2797,12 +2797,12 @@ static BlockBackend * coroutine_fn
> vmdk_co_create_cb(int64_t size, int idx,
>          return NULL;
>      }
>      blk_set_allow_write_beyond_eof(blk, true);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>
>      if (size != -1) {
>          ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain,
> errp);
>          if (ret) {
> -            blk_unref(blk);
> +            blk_co_unref(blk);
>              blk = NULL;
>          }
>      }
> diff --git a/block/vpc.c b/block/vpc.c
> index b89b0ff8e2..07ddda5b99 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -1082,8 +1082,8 @@ static int coroutine_fn
> vpc_co_create(BlockdevCreateOptions *opts,
>      }
>
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -1162,7 +1162,7 @@ vpc_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> --
> 2.40.0
>
>

[-- Attachment #2: Type: text/html, Size: 14192 bytes --]