| Message ID | 20240328091615.335620-1-alexander.ivanov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] blockcommit: Reopen base image as RO after abort | expand |
On 28.03.24 12:16, Alexander Ivanov wrote: > If a blockcommit is aborted the base image remains in RW mode, that leads > to a fail of subsequent live migration. > > How to reproduce: > $ virsh snapshot-create-as vm snp1 --disk-only > > *** write something to the disk inside the guest *** > > $ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort > $ lsof /vzt/vm.qcow2 > COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME > qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2 > $ cat /proc/433203/fdinfo/45 > pos: 0 > flags: 02140002 <==== The last 2 means RW mode > > If the base image is in RW mode at the end of blockcommit and was in RO > mode before blockcommit, reopen the base BDS in RO. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > block/mirror.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 1bdce3b657..d23be57255 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -93,6 +93,7 @@ typedef struct MirrorBlockJob { > int64_t active_write_bytes_in_flight; > bool prepared; > bool in_drain; > + bool base_ro; > } MirrorBlockJob; > > typedef struct MirrorBDSOpaque { > @@ -797,6 +798,10 @@ static int mirror_exit_common(Job *job) > bdrv_drained_end(target_bs); > bdrv_unref(target_bs); > > + if (abort && s->base_ro && !bdrv_is_read_only(target_bs)) { > + bdrv_reopen_set_read_only(target_bs, true, NULL); > + } > + All looks good to me except this: seems it is safer to place this "if" block before "bdrv_drained_end(); bdrv_unref();" above. With it moved: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > bs_opaque->job = NULL; > > bdrv_drained_end(src); > @@ -1717,6 +1722,7 @@ static BlockJob *mirror_start_job( > bool is_none_mode, BlockDriverState *base, > bool auto_complete, const char *filter_node_name, > bool is_mirror, MirrorCopyMode copy_mode, > + bool base_ro, > Error **errp) > { > MirrorBlockJob *s; > @@ -1800,6 +1806,7 @@ static BlockJob *mirror_start_job( > bdrv_unref(mirror_top_bs); > > s->mirror_top_bs = mirror_top_bs; > + s->base_ro = base_ro; > > /* No resize for the target either; while the mirror is still running, a > * consistent read isn't necessarily possible. We could possibly allow > @@ -2029,7 +2036,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, > speed, granularity, buf_size, backing_mode, zero_target, > on_source_error, on_target_error, unmap, NULL, NULL, > &mirror_job_driver, is_none_mode, base, false, > - filter_node_name, true, copy_mode, errp); > + filter_node_name, true, copy_mode, false, errp); > } > > BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, > @@ -2058,7 +2065,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, > on_error, on_error, true, cb, opaque, > &commit_active_job_driver, false, base, auto_complete, > filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND, > - errp); > + base_read_only, errp); > if (!job) { > goto error_restore_flags; > }
diff --git a/block/mirror.c b/block/mirror.c index 1bdce3b657..d23be57255 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -93,6 +93,7 @@ typedef struct MirrorBlockJob { int64_t active_write_bytes_in_flight; bool prepared; bool in_drain; + bool base_ro; } MirrorBlockJob; typedef struct MirrorBDSOpaque { @@ -797,6 +798,10 @@ static int mirror_exit_common(Job *job) bdrv_drained_end(target_bs); bdrv_unref(target_bs); + if (abort && s->base_ro && !bdrv_is_read_only(target_bs)) { + bdrv_reopen_set_read_only(target_bs, true, NULL); + } + bs_opaque->job = NULL; bdrv_drained_end(src); @@ -1717,6 +1722,7 @@ static BlockJob *mirror_start_job( bool is_none_mode, BlockDriverState *base, bool auto_complete, const char *filter_node_name, bool is_mirror, MirrorCopyMode copy_mode, + bool base_ro, Error **errp) { MirrorBlockJob *s; @@ -1800,6 +1806,7 @@ static BlockJob *mirror_start_job( bdrv_unref(mirror_top_bs); s->mirror_top_bs = mirror_top_bs; + s->base_ro = base_ro; /* No resize for the target either; while the mirror is still running, a * consistent read isn't necessarily possible. We could possibly allow @@ -2029,7 +2036,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, speed, granularity, buf_size, backing_mode, zero_target, on_source_error, on_target_error, unmap, NULL, NULL, &mirror_job_driver, is_none_mode, base, false, - filter_node_name, true, copy_mode, errp); + filter_node_name, true, copy_mode, false, errp); } BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, @@ -2058,7 +2065,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, on_error, on_error, true, cb, opaque, &commit_active_job_driver, false, base, auto_complete, filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND, - errp); + base_read_only, errp); if (!job) { goto error_restore_flags; }
If a blockcommit is aborted the base image remains in RW mode, that leads to a fail of subsequent live migration. How to reproduce: $ virsh snapshot-create-as vm snp1 --disk-only *** write something to the disk inside the guest *** $ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort $ lsof /vzt/vm.qcow2 COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2 $ cat /proc/433203/fdinfo/45 pos: 0 flags: 02140002 <==== The last 2 means RW mode If the base image is in RW mode at the end of blockcommit and was in RO mode before blockcommit, reopen the base BDS in RO. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- block/mirror.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)