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

[Denis V. Lunev]

On 2/28/24 11:25, Alexander Ivanov wrote:
> On 1/18/24 14:31, Denis V. Lunev wrote:
> > 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.
> Yes, all the clusters used by extensions are marked as unused
> on loading. In further work they can be reallocated for other
> purposes.
> Agree that we need to set ext_off to zero.
> > 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.
> If QEMU crashes after extensions loading there will be
> zero in the ext_off field and an inappropriate dirty bitmap
> will be ignored.
That should be considered as a minimal kludge.
Normally we should mark format extension cluster
as used and nullify references from the bitmaps
there.

Anyway, even if the ext_off is not zero and
bitmaps are present, bitmaps loading over
unclean image should not be performed. They should
be considered outdated.

Den