Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
Date: Fri, 5 Feb 2021 18:57:06 +0100	[thread overview]
Message-ID: <20210205175706.GI7072@merkur.fritz.box> (raw)
In-Reply-To: <20201127144522.29991-31-vsementsov@virtuozzo.com>

Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Move bdrv_reopen_multiple to new paradigm of permission update:
> first update graph relations, then do refresh the permissions.
> 
> We have to modify reopen process in file-posix driver: with new scheme
> we don't have prepared permissions in raw_reopen_prepare(), so we
> should reconfigure fd in raw_check_perm(). Still this seems more native
> and simple anyway.

Hm... The diffstat shows that it is simpler because it needs less code.

But relying on the permission change callbacks for getting a new file
descriptor that changes more than just permissions doesn't feel
completely right either. Can we even expect the permission callbacks to
be called when the permissions aren't changed?

But then, reopen and permission updates were already a bit entangled
before. If we can guarantee that the permission functions will always be
called, even if the permissions don't change, I guess it's okay.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h |   2 +-
>  block.c               | 183 +++++++++++-------------------------------
>  block/file-posix.c    |  84 +++++--------------
>  3 files changed, 70 insertions(+), 199 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 0f21ef313f..82271d9ccd 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -195,7 +195,7 @@ typedef struct BDRVReopenState {
>      BlockdevDetectZeroesOptions detect_zeroes;
>      bool backing_missing;
>      bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
> -    BlockDriverState *new_backing_bs; /* If NULL then detach the current bs */
> +    BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
>      uint64_t perm, shared_perm;

perm and shared_perm are unused now and can be removed.

>      QDict *options;
>      QDict *explicit_options;
> diff --git a/block.c b/block.c
> index 617cba9547..474e624152 100644
> --- a/block.c
> +++ b/block.c
> @@ -103,8 +103,9 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
>                                      GSList **tran, Error **errp);
>  static void bdrv_remove_backing(BlockDriverState *bs, GSList **tran);
>  
> -static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue
> -                               *queue, Error **errp);
> +static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
> +                               BlockReopenQueue *queue,
> +                               GSList **set_backings_tran, Error **errp);
>  static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
>  static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>  
> @@ -2403,6 +2404,7 @@ static void bdrv_list_abort_perm_update(GSList *list)
>      }
>  }
>  
> +__attribute__((unused))
>  static void bdrv_abort_perm_update(BlockDriverState *bs)
>  {
>      g_autoptr(GSList) list = bdrv_topological_dfs(NULL, NULL, bs);
> @@ -2498,6 +2500,7 @@ char *bdrv_perm_names(uint64_t perm)
>   *
>   * Needs to be followed by a call to either bdrv_set_perm() or
>   * bdrv_abort_perm_update(). */
> +__attribute__((unused))
>  static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
>                                    uint64_t new_used_perm,
>                                    uint64_t new_shared_perm,
> @@ -4100,10 +4103,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      bs_entry->state.explicit_options = explicit_options;
>      bs_entry->state.flags = flags;
>  
> -    /* This needs to be overwritten in bdrv_reopen_prepare() */
> -    bs_entry->state.perm = UINT64_MAX;
> -    bs_entry->state.shared_perm = 0;
> -
>      /*
>       * If keep_old_opts is false then it means that unspecified
>       * options must be reset to their original value. We don't allow
> @@ -4186,40 +4185,37 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>   */
>  int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>  {
> -    int ret = -1;
> +    int ret = 0;

I would prefer to leave this right before the 'goto cleanup'.

Not sure if I fully understand all consequences yet, but overall, apart
from my concerns about file-posix and the potential AioContext locking
problems, this looks like a nice simplification of the process.

Come to think of it, the AioContext handling is probably wrong already
before your series. reopen_commit for one node could move the whole tree
to a different context and then the later nodes would all be processed
while holding the wrong lock.

Kevin