Re: [PATCH v6 20/33] block: rename bdrv_invalidate_cache_all, blk_invalidate_cache and test_sync_op_invalidate_cache

[Juan Quintela]

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.