| Message ID | 20201127144522.29991-31-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: update graph permissions update | expand |
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
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 <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).
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
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
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(&bs_entry->state, bs_queue, errp)) { - goto cleanup; + ret = bdrv_reopen_prepare(&bs_entry->state, bs_queue, &tran, 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 = &bs_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, - &nperm, &nshared); - 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 = bdrv_topological_dfs(refresh_list, found, + state->old_backing_bs); } - bs_entry->perms_checked = true; + } + + ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp); + if (ret < 0) { + goto abort; } /* @@ -4235,51 +4231,29 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) bdrv_reopen_commit(&bs_entry->state); } - ret = 0; -cleanup_perm: - QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { - BDRVReopenState *state = &bs_entry->state; + tran_commit(tran); - if (!bs_entry->perms_checked) { - continue; - } - - if (ret == 0) { - uint64_t perm, shared; - - bdrv_get_cumulative_perm(state->bs, &perm, &shared); - assert(perm == state->perm); - assert(shared == state->shared_perm); + QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) { + BlockDriverState *bs = bs_entry->state.bs; - bdrv_set_perm(state->bs); - } else { - bdrv_abort_perm_update(state->bs); - if (state->replace_backing_bs && state->new_backing_bs) { - bdrv_abort_perm_update(state->new_backing_bs); - } + if (bs->drv->bdrv_reopen_commit_post) { + bs->drv->bdrv_reopen_commit_post(&bs_entry->state); } } + goto cleanup; - if (ret == 0) { - QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) { - BlockDriverState *bs = bs_entry->state.bs; - - if (bs->drv->bdrv_reopen_commit_post) - bs->drv->bdrv_reopen_commit_post(&bs_entry->state); +abort: + tran_abort(tran); + QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { + if (bs_entry->prepared) { + bdrv_reopen_abort(&bs_entry->state); } + qobject_unref(bs_entry->state.explicit_options); + qobject_unref(bs_entry->state.options); } + cleanup: QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { - if (ret) { - if (bs_entry->prepared) { - bdrv_reopen_abort(&bs_entry->state); - } - qobject_unref(bs_entry->state.explicit_options); - qobject_unref(bs_entry->state.options); - } - if (bs_entry->state.new_backing_bs) { - bdrv_unref(bs_entry->state.new_backing_bs); - } g_free(bs_entry); } g_free(bs_queue); @@ -4304,53 +4278,6 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, return ret; } -static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q, - BdrvChild *c) -{ - BlockReopenQueueEntry *entry; - - QTAILQ_FOREACH(entry, q, entry) { - BlockDriverState *bs = entry->state.bs; - BdrvChild *child; - - QLIST_FOREACH(child, &bs->children, next) { - if (child == c) { - return entry; - } - } - } - - return NULL; -} - -static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs, - uint64_t *perm, uint64_t *shared) -{ - BdrvChild *c; - BlockReopenQueueEntry *parent; - uint64_t cumulative_perms = 0; - uint64_t cumulative_shared_perms = BLK_PERM_ALL; - - QLIST_FOREACH(c, &bs->parents, next_parent) { - parent = find_parent_in_reopen_queue(q, c); - if (!parent) { - cumulative_perms |= c->perm; - cumulative_shared_perms &= c->shared_perm; - } else { - uint64_t nperm, nshared; - - bdrv_child_perm(parent->state.bs, bs, c, c->role, q, - parent->state.perm, parent->state.shared_perm, - &nperm, &nshared); - - cumulative_perms |= nperm; - cumulative_shared_perms &= nshared; - } - } - *perm = cumulative_perms; - *shared = cumulative_shared_perms; -} - static bool bdrv_reopen_can_attach(BlockDriverState *parent, BdrvChild *child, BlockDriverState *new_child, @@ -4392,6 +4319,7 @@ static bool bdrv_reopen_can_attach(BlockDriverState *parent, * Return 0 on success, otherwise return < 0 and set @errp. */ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, + GSList **set_backings_tran, Error **errp) { BlockDriverState *bs = reopen_state->bs; @@ -4468,6 +4396,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, /* If we want to replace the backing file we need some extra checks */ if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { + int ret; + /* Check for implicit nodes between bs and its backing file */ if (bs != overlay_bs) { error_setg(errp, "Cannot change backing link if '%s' has " @@ -4488,9 +4418,11 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, return -EPERM; } reopen_state->replace_backing_bs = true; - if (new_backing_bs) { - bdrv_ref(new_backing_bs); - reopen_state->new_backing_bs = new_backing_bs; + reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; + ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, + errp); + if (ret < 0) { + return ret; } } @@ -4515,7 +4447,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, * */ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, - BlockReopenQueue *queue, Error **errp) + BlockReopenQueue *queue, + GSList **set_backings_tran, Error **errp) { int ret = -1; int old_flags; @@ -4582,10 +4515,6 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, goto error; } - /* Calculate required permissions after reopening */ - bdrv_reopen_perm(queue, reopen_state->bs, - &reopen_state->perm, &reopen_state->shared_perm); - ret = bdrv_flush(reopen_state->bs); if (ret) { error_setg_errno(errp, -ret, "Error flushing drive"); @@ -4645,7 +4574,7 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, * either a reference to an existing node (using its node name) * or NULL to simply detach the current backing file. */ - ret = bdrv_reopen_parse_backing(reopen_state, errp); + ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp); if (ret < 0) { goto error; } @@ -4767,22 +4696,6 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state) qdict_del(bs->explicit_options, child->name); qdict_del(bs->options, child->name); } - - /* - * Change the backing file if a new one was specified. We do this - * after updating bs->options, so bdrv_refresh_filename() (called - * from bdrv_set_backing_hd()) has the new values. - */ - if (reopen_state->replace_backing_bs) { - BlockDriverState *old_backing_bs = child_bs(bs->backing); - assert(!old_backing_bs || !old_backing_bs->implicit); - /* Abort the permission update on the backing bs we're detaching */ - if (old_backing_bs) { - bdrv_abort_perm_update(old_backing_bs); - } - bdrv_set_backing_hd(bs, reopen_state->new_backing_bs, &error_abort); - } - bdrv_refresh_limits(bs, NULL); } diff --git a/block/file-posix.c b/block/file-posix.c index 37d9266f6a..42c60c8a02 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -175,7 +175,6 @@ typedef struct BDRVRawState { } BDRVRawState; typedef struct BDRVRawReopenState { - int fd; int open_flags; bool drop_cache; bool check_cache_dropped; @@ -1062,7 +1061,6 @@ static int raw_reopen_prepare(BDRVReopenState *state, BDRVRawReopenState *rs; QemuOpts *opts; int ret; - Error *local_err = NULL; assert(state != NULL); assert(state->bs != NULL); @@ -1088,32 +1086,9 @@ static int raw_reopen_prepare(BDRVReopenState *state, * bdrv_reopen_prepare() will detect changes and complain. */ qemu_opts_to_qdict(opts, state->options); - rs->fd = raw_reconfigure_getfd(state->bs, state->flags, &rs->open_flags, - state->perm, true, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -1; - goto out; - } - - /* Fail already reopen_prepare() if we can't get a working O_DIRECT - * alignment with the new fd. */ - if (rs->fd != -1) { - raw_probe_alignment(state->bs, rs->fd, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto out_fd; - } - } - s->reopen_state = state; ret = 0; -out_fd: - if (ret < 0) { - qemu_close(rs->fd); - rs->fd = -1; - } + out: qemu_opts_del(opts); return ret; @@ -1127,10 +1102,6 @@ static void raw_reopen_commit(BDRVReopenState *state) s->drop_cache = rs->drop_cache; s->check_cache_dropped = rs->check_cache_dropped; s->open_flags = rs->open_flags; - - qemu_close(s->fd); - s->fd = rs->fd; - g_free(state->opaque); state->opaque = NULL; @@ -1149,10 +1120,6 @@ static void raw_reopen_abort(BDRVReopenState *state) return; } - if (rs->fd >= 0) { - qemu_close(rs->fd); - rs->fd = -1; - } g_free(state->opaque); state->opaque = NULL; @@ -3060,39 +3027,30 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, Error **errp) { BDRVRawState *s = bs->opaque; - BDRVRawReopenState *rs = NULL; + int input_flags = s->reopen_state ? s->reopen_state->flags : bs->open_flags; int open_flags; int ret; - if (s->perm_change_fd) { + /* We may need a new fd if auto-read-only switches the mode */ + ret = raw_reconfigure_getfd(bs, input_flags, &open_flags, perm, + false, errp); + if (ret < 0) { + return ret; + } else if (ret != s->fd) { + Error *local_err = NULL; + /* - * In the context of reopen, this function may be called several times - * (directly and recursively while change permissions of the parent). - * This is even true for children that don't inherit from the original - * reopen node, so s->reopen_state is not set. - * - * Ignore all but the first call. + * Fail already check_perm() if we can't get a working O_DIRECT + * alignment with the new fd. */ - return 0; - } - - if (s->reopen_state) { - /* We already have a new file descriptor to set permissions for */ - assert(s->reopen_state->perm == perm); - assert(s->reopen_state->shared_perm == shared); - rs = s->reopen_state->opaque; - s->perm_change_fd = rs->fd; - s->perm_change_flags = rs->open_flags; - } else { - /* We may need a new fd if auto-read-only switches the mode */ - ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags, perm, - false, errp); - if (ret < 0) { - return ret; - } else if (ret != s->fd) { - s->perm_change_fd = ret; - s->perm_change_flags = open_flags; + raw_probe_alignment(bs, ret, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; } + + s->perm_change_fd = ret; + s->perm_change_flags = open_flags; } /* Prepare permissions on old fd to avoid conflicts between old and new, @@ -3114,7 +3072,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared, return 0; fail: - if (s->perm_change_fd && !s->reopen_state) { + if (s->perm_change_fd) { qemu_close(s->perm_change_fd); } s->perm_change_fd = 0; @@ -3145,7 +3103,7 @@ static void raw_abort_perm_update(BlockDriverState *bs) /* For reopen, .bdrv_reopen_abort is called afterwards and will close * the file descriptor. */ - if (s->perm_change_fd && !s->reopen_state) { + if (s->perm_change_fd) { qemu_close(s->perm_change_fd); } s->perm_change_fd = 0;
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 <vsementsov@virtuozzo.com> --- include/block/block.h | 2 +- block.c | 183 +++++++++++------------------------------- block/file-posix.c | 84 +++++-------------- 3 files changed, 70 insertions(+), 199 deletions(-)