Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
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
---
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
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
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
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
---
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
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.
--
Best regards,
Alexander Ivanov
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
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
---
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.From 2f70166ef640304726d5dfcee3e906b0ba1676dd Mon Sep 17 00:00:00 2001
From: "Denis V. Lunev"
Date: Thu, 18 Jan 2024 13:29:56 +0100
Subject: [PATCH 1/1] parallels: drop dirty bitmap data if the image was not
properly closed
This data is obsolete.
The approach is exactly the same like we use with QCOW2.
Signed-off-by: Denis V. Lunev
---
block/parallels-ext.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..54e8bb66a6 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,14 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
return 0;
case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+if (s->header_unclean) {
+/*
+ * The image was not closed correctly and thus dirty bitmap
+ * data inside the image is considered as incorrect and thus
+ * it should be dropper, exactly like we do for QCOW2.
+ */
+break;
+}
bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
if (!bitmap) {
goto fail;
--
2.34.1
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
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
---
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
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.
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
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
---
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
[PATCH v4 12/21] parallels: Let image extensions work in RW mode
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
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;
}
}
--
2.40.1
