Re: [RFC PATCH v2 15/25] include/block/snapshot: global state API + assertions

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/10/21 16:32, Emanuele Giuseppe Esposito wrote:
> Snapshots run also under the BQL lock, so they all are
> in the global state API. The aiocontext lock that they hold
> is currently an overkill and in future could be removed.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/snapshot.c         | 28 ++++++++++++++++++++++++++++
>   include/block/snapshot.h | 21 +++++++++++++++++++++
>   migration/savevm.c       |  2 ++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index ccacda8bd5..e8756f7f90 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -57,6 +57,8 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>       QEMUSnapshotInfo *sn_tab, *sn;
>       int nb_sns, i, ret;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       ret = -ENOENT;
>       nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>       if (nb_sns < 0) {
> @@ -105,6 +107,7 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>       bool ret = false;
>   
>       assert(id || name);
> +    g_assert(qemu_in_main_thread());
>   
>       nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>       if (nb_sns < 0) {
> @@ -200,6 +203,7 @@ static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
>   int bdrv_can_snapshot(BlockDriverState *bs)
>   {
>       BlockDriver *drv = bs->drv;
> +    g_assert(qemu_in_main_thread());
>       if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>           return 0;
>       }
> @@ -220,6 +224,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>   {
>       BlockDriver *drv = bs->drv;
>       BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
> +
> +    g_assert(qemu_in_main_thread());
> +
>       if (!drv) {
>           return -ENOMEDIUM;
>       }
> @@ -240,6 +247,8 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>       BdrvChild **fallback_ptr;
>       int ret, open_ret;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (!drv) {
>           error_setg(errp, "Block driver is closed");
>           return -ENOMEDIUM;
> @@ -348,6 +357,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
>       BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
>       int ret;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (!drv) {
>           error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
>           return -ENOMEDIUM;
> @@ -380,6 +391,8 @@ int bdrv_snapshot_list(BlockDriverState *bs,
>   {
>       BlockDriver *drv = bs->drv;
>       BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
> +
> +    g_assert(qemu_in_main_thread());
>       if (!drv) {
>           return -ENOMEDIUM;
>       }
> @@ -419,6 +432,8 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>   {
>       BlockDriver *drv = bs->drv;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (!drv) {
>           error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
>           return -ENOMEDIUM;
> @@ -447,6 +462,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>       int ret;
>       Error *local_err = NULL;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       ret = bdrv_snapshot_load_tmp(bs, id_or_name, NULL, &local_err);
>       if (ret == -ENOENT || ret == -EINVAL) {
>           error_free(local_err);
> @@ -515,6 +532,8 @@ bool bdrv_all_can_snapshot(bool has_devices, strList *devices,
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return false;
>       }
> @@ -549,6 +568,8 @@ int bdrv_all_delete_snapshot(const char *name,
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return -1;
>       }
> @@ -588,6 +609,8 @@ int bdrv_all_goto_snapshot(const char *name,
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return -1;
>       }
> @@ -622,6 +645,8 @@ int bdrv_all_has_snapshot(const char *name,
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return -1;
>       }
> @@ -663,6 +688,7 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>   {
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
> +    g_assert(qemu_in_main_thread());
>   
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return -1;
> @@ -703,6 +729,8 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
>       g_autoptr(GList) bdrvs = NULL;
>       GList *iterbdrvs;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
>           return NULL;
>       }
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 940345692f..3a84849388 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -45,6 +45,27 @@ typedef struct QEMUSnapshotInfo {
>       uint64_t icount; /* record/replay step */
>   } QEMUSnapshotInfo;
>   
> +/*
> + * Global state (GS) API. These functions run under the BQL lock.
> + *
> + * If a function modifies the graph, it also uses drain and/or
> + * aio_context_acquire/release to be sure it has unique access.
> + * aio_context locking is needed together with BQL because of
> + * the thread-safe I/O API that concurrently runs and accesses
> + * the graph without the BQL.
> + *
> + * It is important to note that not all of these functions are
> + * necessarily limited to running under the BQL, but they would
> + * require additional auditing and may small thread-safety changes

Apart from the usual comment about s/may/many/,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> + * to move them into the I/O API. Often it's not worth doing that
> + * work since the APIs are only used with the BQL held at the
> + * moment, so they have been placed in the GS API (for now).
> + *
> + * All functions in this header must use this assertion:
> + * g_assert(qemu_in_main_thread());
> + * to catch when they are accidentally called without the BQL.
> + */
> +
>   int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                          const char *name);
>   bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7b7b64bd13..439f736ad5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2785,6 +2785,8 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>       g_autoptr(GDateTime) now = g_date_time_new_now_local();
>       AioContext *aio_context;
>   
> +    g_assert(qemu_in_main_thread());
> +
>       if (migration_is_blocked(errp)) {
>           return false;
>       }
>