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 <vsement...@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
