Re: [PATCH v5 05/31] block-backend: special comments for blk_set/get_perm due to fuse

[Hanna Reitz]

On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> Fuse logic can be classified as I/O, so there is no BQL held
> during its execution. And yet, it uses blk_{get/set}_perm
> functions, that are classified as BQL and clearly require
> the BQL lock. Since there is no easy solution for this,
> add a couple of TODOs and FIXME in the relevant sections of the
> code.

Well, the problem goes deeper, because we still consider them GS 
functions.  So it’s fine for the permission function 
raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS function, 
and then you still get:

qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion 
`qemu_in_main_thread()' failed.

It looks like in this case making bdrv_get_flags() not a GS function 
would be feasible and might solve the problem, but I guess we should 
instead think about adding something like

if (!exp->growable && !qemu_in_main_thread()) {
    /* Changing permissions like below only works in the main thread */
    return -EPERM;
}

to fuse_do_truncate().

Ideally, to make up for this, we should probably take the RESIZE 
permission in fuse_export_create() for writable exports in iothreads.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-backend.c | 10 ++++++++++
>   block/export/fuse.c   | 16 ++++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 1f0bda578e..7a4b50a8f0 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, 
> uint64_t shared_perm,
>                    Error **errp)
>   {
>       int ret;
> +    /*
> +     * FIXME: blk_{get/set}_perm should be always called under
> +     * BQL, but it is not the case right now (see block/export/fuse.c)
> +     */
> +    /* assert(qemu_in_main_thread()); */
>   
>       if (blk->root && !blk->disable_perm) {
>           ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp);
> @@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, 
> uint64_t shared_perm,
>   
>   void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
>   {
> +    /*
> +     * FIXME: blk_{get/set}_perm should be always called under
> +     * BQL, but it is not the case right now (see block/export/fuse.c)
> +     */
> +    /* assert(qemu_in_main_thread()); */
>       *perm = blk->perm;
>       *shared_perm = blk->shared_perm;
>   }
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index 823c126d23..7ceb8d783b 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp,
>       /* For growable exports, take the RESIZE permission */
>       if (args->growable) {
>           uint64_t blk_perm, blk_shared_perm;
> -
> +        /*
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */

I believe we are, BlockExportDriver.create() is called by blk_exp_add(), 
which looks very much like it runs in the main thread (calling 
bdrv_lookup_bs(), bdrv_try_set_aio_context(), blk_new(), and whatnot).

Hanna

>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
>   
>           ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
> @@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t 
> size,
>   
>       /* Growable exports have a permanent RESIZE permission */
>       if (!exp->growable) {
> +
> +        /*
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */
>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
>   
>           ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
> @@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t 
> size,
>                          truncate_flags, NULL);
>   
>       if (!exp->growable) {
> -        /* Must succeed, because we are only giving up the RESIZE permission */
> +        /*
> +         * Must succeed, because we are only giving up the RESIZE permission.
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */
>           blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, 
> &error_abort);
>       }
>