| Message ID | 20200902164841.214948-3-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | mirror: Freeze backing chain for sync=top | expand |
On 02.09.20 18:48, Max Reitz wrote: > Reported-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/mirror.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 11ebffdf99..27422ab7a5 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -649,8 +649,8 @@ static int mirror_exit_common(Job *job) > src = mirror_top_bs->backing->bs; > target_bs = blk_bs(s->target); > > - if (bdrv_chain_contains(src, target_bs)) { > - bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); > + if (s->base) { > + bdrv_unfreeze_backing_chain(mirror_top_bs, s->base); > } > > bdrv_release_dirty_bitmap(s->dirty_bitmap); > @@ -1780,8 +1780,22 @@ static BlockJob *mirror_start_job( > goto fail; > } > } > + } > + > + if (s->base) { > + /* > + * For active commit or mirror with sync=top, we need to > + * freeze the backing chain down to the base, because we keep > + * pointers to it and its overlay. For other cases (mirror > + * with sync=full or sync=none), we do not, so there is no > + * need to freeze any part of the chain. > + * (s->base is non-NULL only in these cases.) > + */ > + if (target_is_backing) { > + assert(s->base == target); > + } On second thought, this assertion doesn’t hold true when mirroring to an image in the backing chain (i.e., specifically not using commit). Now, this mostly doesn’t work thanks to op blockers, but after “Deal with filters”, it’s possible. I don’t know whether it produces any sensible results, though. Should we leave it possible, or should we make mirroring to a node in the backing chain (i.e. target_is_backing && s->base != target) an error? Max > > - if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) { > + if (bdrv_freeze_backing_chain(mirror_top_bs, s->base, errp) < 0) { > goto fail; > } > } >
diff --git a/block/mirror.c b/block/mirror.c index 11ebffdf99..27422ab7a5 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -649,8 +649,8 @@ static int mirror_exit_common(Job *job) src = mirror_top_bs->backing->bs; target_bs = blk_bs(s->target); - if (bdrv_chain_contains(src, target_bs)) { - bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); + if (s->base) { + bdrv_unfreeze_backing_chain(mirror_top_bs, s->base); } bdrv_release_dirty_bitmap(s->dirty_bitmap); @@ -1780,8 +1780,22 @@ static BlockJob *mirror_start_job( goto fail; } } + } + + if (s->base) { + /* + * For active commit or mirror with sync=top, we need to + * freeze the backing chain down to the base, because we keep + * pointers to it and its overlay. For other cases (mirror + * with sync=full or sync=none), we do not, so there is no + * need to freeze any part of the chain. + * (s->base is non-NULL only in these cases.) + */ + if (target_is_backing) { + assert(s->base == target); + } - if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) { + if (bdrv_freeze_backing_chain(mirror_top_bs, s->base, errp) < 0) { goto fail; } }
Reported-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)