Re: [PATCH v5 28/31] block.c: assert BQL lock held in bdrv_co_invalidate_cache

[Hanna Reitz]

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:
> bdrv_co_invalidate_cache is special: it is an I/O function,

I still don’t believe it is, but well.

(Yes, it is called by a test in an iothread, but I believe we’ve seen 
that the tests simply sometimes test things that shouldn’t be allowed.)

> but uses the block layer permission API, which is GS.
>
> Because of this, we can assert that either the function is
> being called with BQL held, and thus can use the permission API,
> or make sure that the permission API is not used, by ensuring that
> bs (and parents) .open_flags does not contain BDRV_O_INACTIVE.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/block.c b/block.c
> index a0309f827d..805974676b 100644
> --- a/block.c
> +++ b/block.c
> @@ -6574,6 +6574,26 @@ void bdrv_init_with_whitelist(void)
>       bdrv_init();
>   }
>   
> +static bool bdrv_is_active(BlockDriverState *bs)
> +{
> +    BdrvChild *parent;
> +
> +    if (bs->open_flags & BDRV_O_INACTIVE) {
> +        return false;
> +    }
> +
> +    QLIST_FOREACH(parent, &bs->parents, next_parent) {
> +        if (parent->klass->parent_is_bds) {
> +            BlockDriverState *parent_bs = parent->opaque;

This looks like a really bad hack to me.  We purposefully have made the 
parent link opaque so that a BDS cannot easily reach its parents.  All 
accesses should go through BdrvChildClass methods.

I also don’t understand why we need to query parents at all.  The only 
fact that determines whether the current BDS will have its permissions 
changed is whether the BDS itself is active or inactive.  Sure, we’ll 
invoke bdrv_co_invalidate_cache() on the parents, too, but then we could 
simply let the assertion fail there.

> +            if (!bdrv_is_active(parent_bs)) {
> +                return false;
> +            }
> +        }
> +    }
> +
> +   return true;
> +}
> +
>   int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>   {
>       BdrvChild *child, *parent;
> @@ -6585,6 +6605,12 @@ int coroutine_fn 
> bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
>           return -ENOMEDIUM;
>       }
>   
> +    /*
> +     * No need to muck with permissions if bs is active.
> +     * TODO: should activation be a separate function?
> +     */
> +    assert(qemu_in_main_thread() || bdrv_is_active(bs));
> +

I don’t understand this, really.  It looks to me like “if you don’t call 
this in the main thread, this better be a no-op”, i.e., you must never 
call this function in an I/O thread if you really want to use it.  I.e. 
what I’d classify as a GS function.

It sounds like this is just a special case for said test, and 
special-casing code for tests sounds like a bad idea.

Hanna

>       QLIST_FOREACH(child, &bs->children, next) {
>           bdrv_co_invalidate_cache(child->bs, &local_err);
>           if (local_err) {