| Message ID | 20191111160216.197086-14-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: > There is no guarantee that we can still replace the node we want to > replace at the end of the mirror job. Double-check by calling > bdrv_recurse_can_replace(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/mirror.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/block/mirror.c b/block/mirror.c > index f0f2d9dff1..68a4404666 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -695,7 +695,19 @@ static int mirror_exit_common(Job *job) > * drain potential other users of the BDS before changing the graph. */ > assert(s->in_drain); > bdrv_drained_begin(target_bs); > - bdrv_replace_node(to_replace, target_bs, &local_err); > + /* > + * Cannot use check_to_replace_node() here, because that would > + * check for an op blocker on @to_replace, and we have our own > + * there. > + */ interesting, that check_to_replace_node would acquire aio context of src.. Here we acquire aio context only if s->to_replace set (above this hunk).. Isn't it a bug? If it is, it's preexisting, and not directly related to the patch, so here: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > + if (bdrv_recurse_can_replace(src, to_replace)) { > + bdrv_replace_node(to_replace, target_bs, &local_err); > + } else { > + error_setg(&local_err, "Can no longer replace '%s' by '%s', " > + "because it can no longer be guaranteed that doing so " > + "would not lead to an abrupt change of visible data", > + to_replace->node_name, target_bs->node_name); > + } > bdrv_drained_end(target_bs); > if (local_err) { > error_report_err(local_err); >
diff --git a/block/mirror.c b/block/mirror.c index f0f2d9dff1..68a4404666 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -695,7 +695,19 @@ static int mirror_exit_common(Job *job) * drain potential other users of the BDS before changing the graph. */ assert(s->in_drain); bdrv_drained_begin(target_bs); - bdrv_replace_node(to_replace, target_bs, &local_err); + /* + * Cannot use check_to_replace_node() here, because that would + * check for an op blocker on @to_replace, and we have our own + * there. + */ + if (bdrv_recurse_can_replace(src, to_replace)) { + bdrv_replace_node(to_replace, target_bs, &local_err); + } else { + error_setg(&local_err, "Can no longer replace '%s' by '%s', " + "because it can no longer be guaranteed that doing so " + "would not lead to an abrupt change of visible data", + to_replace->node_name, target_bs->node_name); + } bdrv_drained_end(target_bs); if (local_err) { error_report_err(local_err);
There is no guarantee that we can still replace the node we want to replace at the end of the mirror job. Double-check by calling bdrv_recurse_can_replace(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)