From d0ac038025bb6e7886c2324258d24e71215b5d35 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 1 Mar 2017 17:30:41 +0100 Subject: block: Factor out should_update_child() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé --- block.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index f293ccb5af..6dc02b85aa 100644 --- a/block.c +++ b/block.c @@ -2886,28 +2886,40 @@ void bdrv_close_all(void) assert(QTAILQ_EMPTY(&all_bdrv_states)); } +static bool should_update_child(BdrvChild *c, BlockDriverState *to) +{ + BdrvChild *to_c; + + if (c->role->stay_at_node) { + return false; + } + + if (c->role == &child_backing) { + /* If @from is a backing file of @to, ignore the child to avoid + * creating a loop. We only want to change the pointer of other + * parents. */ + QLIST_FOREACH(to_c, &to->children, next) { + if (to_c == c) { + break; + } + } + if (to_c) { + return false; + } + } + + return true; +} + static void change_parent_backing_link(BlockDriverState *from, BlockDriverState *to) { - BdrvChild *c, *next, *to_c; + BdrvChild *c, *next; QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { - if (c->role->stay_at_node) { + if (!should_update_child(c, to)) { continue; } - if (c->role == &child_backing) { - /* If @from is a backing file of @to, ignore the child to avoid - * creating a loop. We only want to change the pointer of other - * parents. */ - QLIST_FOREACH(to_c, &to->children, next) { - if (to_c == c) { - break; - } - } - if (to_c) { - continue; - } - } bdrv_ref(to); /* FIXME Are we sure that bdrv_replace_child() can't run into -- cgit v1.2.3 From 8ee039951dea9a809e4745c42aebb4a7cec4bbbb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 13:45:28 +0100 Subject: block: Factor out bdrv_replace_child_noperm() Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index 6dc02b85aa..d4570c85b3 100644 --- a/block.c +++ b/block.c @@ -1713,11 +1713,10 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, *nshared = shared; } -static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, - bool check_new_perm) +static void bdrv_replace_child_noperm(BdrvChild *child, + BlockDriverState *new_bs) { BlockDriverState *old_bs = child->bs; - uint64_t perm, shared_perm; if (old_bs) { if (old_bs->quiesce_counter && child->role->drained_end) { @@ -1727,7 +1726,29 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, child->role->detach(child); } QLIST_REMOVE(child, next_parent); + } + + child->bs = new_bs; + + if (new_bs) { + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); + if (new_bs->quiesce_counter && child->role->drained_begin) { + child->role->drained_begin(child); + } + + if (child->role->attach) { + child->role->attach(child); + } + } +} +static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, + bool check_new_perm) +{ + BlockDriverState *old_bs = child->bs; + uint64_t perm, shared_perm; + + if (old_bs) { /* Update permissions for old node. This is guaranteed to succeed * because we're just taking a parent away, so we're loosening * restrictions. */ @@ -1736,23 +1757,14 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, bdrv_set_perm(old_bs, perm, shared_perm); } - child->bs = new_bs; + bdrv_replace_child_noperm(child, new_bs); if (new_bs) { - QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); - if (new_bs->quiesce_counter && child->role->drained_begin) { - child->role->drained_begin(child); - } - bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm); if (check_new_perm) { bdrv_check_perm(new_bs, perm, shared_perm, &error_abort); } bdrv_set_perm(new_bs, perm, shared_perm); - - if (child->role->attach) { - child->role->attach(child); - } } } -- cgit v1.2.3 From 46181129eac9a56d9a948667282dd03d5015f096 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 15:00:13 +0100 Subject: block: Ignore multiple children in bdrv_check_update_perm() change_parent_backing_link() will need to update multiple BdrvChild objects at once. Checking permissions reference by reference doesn't work because permissions need to be consistent only with all parents moved to the new child. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index d4570c85b3..a7b09d32c3 100644 --- a/block.c +++ b/block.c @@ -1398,7 +1398,8 @@ static int bdrv_fill_options(QDict **options, const char *filename, * or bdrv_abort_perm_update(). */ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms, - uint64_t cumulative_shared_perms, Error **errp) + uint64_t cumulative_shared_perms, + GSList *ignore_children, Error **errp) { BlockDriver *drv = bs->drv; BdrvChild *c; @@ -1434,7 +1435,8 @@ static int bdrv_check_perm(BlockDriverState *bs, uint64_t cumulative_perms, drv->bdrv_child_perm(bs, c, c->role, cumulative_perms, cumulative_shared_perms, &cur_perm, &cur_shared); - ret = bdrv_child_check_perm(c, cur_perm, cur_shared, errp); + ret = bdrv_child_check_perm(c, cur_perm, cur_shared, ignore_children, + errp); if (ret < 0) { return ret; } @@ -1554,15 +1556,15 @@ static char *bdrv_perm_names(uint64_t perm) /* * Checks whether a new reference to @bs can be added if the new user requires - * @new_used_perm/@new_shared_perm as its permissions. If @ignore_child is set, - * this old reference is ignored in the calculations; this allows checking - * permission updates for an existing reference. + * @new_used_perm/@new_shared_perm as its permissions. If @ignore_children is + * set, the BdrvChild objects in this list are ignored in the calculations; + * this allows checking permission updates for an existing reference. * * Needs to be followed by a call to either bdrv_set_perm() or * bdrv_abort_perm_update(). */ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm, uint64_t new_shared_perm, - BdrvChild *ignore_child, Error **errp) + GSList *ignore_children, Error **errp) { BdrvChild *c; uint64_t cumulative_perms = new_used_perm; @@ -1572,7 +1574,7 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm, assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED); QLIST_FOREACH(c, &bs->parents, next_parent) { - if (c == ignore_child) { + if (g_slist_find(ignore_children, c)) { continue; } @@ -1602,15 +1604,22 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm, cumulative_shared_perms &= c->shared_perm; } - return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms, errp); + return bdrv_check_perm(bs, cumulative_perms, cumulative_shared_perms, + ignore_children, errp); } /* Needs to be followed by a call to either bdrv_child_set_perm() or * bdrv_child_abort_perm_update(). */ int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared, - Error **errp) + GSList *ignore_children, Error **errp) { - return bdrv_check_update_perm(c->bs, perm, shared, c, errp); + int ret; + + ignore_children = g_slist_prepend(g_slist_copy(ignore_children), c); + ret = bdrv_check_update_perm(c->bs, perm, shared, ignore_children, errp); + g_slist_free(ignore_children); + + return ret; } void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared) @@ -1635,7 +1644,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared, { int ret; - ret = bdrv_child_check_perm(c, perm, shared, errp); + ret = bdrv_child_check_perm(c, perm, shared, NULL, errp); if (ret < 0) { bdrv_child_abort_perm_update(c); return ret; @@ -1753,7 +1762,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, * because we're just taking a parent away, so we're loosening * restrictions. */ bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm); - bdrv_check_perm(old_bs, perm, shared_perm, &error_abort); + bdrv_check_perm(old_bs, perm, shared_perm, NULL, &error_abort); bdrv_set_perm(old_bs, perm, shared_perm); } @@ -1762,7 +1771,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, if (new_bs) { bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm); if (check_new_perm) { - bdrv_check_perm(new_bs, perm, shared_perm, &error_abort); + bdrv_check_perm(new_bs, perm, shared_perm, NULL, &error_abort); } bdrv_set_perm(new_bs, perm, shared_perm); } -- cgit v1.2.3 From 234ac1a9025bcfcc532449f72a97b3d4754d466c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 2 Mar 2017 18:43:00 +0100 Subject: block: Handle permission errors in change_parent_backing_link() Instead of just trying to change parents by parent over to reference @to instead of @from, and abort()ing whenever the permissions don't allow this, do proper permission checking beforehand and pass any error to the callers. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 6 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index a7b09d32c3..a3101329c0 100644 --- a/block.c +++ b/block.c @@ -2933,21 +2933,53 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) } static void change_parent_backing_link(BlockDriverState *from, - BlockDriverState *to) + BlockDriverState *to, Error **errp) { BdrvChild *c, *next; + GSList *list = NULL, *p; + uint64_t old_perm, old_shared; + uint64_t perm = 0, shared = BLK_PERM_ALL; + int ret; + + /* Make sure that @from doesn't go away until we have successfully attached + * all of its parents to @to. */ + bdrv_ref(from); + /* Put all parents into @list and calculate their cumulative permissions */ QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { if (!should_update_child(c, to)) { continue; } + list = g_slist_prepend(list, c); + perm |= c->perm; + shared &= c->shared_perm; + } + + /* Check whether the required permissions can be granted on @to, ignoring + * all BdrvChild in @list so that they can't block themselves. */ + ret = bdrv_check_update_perm(to, perm, shared, list, errp); + if (ret < 0) { + bdrv_abort_perm_update(to); + goto out; + } + + /* Now actually perform the change. We performed the permission check for + * all elements of @list at once, so set the permissions all at once at the + * very end. */ + for (p = list; p != NULL; p = p->next) { + c = p->data; bdrv_ref(to); - /* FIXME Are we sure that bdrv_replace_child() can't run into - * &error_abort because of permissions? */ - bdrv_replace_child(c, to, true); + bdrv_replace_child_noperm(c, to); bdrv_unref(from); } + + bdrv_get_cumulative_perm(to, &old_perm, &old_shared); + bdrv_set_perm(to, old_perm | perm, old_shared | shared); + +out: + g_slist_free(list); + bdrv_unref(from); } /* @@ -2980,7 +3012,12 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, goto out; } - change_parent_backing_link(bs_top, bs_new); + change_parent_backing_link(bs_top, bs_new, &local_err); + if (local_err) { + error_propagate(errp, local_err); + bdrv_set_backing_hd(bs_new, NULL, &error_abort); + goto out; + } /* bs_new is now referenced by its new parents, we don't need the * additional reference any more. */ @@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new) bdrv_ref(old); - change_parent_backing_link(old, new); + /* FIXME Proper error handling */ + change_parent_backing_link(old, new, &error_abort); bdrv_unref(old); } -- cgit v1.2.3 From 5fe31c25cce3b09e989e40439667cd4961aba969 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 6 Mar 2017 16:20:51 +0100 Subject: block: Fix error handling in bdrv_replace_in_backing_chain() When adding an Error parameter, bdrv_replace_in_backing_chain() would become nothing more than a wrapper around change_parent_backing_link(). So make the latter public, renamed as bdrv_replace_node(), and remove bdrv_replace_in_backing_chain(). Most of the callers just remove a node from the graph that they just inserted, so they can use &error_abort, but completion of a mirror job with 'replaces' set can actually fail. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng Reviewed-by: Eric Blake --- block.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index a3101329c0..dd9ded81b9 100644 --- a/block.c +++ b/block.c @@ -2932,8 +2932,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) return true; } -static void change_parent_backing_link(BlockDriverState *from, - BlockDriverState *to, Error **errp) +void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + Error **errp) { BdrvChild *c, *next; GSList *list = NULL, *p; @@ -2941,6 +2941,9 @@ static void change_parent_backing_link(BlockDriverState *from, uint64_t perm = 0, shared = BLK_PERM_ALL; int ret; + assert(!atomic_read(&from->in_flight)); + assert(!atomic_read(&to->in_flight)); + /* Make sure that @from doesn't go away until we have successfully attached * all of its parents to @to. */ bdrv_ref(from); @@ -3003,16 +3006,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, { Error *local_err = NULL; - assert(!atomic_read(&bs_top->in_flight)); - assert(!atomic_read(&bs_new->in_flight)); - bdrv_set_backing_hd(bs_new, bs_top, &local_err); if (local_err) { error_propagate(errp, local_err); goto out; } - change_parent_backing_link(bs_top, bs_new, &local_err); + bdrv_replace_node(bs_top, bs_new, &local_err); if (local_err) { error_propagate(errp, local_err); bdrv_set_backing_hd(bs_new, NULL, &error_abort); @@ -3025,19 +3025,6 @@ out: bdrv_unref(bs_new); } -void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new) -{ - assert(!bdrv_requests_pending(old)); - assert(!bdrv_requests_pending(new)); - - bdrv_ref(old); - - /* FIXME Proper error handling */ - change_parent_backing_link(old, new, &error_abort); - - bdrv_unref(old); -} - static void bdrv_delete(BlockDriverState *bs) { assert(!bs->job); -- cgit v1.2.3