| Message ID | 20191111160216.197086-12-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix check_to_replace_node() | expand |
11.11.2019 19:02, Max Reitz wrote: > Let check_to_replace_node() use the more specialized > bdrv_recurse_can_replace() instead of > bdrv_recurse_is_first_non_filter(), which is too restrictive. or not enough restrictive in case of quorum > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index de53addeb0..7608f21570 100644 > --- a/block.c > +++ b/block.c > @@ -6243,6 +6243,17 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs, > return false; > } > > +/* > + * Check whether the given @node_name can be replaced by a node that > + * has the same data as @parent_bs. If so, return @node_name's BDS; > + * NULL otherwise. > + * > + * @node_name must be a (recursive) *child of @parent_bs (or this > + * function will return NULL). > + * > + * The result (whether the node can be replaced or not) is only valid > + * for as long as no graph changes occur. actually, no graph changes neither any permission changes or updates. > + */ > BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > const char *node_name, Error **errp) > { > @@ -6267,8 +6278,11 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > * Another benefit is that this tests exclude backing files which are > * blocked by the backing blockers. > */ > - if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { > - error_setg(errp, "Only top most non filter can be replaced"); > + if (!bdrv_recurse_can_replace(parent_bs, to_replace_bs)) { > + error_setg(errp, "Cannot replace '%s' by a node mirrored from '%s', " > + "because it cannot be guaranteed that doing so would not " > + "lead to an abrupt change of visible data", > + node_name, parent_bs->node_name); > to_replace_bs = NULL; > goto out; > } > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Am 11.11.2019 um 17:02 hat Max Reitz geschrieben: > Let check_to_replace_node() use the more specialized > bdrv_recurse_can_replace() instead of > bdrv_recurse_is_first_non_filter(), which is too restrictive. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index de53addeb0..7608f21570 100644 > --- a/block.c > +++ b/block.c > @@ -6243,6 +6243,17 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs, > return false; > } > > +/* > + * Check whether the given @node_name can be replaced by a node that > + * has the same data as @parent_bs. If so, return @node_name's BDS; > + * NULL otherwise. > + * > + * @node_name must be a (recursive) *child of @parent_bs (or this > + * function will return NULL). > + * > + * The result (whether the node can be replaced or not) is only valid > + * for as long as no graph changes occur. > + */ > BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > const char *node_name, Error **errp) > { > @@ -6267,8 +6278,11 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > * Another benefit is that this tests exclude backing files which are > * blocked by the backing blockers. > */ > - if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { > - error_setg(errp, "Only top most non filter can be replaced"); > + if (!bdrv_recurse_can_replace(parent_bs, to_replace_bs)) { > + error_setg(errp, "Cannot replace '%s' by a node mirrored from '%s', " > + "because it cannot be guaranteed that doing so would not " > + "lead to an abrupt change of visible data", > + node_name, parent_bs->node_name); If this function is only supposed to be used in the context of the mirror job, moving it into block/mirror.c could be considered as a cleanup on top. Kevin
diff --git a/block.c b/block.c index de53addeb0..7608f21570 100644 --- a/block.c +++ b/block.c @@ -6243,6 +6243,17 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs, return false; } +/* + * Check whether the given @node_name can be replaced by a node that + * has the same data as @parent_bs. If so, return @node_name's BDS; + * NULL otherwise. + * + * @node_name must be a (recursive) *child of @parent_bs (or this + * function will return NULL). + * + * The result (whether the node can be replaced or not) is only valid + * for as long as no graph changes occur. + */ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, const char *node_name, Error **errp) { @@ -6267,8 +6278,11 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, * Another benefit is that this tests exclude backing files which are * blocked by the backing blockers. */ - if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { - error_setg(errp, "Only top most non filter can be replaced"); + if (!bdrv_recurse_can_replace(parent_bs, to_replace_bs)) { + error_setg(errp, "Cannot replace '%s' by a node mirrored from '%s', " + "because it cannot be guaranteed that doing so would not " + "lead to an abrupt change of visible data", + node_name, parent_bs->node_name); to_replace_bs = NULL; goto out; }
Let check_to_replace_node() use the more specialized bdrv_recurse_can_replace() instead of bdrv_recurse_is_first_non_filter(), which is too restrictive. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)