Re: [PATCH v6 21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate

[Hanna Reitz]

On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
> Split bdrv_co_invalidate cache in two: the GS 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 | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7ab5031027..bad834c86e 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -6579,7 +6576,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().
> +     * drv->bdrv_co_invalidate_cache().

Perhaps just `bdrv_invalidate_cache()`, without the `drv->`?

>        *
>        * 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
> @@ -6594,14 +6591,7 @@ 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;
> -            }
> -        }
> +        bdrv_invalidate_cache(bs, errp);

We should check the returned value here, and return on error.

Other than that, looks good and makes sense.

Hanna

>           FOR_EACH_DIRTY_BITMAP(bs, bm) {
>               bdrv_dirty_bitmap_skip_store(bm, false);