| Message ID | 20211005143215.29500-16-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
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; > } >
On Tue, Oct 05, 2021 at 10:32:05AM -0400, 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(+) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 + * 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; }
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(+)