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

[Kevin Wolf]

Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben:
> 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
> @@ -6550,23 +6550,20 @@ 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;
>      int ret;
>      BdrvDirtyBitmap *bm;
>  
> +    assert(qemu_in_main_thread());
> +
>      if (!bs->drv)  {
>          return -ENOMEDIUM;
>      }
>  
>      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;
> @@ -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().
>       *
>       * 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);
>  
>          FOR_EACH_DIRTY_BITMAP(bs, bm) {
>              bdrv_dirty_bitmap_skip_store(bm, false);
> @@ -6629,6 +6619,22 @@ 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;
> +
> +    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;

This doesn't feel like the right place. The flag is cleared by the
caller, so it should also be set again on failure by the caller and not
by this function.

What bdrv_co_invalidate_cache() could do is assert that BDRV_O_INACTIVE
is cleared when it's called.

> +            error_propagate(errp, local_err);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}

Kevin