[PATCH v4] blockcommit: Reopen base image as RO after abort

[PATCH v4] blockcommit: Reopen base image as RO after abort

[Alexander Ivanov]

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);
+    }
+
     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;
     }
-- 
2.40.1

Re: [PATCH v4] blockcommit: Reopen base image as RO after abort

[Vladimir Sementsov-Ogievskiy]

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;
>       }

-- 
Best regards,
Vladimir