Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
Am 08.02.2021 um 12:21 hat Vladimir Sementsov-Ogievskiy geschrieben: > > 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). Whoops, what I gave was kind of a non-answer... So essentially the reason for the locking rules of changing the AioContext is that they drain the node first and drain imposes the locking rule that the AioContext for the node to be drained must be locked, and all other AioContexts must be unlocked. The reason why drain imposes the rule is that we run AIO_WAIT_WHILE() in one thread and we may need the event loops in other threads to make progress until the while condition can eventually become false. If other threads can't make progress because their lock is taken, we'll see deadlocks sooner or later. Kevin
Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
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
> > > ---
> > > 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
Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
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.
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
---
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
Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
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
> ---
> 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
[PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph
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.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
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;
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;
BlockReopenQueueEntry *bs_entry, *next;
+GSList *tran = NULL;
+g_autoptr(GHashTable) found = NULL;
+g_autoptr(GSList) refresh_list = NULL;
assert(bs_queue != NULL);
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
assert(bs_entry->state.bs->quiesce_counter > 0);
-if (bdrv_reopen_prepare(_entry->state, bs_queue, errp)) {
-goto cleanup;
+ret = bdrv_reopen_prepare(_entry->state, bs_queue, , errp);
+if (ret < 0) {
+goto abort;
}
bs_entry->prepared = true;
}
+found = g_hash_table_new(NULL, NULL);
QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
BDRVReopenState *state = _entry->state;
-ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
- state->shared_perm, errp);
-if (ret < 0) {
-goto cleanup_perm;
-}
-/* Check if new_backing_bs would accept the new permissions */
-if (state->replace_backing_bs && state->new_backing_bs) {
-uint64_t nperm, nshared;
-bdrv_child_perm(state->bs, state->new_backing_bs,
-NULL, bdrv_backing_role(state->bs),
-bs_queue, state->perm, state->shared_perm,
-, );
-ret = bdrv_check_update_perm(state->new_backing_bs, NULL,
- nperm, nshared, errp);
-if (ret < 0) {
-goto cleanup_perm;
-}
+
+refresh_list = bdrv_topological_dfs(refresh_list, found, state->bs);
+if (state->old_backing_bs) {
+refresh_list =
