* [PATCH 1/5] crypto: perform permission checks under BQL
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
@ 2022-02-09 10:54 ` Emanuele Giuseppe Esposito
2022-03-01 9:32 ` Kevin Wolf
2022-02-09 10:54 ` [PATCH 2/5] crypto: distinguish between main loop and I/O in block_crypto_amend_options_generic_luks Emanuele Giuseppe Esposito
` (4 subsequent siblings)
5 siblings, 1 reply; 8+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-02-09 10:54 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Emanuele Giuseppe Esposito, Denis V. Lunev,
Dr. David Alan Gilbert, David Gibson
Move the permission API calls into driver-specific callbacks
that always run under BQL. In this case, bdrv_crypto_luks
needs to perform permission checks before and after
qcrypto_block_amend_options(). The problem is that the caller,
block_crypto_amend_options_generic_luks(), can also run in I/O
from .bdrv_co_amend(). This does not comply with Global State-I/O API split,
as permissions API must always run under BQL.
Firstly, introduce .bdrv_amend_pre_run() and .bdrv_amend_clean()
callbacks. These two callbacks are guaranteed to be invoked under
BQL, respectively before and after .bdrv_co_amend().
They take care of performing the permission checks
in the same way as they are currently done before and after
qcrypto_block_amend_options().
These callbacks are in preparation for next patch, where we
delete the original permission check. Right now they just add redundant
control.
Then, call .bdrv_amend_pre_run() before job_start in
qmp_x_blockdev_amend(), so that it will be run before the job coroutine
is created and stay in the main loop.
As a cleanup, use JobDriver's .clean() callback to call
.bdrv_amend_clean(), and run amend-specific cleanup callbacks under BQL.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/amend.c | 24 ++++++++++++++++++++++++
block/crypto.c | 27 +++++++++++++++++++++++++++
include/block/block_int.h | 14 ++++++++++++++
3 files changed, 65 insertions(+)
diff --git a/block/amend.c b/block/amend.c
index 392df9ef83..329bca53dc 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -53,10 +53,29 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
return ret;
}
+static int blockdev_amend_pre_run(BlockdevAmendJob *s, Error **errp)
+{
+ if (s->bs->drv->bdrv_amend_pre_run) {
+ return s->bs->drv->bdrv_amend_pre_run(s->bs, errp);
+ }
+
+ return 0;
+}
+
+static void blockdev_amend_clean(Job *job)
+{
+ BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
+
+ if (s->bs->drv->bdrv_amend_clean) {
+ s->bs->drv->bdrv_amend_clean(s->bs);
+ }
+}
+
static const JobDriver blockdev_amend_job_driver = {
.instance_size = sizeof(BlockdevAmendJob),
.job_type = JOB_TYPE_AMEND,
.run = blockdev_amend_run,
+ .clean = blockdev_amend_clean,
};
void qmp_x_blockdev_amend(const char *job_id,
@@ -113,5 +132,10 @@ void qmp_x_blockdev_amend(const char *job_id,
s->bs = bs,
s->opts = QAPI_CLONE(BlockdevAmendOptions, options),
s->force = has_force ? force : false;
+
+ if (blockdev_amend_pre_run(s, errp)) {
+ return;
+ }
+
job_start(&s->common);
}
diff --git a/block/crypto.c b/block/crypto.c
index c8ba4681e2..59f768ea8d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -777,6 +777,31 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
return spec_info;
}
+static int
+block_crypto_amend_prepare(BlockDriverState *bs, Error **errp)
+{
+ BlockCrypto *crypto = bs->opaque;
+
+ /* apply for exclusive read/write permissions to the underlying file*/
+ crypto->updating_keys = true;
+ return bdrv_child_refresh_perms(bs, bs->file, errp);
+}
+
+static void
+block_crypto_amend_cleanup(BlockDriverState *bs)
+{
+ BlockCrypto *crypto = bs->opaque;
+ Error *errp = NULL;
+
+ /* release exclusive read/write permissions to the underlying file*/
+ crypto->updating_keys = false;
+ bdrv_child_refresh_perms(bs, bs->file, &errp);
+
+ if (errp) {
+ error_report_err(errp);
+ }
+}
+
static int
block_crypto_amend_options_generic_luks(BlockDriverState *bs,
QCryptoBlockAmendOptions *amend_options,
@@ -931,6 +956,8 @@ static BlockDriver bdrv_crypto_luks = {
.bdrv_get_specific_info = block_crypto_get_specific_info_luks,
.bdrv_amend_options = block_crypto_amend_options_luks,
.bdrv_co_amend = block_crypto_co_amend_luks,
+ .bdrv_amend_pre_run = block_crypto_amend_prepare,
+ .bdrv_amend_clean = block_crypto_amend_cleanup,
.is_format = true,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 27008cfb22..31bd788919 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -124,6 +124,20 @@ struct BlockDriver {
* on those children.
*/
bool is_format;
+
+ /*
+ * This function is invoked under BQL before .bdrv_co_amend()
+ * (which in contrast does not necessarily run under the BQL)
+ * to allow driver-specific initialization code that requires
+ * the BQL, like setting up specific permission flags.
+ */
+ int (*bdrv_amend_pre_run)(BlockDriverState *bs, Error **errp);
+ /*
+ * This function is invoked under BQL after .bdrv_co_amend()
+ * to allow cleaning up what was done in .bdrv_amend_pre_run().
+ */
+ void (*bdrv_amend_clean)(BlockDriverState *bs);
+
/*
* Return true if @to_replace can be replaced by a BDS with the
* same data as @bs without it affecting @bs's behavior (that is,
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/5] crypto: perform permission checks under BQL
2022-02-09 10:54 ` [PATCH 1/5] crypto: perform permission checks under BQL Emanuele Giuseppe Esposito
@ 2022-03-01 9:32 ` Kevin Wolf
0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2022-03-01 9:32 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Fam Zheng, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Denis V. Lunev, Dr. David Alan Gilbert,
David Gibson
Am 09.02.2022 um 11:54 hat Emanuele Giuseppe Esposito geschrieben:
> Move the permission API calls into driver-specific callbacks
> that always run under BQL. In this case, bdrv_crypto_luks
> needs to perform permission checks before and after
> qcrypto_block_amend_options(). The problem is that the caller,
> block_crypto_amend_options_generic_luks(), can also run in I/O
> from .bdrv_co_amend(). This does not comply with Global State-I/O API split,
> as permissions API must always run under BQL.
>
> Firstly, introduce .bdrv_amend_pre_run() and .bdrv_amend_clean()
> callbacks. These two callbacks are guaranteed to be invoked under
> BQL, respectively before and after .bdrv_co_amend().
> They take care of performing the permission checks
> in the same way as they are currently done before and after
> qcrypto_block_amend_options().
> These callbacks are in preparation for next patch, where we
> delete the original permission check. Right now they just add redundant
> control.
>
> Then, call .bdrv_amend_pre_run() before job_start in
> qmp_x_blockdev_amend(), so that it will be run before the job coroutine
> is created and stay in the main loop.
> As a cleanup, use JobDriver's .clean() callback to call
> .bdrv_amend_clean(), and run amend-specific cleanup callbacks under BQL.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> block/amend.c | 24 ++++++++++++++++++++++++
> block/crypto.c | 27 +++++++++++++++++++++++++++
> include/block/block_int.h | 14 ++++++++++++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/block/amend.c b/block/amend.c
> index 392df9ef83..329bca53dc 100644
> --- a/block/amend.c
> +++ b/block/amend.c
> @@ -53,10 +53,29 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
> return ret;
> }
>
> +static int blockdev_amend_pre_run(BlockdevAmendJob *s, Error **errp)
> +{
> + if (s->bs->drv->bdrv_amend_pre_run) {
> + return s->bs->drv->bdrv_amend_pre_run(s->bs, errp);
> + }
> +
> + return 0;
> +}
> +
> +static void blockdev_amend_clean(Job *job)
> +{
> + BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
> +
> + if (s->bs->drv->bdrv_amend_clean) {
> + s->bs->drv->bdrv_amend_clean(s->bs);
> + }
> +}
> +
> static const JobDriver blockdev_amend_job_driver = {
> .instance_size = sizeof(BlockdevAmendJob),
> .job_type = JOB_TYPE_AMEND,
> .run = blockdev_amend_run,
> + .clean = blockdev_amend_clean,
> };
>
> void qmp_x_blockdev_amend(const char *job_id,
> @@ -113,5 +132,10 @@ void qmp_x_blockdev_amend(const char *job_id,
> s->bs = bs,
> s->opts = QAPI_CLONE(BlockdevAmendOptions, options),
> s->force = has_force ? force : false;
> +
> + if (blockdev_amend_pre_run(s, errp)) {
> + return;
> + }
> +
> job_start(&s->common);
> }
> diff --git a/block/crypto.c b/block/crypto.c
> index c8ba4681e2..59f768ea8d 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -777,6 +777,31 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
> return spec_info;
> }
>
> +static int
> +block_crypto_amend_prepare(BlockDriverState *bs, Error **errp)
> +{
> + BlockCrypto *crypto = bs->opaque;
> +
> + /* apply for exclusive read/write permissions to the underlying file*/
Missing space before the end of the comment.
> + crypto->updating_keys = true;
> + return bdrv_child_refresh_perms(bs, bs->file, errp);
> +}
> +
> +static void
> +block_crypto_amend_cleanup(BlockDriverState *bs)
> +{
> + BlockCrypto *crypto = bs->opaque;
> + Error *errp = NULL;
> +
> + /* release exclusive read/write permissions to the underlying file*/
And here.
I can fix this up while applying.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/5] crypto: distinguish between main loop and I/O in block_crypto_amend_options_generic_luks
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 1/5] crypto: perform permission checks under BQL Emanuele Giuseppe Esposito
@ 2022-02-09 10:54 ` Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 3/5] block: introduce bdrv_activate Emanuele Giuseppe Esposito
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-02-09 10:54 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Emanuele Giuseppe Esposito, Denis V. Lunev,
Dr. David Alan Gilbert, David Gibson
block_crypto_amend_options_generic_luks uses the block layer
permission API, therefore it should be called with the BQL held.
However, the same function is being called by two BlockDriver
callbacks: bdrv_amend_options (under BQL) and bdrv_co_amend (I/O).
The latter is I/O because it is invoked by block/amend.c's
blockdev_amend_run(), a .run callback of the amend JobDriver.
Therefore we want to change this function to still perform
the permission check, but making sure it is done under BQL regardless
of the caller context.
Remove the permission check in block_crypto_amend_options_generic_luks()
and:
- in block_crypto_amend_options_luks() (BQL case, called by
.bdrv_amend_options()), reuse helper functions
block_crypto_amend_{prepare/cleanup} that take care of checking
permissions.
- for block_crypto_co_amend_luks() (I/O case, called by
.bdrv_co_amend()), don't check for permissions but delegate
.bdrv_amend_pre_run() and .bdrv_amend_clean() to do it,
performing these checks before and after the job runs in its aiocontext.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/crypto.c | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index 59f768ea8d..c546b96dbd 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -809,30 +809,17 @@ block_crypto_amend_options_generic_luks(BlockDriverState *bs,
Error **errp)
{
BlockCrypto *crypto = bs->opaque;
- int ret;
assert(crypto);
assert(crypto->block);
- /* apply for exclusive read/write permissions to the underlying file*/
- crypto->updating_keys = true;
- ret = bdrv_child_refresh_perms(bs, bs->file, errp);
- if (ret) {
- goto cleanup;
- }
-
- ret = qcrypto_block_amend_options(crypto->block,
- block_crypto_read_func,
- block_crypto_write_func,
- bs,
- amend_options,
- force,
- errp);
-cleanup:
- /* release exclusive read/write permissions to the underlying file*/
- crypto->updating_keys = false;
- bdrv_child_refresh_perms(bs, bs->file, errp);
- return ret;
+ return qcrypto_block_amend_options(crypto->block,
+ block_crypto_read_func,
+ block_crypto_write_func,
+ bs,
+ amend_options,
+ force,
+ errp);
}
static int
@@ -858,8 +845,16 @@ block_crypto_amend_options_luks(BlockDriverState *bs,
if (!amend_options) {
goto cleanup;
}
+
+ ret = block_crypto_amend_prepare(bs, errp);
+ if (ret) {
+ goto perm_cleanup;
+ }
ret = block_crypto_amend_options_generic_luks(bs, amend_options,
force, errp);
+
+perm_cleanup:
+ block_crypto_amend_cleanup(bs);
cleanup:
qapi_free_QCryptoBlockAmendOptions(amend_options);
return ret;
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/5] block: introduce bdrv_activate
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 1/5] crypto: perform permission checks under BQL Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 2/5] crypto: distinguish between main loop and I/O in block_crypto_amend_options_generic_luks Emanuele Giuseppe Esposito
@ 2022-02-09 10:54 ` Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 4/5] block: rename bdrv_invalidate_cache_all, blk_invalidate_cache and test_sync_op_invalidate_cache Emanuele Giuseppe Esposito
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-02-09 10:54 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Emanuele Giuseppe Esposito, Denis V. Lunev,
Dr. David Alan Gilbert, David Gibson
This function is currently just a wrapper for bdrv_invalidate_cache(),
but in future will contain the code of bdrv_co_invalidate_cache() that
has to always be protected by BQL, and leave the rest in the I/O
coroutine.
Replace all bdrv_invalidate_cache() invokations with bdrv_activate().
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
block.c | 7 ++++++-
block/block-backend.c | 2 +-
block/export/export.c | 2 +-
block/parallels.c | 2 +-
include/block/block.h | 1 +
tests/unit/test-block-iothread.c | 2 +-
6 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index b54d59d1fa..7746727a47 100644
--- a/block.c
+++ b/block.c
@@ -6393,6 +6393,11 @@ void bdrv_init_with_whitelist(void)
bdrv_init();
}
+int bdrv_activate(BlockDriverState *bs, Error **errp)
+{
+ return bdrv_invalidate_cache(bs, errp);
+}
+
int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
{
BdrvChild *child, *parent;
@@ -6478,7 +6483,7 @@ void bdrv_invalidate_cache_all(Error **errp)
int ret;
aio_context_acquire(aio_context);
- ret = bdrv_invalidate_cache(bs, errp);
+ ret = bdrv_activate(bs, errp);
aio_context_release(aio_context);
if (ret < 0) {
bdrv_next_cleanup(&it);
diff --git a/block/block-backend.c b/block/block-backend.c
index 4ff6b4d785..c516113a36 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1889,7 +1889,7 @@ void blk_invalidate_cache(BlockBackend *blk, Error **errp)
return;
}
- bdrv_invalidate_cache(bs, errp);
+ bdrv_activate(bs, errp);
}
bool blk_is_inserted(BlockBackend *blk)
diff --git a/block/export/export.c b/block/export/export.c
index 6d3b9964c8..7253af3bc3 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -139,7 +139,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
* access since the export could be available before migration handover.
* ctx was acquired in the caller.
*/
- bdrv_invalidate_cache(bs, NULL);
+ bdrv_activate(bs, NULL);
perm = BLK_PERM_CONSISTENT_READ;
if (export->writable) {
diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..e58c828422 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -873,7 +873,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
s->bat_dirty_bmap =
bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
- /* Disable migration until bdrv_invalidate_cache method is added */
+ /* Disable migration until bdrv_activate method is added */
error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
diff --git a/include/block/block.h b/include/block/block.h
index e1713ee306..18cd336911 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -494,6 +494,7 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
int generated_co_wrapper bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp);
void bdrv_invalidate_cache_all(Error **errp);
+int bdrv_activate(BlockDriverState *bs, Error **errp);
int bdrv_inactivate_all(void);
/* Ensure contents are flushed to disk. */
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index aea660aeed..378a7b7869 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -282,7 +282,7 @@ static void test_sync_op_check(BdrvChild *c)
static void test_sync_op_invalidate_cache(BdrvChild *c)
{
/* Early success: Image is not inactive */
- bdrv_invalidate_cache(c->bs, NULL);
+ bdrv_activate(c->bs, NULL);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/5] block: rename bdrv_invalidate_cache_all, blk_invalidate_cache and test_sync_op_invalidate_cache
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
` (2 preceding siblings ...)
2022-02-09 10:54 ` [PATCH 3/5] block: introduce bdrv_activate Emanuele Giuseppe Esposito
@ 2022-02-09 10:54 ` Emanuele Giuseppe Esposito
2022-02-09 10:54 ` [PATCH 5/5] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate Emanuele Giuseppe Esposito
2022-03-01 10:34 ` [PATCH 0/5] block layer: permission API refactoring in preparation Kevin Wolf
5 siblings, 0 replies; 8+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-02-09 10:54 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Emanuele Giuseppe Esposito, Denis V. Lunev,
Dr. David Alan Gilbert, David Gibson
Following the bdrv_activate renaming, change also the name
of the respective callers.
bdrv_invalidate_cache_all -> bdrv_activate_all
blk_invalidate_cache -> blk_activate
test_sync_op_invalidate_cache -> test_sync_op_activate
No functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
block.c | 2 +-
block/block-backend.c | 2 +-
hw/block/pflash_cfi01.c | 2 +-
hw/nvram/spapr_nvram.c | 2 +-
include/block/block.h | 2 +-
include/sysemu/block-backend.h | 2 +-
migration/block.c | 2 +-
migration/migration.c | 14 +++++++-------
migration/savevm.c | 6 +++---
monitor/qmp-cmds.c | 2 +-
tests/unit/test-block-iothread.c | 6 +++---
11 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index 7746727a47..5e65f134f8 100644
--- a/block.c
+++ b/block.c
@@ -6473,7 +6473,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
return 0;
}
-void bdrv_invalidate_cache_all(Error **errp)
+void bdrv_activate_all(Error **errp)
{
BlockDriverState *bs;
BdrvNextIterator it;
diff --git a/block/block-backend.c b/block/block-backend.c
index c516113a36..98bfcd5cf2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1880,7 +1880,7 @@ void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
blk->enable_write_cache = wce;
}
-void blk_invalidate_cache(BlockBackend *blk, Error **errp)
+void blk_activate(BlockBackend *blk, Error **errp)
{
BlockDriverState *bs = blk_bs(blk);
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 81f9f971d8..74c7190302 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -1023,7 +1023,7 @@ static void postload_update_cb(void *opaque, bool running, RunState state)
{
PFlashCFI01 *pfl = opaque;
- /* This is called after bdrv_invalidate_cache_all. */
+ /* This is called after bdrv_activate_all. */
qemu_del_vm_change_state_handler(pfl->vmstate);
pfl->vmstate = NULL;
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index fbfdf47e26..18b43be7f6 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -219,7 +219,7 @@ static void postload_update_cb(void *opaque, bool running, RunState state)
{
SpaprNvram *nvram = opaque;
- /* This is called after bdrv_invalidate_cache_all. */
+ /* This is called after bdrv_activate_all. */
qemu_del_vm_change_state_handler(nvram->vmstate);
nvram->vmstate = NULL;
diff --git a/include/block/block.h b/include/block/block.h
index 18cd336911..d27e616d29 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -493,8 +493,8 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
/* Invalidate any cached metadata used by image formats */
int generated_co_wrapper bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp);
-void bdrv_invalidate_cache_all(Error **errp);
int bdrv_activate(BlockDriverState *bs, Error **errp);
+void bdrv_activate_all(Error **errp);
int bdrv_inactivate_all(void);
/* Ensure contents are flushed to disk. */
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index e5e1524f06..6c5104cd4a 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -206,7 +206,7 @@ bool blk_is_writable(BlockBackend *blk);
bool blk_is_sg(BlockBackend *blk);
bool blk_enable_write_cache(BlockBackend *blk);
void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
-void blk_invalidate_cache(BlockBackend *blk, Error **errp);
+void blk_activate(BlockBackend *blk, Error **errp);
bool blk_is_inserted(BlockBackend *blk);
bool blk_is_available(BlockBackend *blk);
void blk_lock_medium(BlockBackend *blk, bool locked);
diff --git a/migration/block.c b/migration/block.c
index a950977855..077a413325 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -932,7 +932,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
return -EINVAL;
}
- blk_invalidate_cache(blk, &local_err);
+ blk_activate(blk, &local_err);
if (local_err) {
error_report_err(local_err);
return -EINVAL;
diff --git a/migration/migration.c b/migration/migration.c
index bcc385b94b..ad2e3a3cbd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -497,9 +497,9 @@ static void process_incoming_migration_bh(void *opaque)
if (!migrate_late_block_activate() ||
(autostart && (!global_state_received() ||
global_state_get_runstate() == RUN_STATE_RUNNING))) {
- /* Make sure all file formats flush their mutable metadata.
+ /* Make sure all file formats throw away their mutable metadata.
* If we get an error here, just don't restart the VM yet. */
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
local_err = NULL;
@@ -585,8 +585,8 @@ static void process_incoming_migration_co(void *opaque)
/* we get COLO info, and know if we are in COLO mode */
if (!ret && migration_incoming_colo_enabled()) {
- /* Make sure all file formats flush their mutable metadata */
- bdrv_invalidate_cache_all(&local_err);
+ /* Make sure all file formats throw away their mutable metadata */
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
goto fail;
@@ -1926,7 +1926,7 @@ static void migrate_fd_cancel(MigrationState *s)
if (s->state == MIGRATION_STATUS_CANCELLING && s->block_inactive) {
Error *local_err = NULL;
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
} else {
@@ -3105,7 +3105,7 @@ fail:
*/
Error *local_err = NULL;
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
}
@@ -3250,7 +3250,7 @@ fail_invalidate:
Error *local_err = NULL;
qemu_mutex_lock_iothread();
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
} else {
diff --git a/migration/savevm.c b/migration/savevm.c
index 1599b02fbc..4842462ca6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1438,7 +1438,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
if (inactivate_disks) {
/* Inactivate before sending QEMU_VM_EOF so that the
- * bdrv_invalidate_cache_all() on the other end won't fail. */
+ * bdrv_activate_all() on the other end won't fail. */
ret = bdrv_inactivate_all();
if (ret) {
error_report("%s: bdrv_inactivate_all() failed (%d)",
@@ -2006,9 +2006,9 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
qemu_announce_self(&mis->announce_timer, migrate_announce_params());
- /* Make sure all file formats flush their mutable metadata.
+ /* Make sure all file formats throw away their mutable metadata.
* If we get an error here, just don't restart the VM yet. */
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_report_err(local_err);
local_err = NULL;
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index db4d186448..206d9a8c7b 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -144,7 +144,7 @@ void qmp_cont(Error **errp)
* If there are no inactive block nodes (e.g. because the VM was just
* paused rather than completing a migration), bdrv_inactivate_all() simply
* doesn't do anything. */
- bdrv_invalidate_cache_all(&local_err);
+ bdrv_activate_all(&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 378a7b7869..94718c9319 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -279,7 +279,7 @@ static void test_sync_op_check(BdrvChild *c)
g_assert_cmpint(ret, ==, -ENOTSUP);
}
-static void test_sync_op_invalidate_cache(BdrvChild *c)
+static void test_sync_op_activate(BdrvChild *c)
{
/* Early success: Image is not inactive */
bdrv_activate(c->bs, NULL);
@@ -325,8 +325,8 @@ const SyncOpTest sync_op_tests[] = {
.name = "/sync-op/check",
.fn = test_sync_op_check,
}, {
- .name = "/sync-op/invalidate_cache",
- .fn = test_sync_op_invalidate_cache,
+ .name = "/sync-op/activate",
+ .fn = test_sync_op_activate,
},
};
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 5/5] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
` (3 preceding siblings ...)
2022-02-09 10:54 ` [PATCH 4/5] block: rename bdrv_invalidate_cache_all, blk_invalidate_cache and test_sync_op_invalidate_cache Emanuele Giuseppe Esposito
@ 2022-02-09 10:54 ` Emanuele Giuseppe Esposito
2022-03-01 10:34 ` [PATCH 0/5] block layer: permission API refactoring in preparation Kevin Wolf
5 siblings, 0 replies; 8+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-02-09 10:54 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Vladimir Sementsov-Ogievskiy,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Emanuele Giuseppe Esposito, Denis V. Lunev,
Dr. David Alan Gilbert, David Gibson
Split bdrv_co_invalidate cache in two: the Global State (under BQL)
code that takes care of permissions and running GS callbacks,
and leave only the I/O code (->bdrv_co_invalidate_cache) running in
the I/O coroutine.
The only side effect is that bdrv_co_invalidate_cache is not
recursive anymore, and so is every direct call to
bdrv_invalidate_cache().
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index 5e65f134f8..df353d55e8 100644
--- a/block.c
+++ b/block.c
@@ -6394,11 +6394,6 @@ void bdrv_init_with_whitelist(void)
}
int bdrv_activate(BlockDriverState *bs, Error **errp)
-{
- return bdrv_invalidate_cache(bs, errp);
-}
-
-int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
{
BdrvChild *child, *parent;
Error *local_err = NULL;
@@ -6410,7 +6405,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
}
QLIST_FOREACH(child, &bs->children, next) {
- bdrv_co_invalidate_cache(child->bs, &local_err);
+ bdrv_activate(child->bs, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return -EINVAL;
@@ -6423,7 +6418,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
* Note that the required permissions of inactive images are always a
* subset of the permissions required after activating the image. This
* allows us to just get the permissions upfront without restricting
- * drv->bdrv_invalidate_cache().
+ * bdrv_co_invalidate_cache().
*
* It also means that in error cases, we don't have to try and revert to
* the old permissions (which is an operation that could fail, too). We can
@@ -6438,13 +6433,10 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
return ret;
}
- if (bs->drv->bdrv_co_invalidate_cache) {
- bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
- if (local_err) {
- bs->open_flags |= BDRV_O_INACTIVE;
- error_propagate(errp, local_err);
- return -EINVAL;
- }
+ ret = bdrv_invalidate_cache(bs, errp);
+ if (ret < 0) {
+ bs->open_flags |= BDRV_O_INACTIVE;
+ return ret;
}
FOR_EACH_DIRTY_BITMAP(bs, bm) {
@@ -6473,6 +6465,23 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
return 0;
}
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
+{
+ Error *local_err = NULL;
+
+ assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
+ if (bs->drv->bdrv_co_invalidate_cache) {
+ bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
void bdrv_activate_all(Error **errp)
{
BlockDriverState *bs;
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/5] block layer: permission API refactoring in preparation
2022-02-09 10:54 [PATCH 0/5] block layer: permission API refactoring in preparation Emanuele Giuseppe Esposito
` (4 preceding siblings ...)
2022-02-09 10:54 ` [PATCH 5/5] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate Emanuele Giuseppe Esposito
@ 2022-03-01 10:34 ` Kevin Wolf
5 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2022-03-01 10:34 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: Fam Zheng, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, qemu-devel, Markus Armbruster,
Daniel Henrique Barboza, Philippe Mathieu-Daudé, Greg Kurz,
Hanna Reitz, qemu-ppc, Cédric Le Goater, Stefan Hajnoczi,
Paolo Bonzini, Denis V. Lunev, Dr. David Alan Gilbert,
David Gibson
Am 09.02.2022 um 11:54 hat Emanuele Giuseppe Esposito geschrieben:
> This serie aims to refactoring and fixing permission API related bugs that came
> up in the serie "block layer: split block APIs in global state and I/O".
> In that serie, we are splitting all block layer headers in
> Global State (GS) APIs, holding always the BQL and running in the
> main loop, and I/O running in iothreads.
>
> The patches in this serie are taken from v6 of the API split,
> to reduce its size and apply these fixes independently.
>
> Patches 1 and 2 take care of crypto and amend jobs, since they
> incorrectly use the permission API also in iothreads.
> Patches 3-4-5 take care of bdrv_invalidate_cache and callers,
> since this function checks too for permisisons while being
> called by an iothread.
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread