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

["Denis V. Lunev" <den@virtuozzo.com>]

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