| Message ID | 20220121170544.2049944-21-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
Emanuele Giuseppe Esposito <eesposit@redhat.com> wrote: > 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. Reviewed-by: Juan Quintela <quintela@redhat.com> In the sense that makes sense for this series. But if you have to respin. > diff --git a/migration/migration.c b/migration/migration.c > index 0652165610..1f06fd2d18 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -499,7 +499,7 @@ static void process_incoming_migration_bh(void *opaque) > global_state_get_runstate() == RUN_STATE_RUNNING))) { > /* Make sure all file formats flush 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); I guess that we can change the comment here, it just looks weird the comment saying flush() and the function nawed _activate() > @@ -586,7 +586,7 @@ 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); > + bdrv_activate_all(&local_err); > if (local_err) { > error_report_err(local_err); > goto fail; same here. > diff --git a/migration/savevm.c b/migration/savevm.c > index adb5fae9f1..3f4f924093 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1437,7 +1437,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)", Here the comment makes sense. > @@ -2007,7 +2007,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) > > /* Make sure all file formats flush 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; Comment here is bad. > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 14e3beeaaf..f97bf7ca77 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; I don't understand the comment here. And yes, I know, this was here before your change. Thanks, Juan.
On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: > 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> > --- > block.c | 2 +- > block/block-backend.c | 2 +- > hw/block/pflash_cfi01.c | 2 +- > hw/nvram/spapr_nvram.c | 2 +- > include/block/block-global-state.h | 2 +- > include/sysemu/block-backend-io.h | 2 +- > migration/block.c | 2 +- > migration/migration.c | 10 +++++----- > migration/savevm.c | 4 ++-- > monitor/qmp-cmds.c | 2 +- > tests/unit/test-block-iothread.c | 6 +++--- > 11 files changed, 18 insertions(+), 18 deletions(-) Reviewed-by: Hanna Reitz <hreitz@redhat.com>
On 24/01/2022 11:44, Juan Quintela wrote: >> diff --git a/migration/migration.c b/migration/migration.c >> index 0652165610..1f06fd2d18 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -499,7 +499,7 @@ static void process_incoming_migration_bh(void *opaque) >> global_state_get_runstate() == RUN_STATE_RUNNING))) { >> /* Make sure all file formats flush 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); > I guess that we can change the comment here, it just looks weird the > comment saying flush() and the function nawed _activate() > Do you think it's enough to replace "flush" with "activate"? I am not sure whether "activate their mutable metadata" is meaningful. Thank you, Emanuele
On 1/27/22 10:18, Emanuele Giuseppe Esposito wrote: >>> /* Make sure all file formats flush 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); >> I guess that we can change the comment here, it just looks weird the >> comment saying flush() and the function nawed _activate() >> > Do you think it's enough to replace "flush" with "activate"? I am not > sure whether "activate their mutable metadata" is meaningful. "Activation" consists of throwing away mutable metadata. That's what "flush" means in this case, but it is often interpreted as "persisting" the metadata instead. So perhaps s/flush/throw away/. Paolo
diff --git a/block.c b/block.c index 73c4e0a5a5..7ab5031027 100644 --- a/block.c +++ b/block.c @@ -6629,7 +6629,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 a962fc407a..eb2433d9cb 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1931,7 +1931,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-global-state.h b/include/block/block-global-state.h index a98113189e..dcf0ebcade 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -144,7 +144,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, /* async block I/O */ void bdrv_aio_cancel(BlockAIOCB *acb); int bdrv_activate(BlockDriverState *bs, Error **errp); -void bdrv_invalidate_cache_all(Error **errp); +void bdrv_activate_all(Error **errp); int bdrv_inactivate_all(void); int bdrv_flush_all(void); diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h index 178cf197fb..83f58f66dd 100644 --- a/include/sysemu/block-backend-io.h +++ b/include/sysemu/block-backend-io.h @@ -112,7 +112,7 @@ int blk_get_max_iov(BlockBackend *blk); int blk_get_max_hw_iov(BlockBackend *blk); void blk_set_guest_block_size(BlockBackend *blk, int align); -void blk_invalidate_cache(BlockBackend *blk, Error **errp); +void blk_activate(BlockBackend *blk, Error **errp); void blk_io_plug(BlockBackend *blk); void blk_io_unplug(BlockBackend *blk); 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 0652165610..1f06fd2d18 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -499,7 +499,7 @@ static void process_incoming_migration_bh(void *opaque) global_state_get_runstate() == RUN_STATE_RUNNING))) { /* Make sure all file formats flush 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; @@ -586,7 +586,7 @@ 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); + bdrv_activate_all(&local_err); if (local_err) { error_report_err(local_err); goto fail; @@ -1923,7 +1923,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); } @@ -3246,7 +3246,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 adb5fae9f1..3f4f924093 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1437,7 +1437,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)", @@ -2007,7 +2007,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) /* Make sure all file formats flush 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 14e3beeaaf..f97bf7ca77 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, }, };
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> --- block.c | 2 +- block/block-backend.c | 2 +- hw/block/pflash_cfi01.c | 2 +- hw/nvram/spapr_nvram.c | 2 +- include/block/block-global-state.h | 2 +- include/sysemu/block-backend-io.h | 2 +- migration/block.c | 2 +- migration/migration.c | 10 +++++----- migration/savevm.c | 4 ++-- monitor/qmp-cmds.c | 2 +- tests/unit/test-block-iothread.c | 6 +++--- 11 files changed, 18 insertions(+), 18 deletions(-)