| Message ID | 20231228101232.372142-13-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add full dirty bitmap support | expand |
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>
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.
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; > } > } > something like attached should be taken into the account. Though the destiny of cluster with old extension offset requires some thinking. I would say that it could be marked as used on read. Anyway, this requires at least detailed thinking.
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.
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
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; } }
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(-)