Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

On 1/16/24 15:45, Denis V. Lunev wrote:
> On 12/28/23 11:12, Alexander Ivanov wrote:
> > Now we support extensions saving and can let to work with them in
> > read-write mode.
> >
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > ---
> >   block/parallels-ext.c |  4 ----
> >   block/parallels.c     | 17 ++++-------------
> >   2 files changed, 4 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> > index c83d1ea393..195b01b109 100644
> > --- a/block/parallels-ext.c
> > +++ b/block/parallels-ext.c
> > @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, 
> > uint8_t *data, size_t data_size,
> >           return NULL;
> >       }
> >   -    /* We support format extension only for RO parallels images. */
> > -    assert(!(bs->open_flags & BDRV_O_RDWR));
> > -    bdrv_dirty_bitmap_set_readonly(bitmap, true);
> > -
> >       return bitmap;
> >   }
> >   diff --git a/block/parallels.c b/block/parallels.c
> > index a49922c6a7..d5d87984cf 100644
> > --- a/block/parallels.c
> > +++ b/block/parallels.c
> > @@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState 
> > *bs, QDict *options, int flags,
> >       }
> >         if (ph.ext_off) {
> > -        if (flags & BDRV_O_RDWR) {
> > -            /*
> > -             * It's unsafe to open image RW if there is an extension 
> > (as we
> > -             * don't support it). But parallels driver in QEMU 
> > historically
> > -             * ignores the extension, so print warning and don't care.
> > -             */
> > -            warn_report("Format Extension ignored in RW mode");
> > -        } else {
> > -            ret = parallels_read_format_extension(
> > -                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, 
> > errp);
> > -            if (ret < 0) {
> > -                goto fail;
> > -            }
> > +        ret = parallels_read_format_extension(
> > +                bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
> > +        if (ret < 0) {
> > +            goto fail;
> >           }
> >       }
> Reviewed-by: Denis V. Lunev <den@openvz.org>
This patch also deserves a note, what will happen with
format extensions clusters. According to the current
policy, we have only transient extensions, i.e.
CBT. Cluster allocation mechanism will reuse these
clusters as they are not marked as used.
Thus we should either set format extension offset
in the header to 0 or perform any other correct
measures to properly handle this.

It should also be noted, that on any QEMU crash
appropriate format extensions are to be properly
treated. We could not make them RW until this would
not be addressed as we could easily mess up with
trashed metadata.