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

[Kevin Wolf <kwolf@redhat.com>]

Am 08.02.2021 um 12:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 05.02.2021 20:57, Kevin Wolf wrote:
> > 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?
> 
> With new scheme permission update becomes an obvious step of
> bdrv_reopen_multiple(): we do call bdrv_list_refresh_perms(), for the
> list of all touched nodes and all their subtrees. And callbacks are
> called unconditionally bdrv_node_refresh_perm()->bdrv_drv_set_perm().
> So, I think, we can rely on it. Probably worth one-two comments.

Yes, some comments in the right places that we must call the driver
callbacks even if the permissions are the same as before wouldn't hurt.

> > 
> > 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.
> > 
> 
> Probably proper way is to acquire all involved aio contexts as I do in
> 29 and update aio-context updating functions to work in such
> conditions(all aio contexts are already acquired by caller).

Well, as we already discussed, patch 29 is probably wrong in its current
form. But you seemed to have a solution in mind, which will hopefully
work here, too.

Kevin