Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
09.07.2021 18:39, Eric Blake wrote:
Waiting until the end of the convert operation (a potentially
time-consuming task) to finally detect that we can't copy a bitmap is
bad, comparing to failing fast up front. Furthermore, this prevents
us from leaving a file behind with a bitmap that is not marked as
inconsistent even though it does not have sane contents.
This fixes the problems exposed in the previous patch to the iotest.
Signed-off-by: Eric Blake
I'm OK as is (still some notes below):
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
qemu-img.c| 30 +--
tests/qemu-iotests/tests/qemu-img-bitmaps | 2 --
tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++---
3 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7956a8996512..e84b3c530155 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+/* Check that bitmaps can be copied, or output an error */
+static int convert_check_bitmaps(BlockDriverState *src)
+{
+BdrvDirtyBitmap *bm;
+
+if (!bdrv_supports_persistent_dirty_bitmap(src)) {
+error_report("Source lacks bitmap support");
+return -1;
+}
+FOR_EACH_DIRTY_BITMAP(src, bm) {
+const char *name;
+
+if (!bdrv_dirty_bitmap_get_persistence(bm)) {
hmm it should be impossible in context of qemu-img... Still, not sure that we
need an assertion instead.. Who knows, may be we'll use some intermediate
non-persistent bitmap during convert.
+continue;
+}
+name = bdrv_dirty_bitmap_name(bm);
Nitpicking: a bit strange that you care to not initialize name before previous
check (as it's not needed), but initialize here, before next check, when name
is needed only if bitmap is inconsistent.
+if (bdrv_dirty_bitmap_inconsistent(bm)) {
You can define and intialize name here.. Or inside error_report(..);
+error_report("Cannot copy inconsistent bitmap '%s'", name);
+return -1;
+}
+}
+return 0;
+}
+
static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
{
BdrvDirtyBitmap *bm;
@@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState *src,
BlockDriverState *dst)
);
if (err) {
error_reportf_err(err, "Failed to populate bitmap %s: ", name);
+qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);
Good change, but not covered by commit message
return -1;
}
}
@@ -2552,9 +2577,8 @@ static int img_convert(int argc, char **argv)
ret = -1;
goto out;
}
-if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
-error_report("Source lacks bitmap support");
-ret = -1;
+ret = convert_check_bitmaps(blk_bs(s.src[0]));
+if (ret < 0) {
goto out;
}
}
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps
b/tests/qemu-iotests/tests/qemu-img-bitmaps
index 2f51651d0ce5..3fde95907515 100755
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
@@ -141,8 +141,6 @@ $QEMU_IMG bitmap --remove "$TEST_IMG" b1
_img_info --format-specific | _filter_irrelevant_img_info
$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
echo "unexpected success"
-# Bug - even though we failed at conversion, we left a file around with
-# a bitmap marked as not corrupt
TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
| _filter_irrelevant_img_info
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
index b762362075d1..546aaa404bba 100644
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
@@ -143,22 +143,6 @@ Format specific information:
name: b4
granularity: 65536
corrupt: false
-qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot
be used
-Try block-dirty-bitmap-remove to delete this bitmap from disk
-image: TEST_DIR/t.IMGFMT.copy
-file format: IMGFMT
-virtual size: 10 MiB (10485760 bytes)
-cluster_size: 65536
-Format specific information:
-bitmaps:
-[0]:
-flags:
-name: b0
-granularity: 65536
-[1]:
-flags:
-[0]: auto
-name: b4
-granularity: 65536
-corrupt: false
+qemu-img: Cannot copy inconsistent bitmap 'b0'
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open
'TEST_DIR/t.IMGFMT.copy': No such file or directory
*** done
--
Best regards,
Vladimir
Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
On Tue, Jul 13, 2021 at 8:48 PM Eric Blake wrote:
>
> On Sat, Jul 10, 2021 at 09:06:24PM +0300, Nir Soffer wrote:
> > On 7/9/21 6:39 PM, Eric Blake wrote:
> > > Waiting until the end of the convert operation (a potentially
> > > time-consuming task) to finally detect that we can't copy a bitmap is
> > > bad, comparing to failing fast up front. Furthermore, this prevents
> > > us from leaving a file behind with a bitmap that is not marked as
> > > inconsistent even though it does not have sane contents.
> >
> > I don't think this is an issue since qemu-img terminate with non-zero
> > exit code, and we cannot ensure that image is complete if we fail in
> > the middle of the operation for all image formats and protocols.
> >
> > For files we could use a temporary file and rename after successful
> > conversion for for raw format on block device we don't have any way
> > to mark the contents as temporary.
>
> Atomic rename into place for files is nice, but as you point out, it
> doesn't help when targetting block devices. So whatever we do to keep
> block devices robust even across temporary state changes is also
> sufficient for files, even if we can indeed improve the situation for
> files in a later patch.
I think management tools should handle this. In oVirt we keep metadata
and cluster locks for any kind of volume and we use them to mark volumes
being copied as temporary, so from our point of view proper cleanup in
failure flows is non-issue.
> > But failing fast is very important.
> >
> > > This fixes the problems exposed in the previous patch to the iotest.
> > >
> > > Signed-off-by: Eric Blake
> > > ---
> > > qemu-img.c| 30 +--
> > > tests/qemu-iotests/tests/qemu-img-bitmaps | 2 --
> > > tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++---
> > > 3 files changed, 29 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/qemu-img.c b/qemu-img.c
> > > index 7956a8996512..e84b3c530155 100644
> > > --- a/qemu-img.c
> > > +++ b/qemu-img.c
> > > @@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
> > > return s->ret;
> > > }
> > >
> > > +/* Check that bitmaps can be copied, or output an error */
> > > +static int convert_check_bitmaps(BlockDriverState *src)
> > > +{
> > > +BdrvDirtyBitmap *bm;
> > > +
> > > +if (!bdrv_supports_persistent_dirty_bitmap(src)) {
> > > +error_report("Source lacks bitmap support");
> > > +return -1;
> > > +}
> > > +FOR_EACH_DIRTY_BITMAP(src, bm) {
> > > +const char *name;
> > > +
> > > +if (!bdrv_dirty_bitmap_get_persistence(bm)) {
> > > +continue;
> > > +}
> > > +name = bdrv_dirty_bitmap_name(bm);
> > > +if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > > +error_report("Cannot copy inconsistent bitmap '%s'", name);
> >
> > We can add a useful hint:
> >
> > Try "qemu-img bitmap --remove" to delete this bitmap from disk.
>
> Yeah, that might be worthwhile.
>
> >
> > > +return -1;
> > > +}
> > > +}
> > > +return 0;
> > > +}
> > > +
> > > static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState
> > > *dst)
> > > {
> > > BdrvDirtyBitmap *bm;
> > > @@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState
> > > *src, BlockDriverState *dst)
> > > );
> > > if (err) {
> > > error_reportf_err(err, "Failed to populate bitmap %s: ",
> > > name);
> > > +qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);
> >
> > This may fail for the same reason populate failed (e.g. storage became
> > inaccessibel in the middle of the copy). Since we fail the convert, I don't
> > think it worth to try to do this kind of cleanup.
> >
> > If we have a way to disable the bitmap before merge, and enable it after
> > successful merge it make more sense, since if the operation fails we are
> > left with disabled bitmap.
>
> If we got this far, the guest-visible data WAS copied successfully.
> 'qemu-img compare' will report success. The only thing broken at this
> point is a bogus bitmap, and leaving a just-created (but empty) bitmap
> in place rather than erasing it (since we just created it a few lines
> above) is not nice. I see no problem with keeping this cleanup path
> intact, even if it is seldom reached, and even though we still exit
> the overall qemu-img convert with an error.
Sure, no reason to delay this fix. With or without hint on errors,
Reviewed-by: Nir Soffer
Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
On Sat, Jul 10, 2021 at 09:06:24PM +0300, Nir Soffer wrote:
> On 7/9/21 6:39 PM, Eric Blake wrote:
> > Waiting until the end of the convert operation (a potentially
> > time-consuming task) to finally detect that we can't copy a bitmap is
> > bad, comparing to failing fast up front. Furthermore, this prevents
> > us from leaving a file behind with a bitmap that is not marked as
> > inconsistent even though it does not have sane contents.
>
> I don't think this is an issue since qemu-img terminate with non-zero
> exit code, and we cannot ensure that image is complete if we fail in
> the middle of the operation for all image formats and protocols.
>
> For files we could use a temporary file and rename after successful
> conversion for for raw format on block device we don't have any way
> to mark the contents as temporary.
Atomic rename into place for files is nice, but as you point out, it
doesn't help when targetting block devices. So whatever we do to keep
block devices robust even across temporary state changes is also
sufficient for files, even if we can indeed improve the situation for
files in a later patch.
>
> But failing fast is very important.
>
> > This fixes the problems exposed in the previous patch to the iotest.
> >
> > Signed-off-by: Eric Blake
> > ---
> > qemu-img.c| 30 +--
> > tests/qemu-iotests/tests/qemu-img-bitmaps | 2 --
> > tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++---
> > 3 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 7956a8996512..e84b3c530155 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
> > return s->ret;
> > }
> >
> > +/* Check that bitmaps can be copied, or output an error */
> > +static int convert_check_bitmaps(BlockDriverState *src)
> > +{
> > +BdrvDirtyBitmap *bm;
> > +
> > +if (!bdrv_supports_persistent_dirty_bitmap(src)) {
> > +error_report("Source lacks bitmap support");
> > +return -1;
> > +}
> > +FOR_EACH_DIRTY_BITMAP(src, bm) {
> > +const char *name;
> > +
> > +if (!bdrv_dirty_bitmap_get_persistence(bm)) {
> > +continue;
> > +}
> > +name = bdrv_dirty_bitmap_name(bm);
> > +if (bdrv_dirty_bitmap_inconsistent(bm)) {
> > +error_report("Cannot copy inconsistent bitmap '%s'", name);
>
> We can add a useful hint:
>
> Try "qemu-img bitmap --remove" to delete this bitmap from disk.
Yeah, that might be worthwhile.
>
> > +return -1;
> > +}
> > +}
> > +return 0;
> > +}
> > +
> > static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState
> > *dst)
> > {
> > BdrvDirtyBitmap *bm;
> > @@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState
> > *src, BlockDriverState *dst)
> > );
> > if (err) {
> > error_reportf_err(err, "Failed to populate bitmap %s: ",
> > name);
> > +qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);
>
> This may fail for the same reason populate failed (e.g. storage became
> inaccessibel in the middle of the copy). Since we fail the convert, I don't
> think it worth to try to do this kind of cleanup.
>
> If we have a way to disable the bitmap before merge, and enable it after
> successful merge it make more sense, since if the operation fails we are
> left with disabled bitmap.
If we got this far, the guest-visible data WAS copied successfully.
'qemu-img compare' will report success. The only thing broken at this
point is a bogus bitmap, and leaving a just-created (but empty) bitmap
in place rather than erasing it (since we just created it a few lines
above) is not nice. I see no problem with keeping this cleanup path
intact, even if it is seldom reached, and even though we still exit
the overall qemu-img convert with an error.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
On 7/9/21 6:39 PM, Eric Blake wrote:
Waiting until the end of the convert operation (a potentially
time-consuming task) to finally detect that we can't copy a bitmap is
bad, comparing to failing fast up front. Furthermore, this prevents
us from leaving a file behind with a bitmap that is not marked as
inconsistent even though it does not have sane contents.
I don't think this is an issue since qemu-img terminate with non-zero
exit code, and we cannot ensure that image is complete if we fail in
the middle of the operation for all image formats and protocols.
For files we could use a temporary file and rename after successful
conversion for for raw format on block device we don't have any way
to mark the contents as temporary.
But failing fast is very important.
This fixes the problems exposed in the previous patch to the iotest.
Signed-off-by: Eric Blake
---
qemu-img.c| 30 +--
tests/qemu-iotests/tests/qemu-img-bitmaps | 2 --
tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++---
3 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7956a8996512..e84b3c530155 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+/* Check that bitmaps can be copied, or output an error */
+static int convert_check_bitmaps(BlockDriverState *src)
+{
+BdrvDirtyBitmap *bm;
+
+if (!bdrv_supports_persistent_dirty_bitmap(src)) {
+error_report("Source lacks bitmap support");
+return -1;
+}
+FOR_EACH_DIRTY_BITMAP(src, bm) {
+const char *name;
+
+if (!bdrv_dirty_bitmap_get_persistence(bm)) {
+continue;
+}
+name = bdrv_dirty_bitmap_name(bm);
+if (bdrv_dirty_bitmap_inconsistent(bm)) {
+error_report("Cannot copy inconsistent bitmap '%s'", name);
We can add a useful hint:
Try "qemu-img bitmap --remove" to delete this bitmap from disk.
+return -1;
+}
+}
+return 0;
+}
+
static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
{
BdrvDirtyBitmap *bm;
@@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState *src,
BlockDriverState *dst)
);
if (err) {
error_reportf_err(err, "Failed to populate bitmap %s: ", name);
+qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);
This may fail for the same reason populate failed (e.g. storage became
inaccessibel in the middle of the copy). Since we fail the convert, I
don't think it worth to try to do this kind of cleanup.
If we have a way to disable the bitmap before merge, and enable it after
successful merge it make more sense, since if the operation fails we are
left with disabled bitmap.
return -1;
}
}
@@ -2552,9 +2577,8 @@ static int img_convert(int argc, char **argv)
ret = -1;
goto out;
}
-if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
-error_report("Source lacks bitmap support");
-ret = -1;
+ret = convert_check_bitmaps(blk_bs(s.src[0]));
+if (ret < 0) {
goto out;
}
}
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps
b/tests/qemu-iotests/tests/qemu-img-bitmaps
index 2f51651d0ce5..3fde95907515 100755
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
@@ -141,8 +141,6 @@ $QEMU_IMG bitmap --remove "$TEST_IMG" b1
_img_info --format-specific | _filter_irrelevant_img_info
$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
echo "unexpected success"
-# Bug - even though we failed at conversion, we left a file around with
-# a bitmap marked as not corrupt
TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
| _filter_irrelevant_img_info
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
index b762362075d1..546aaa404bba 100644
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
@@ -143,22 +143,6 @@ Format specific information:
name: b4
granularity: 65536
corrupt: false
-qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot
be used
-Try block-dirty-bitmap-remove to delete this bitmap from disk
-image: TEST_DIR/t.IMGFMT.copy
-file format: IMGFMT
-virtual size: 10 MiB (10485760 bytes)
-cluster_size: 65536
-Format specific information:
-bitmaps:
-[0]:
-flags:
-name: b0
-granularity: 65536
-[1]:
-flags:
-[0]: auto
-name: b4
-granularity: 65536
-corrupt: false
+qemu-img: Cannot copy inconsistent bitmap 'b0'
+qemu-img: Could not open
[PATCH v2 2/3] qemu-img: Fail fast on convert --bitmaps with inconsistent bitmap
Waiting until the end of the convert operation (a potentially
time-consuming task) to finally detect that we can't copy a bitmap is
bad, comparing to failing fast up front. Furthermore, this prevents
us from leaving a file behind with a bitmap that is not marked as
inconsistent even though it does not have sane contents.
This fixes the problems exposed in the previous patch to the iotest.
Signed-off-by: Eric Blake
---
qemu-img.c| 30 +--
tests/qemu-iotests/tests/qemu-img-bitmaps | 2 --
tests/qemu-iotests/tests/qemu-img-bitmaps.out | 20 ++---
3 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7956a8996512..e84b3c530155 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2101,6 +2101,30 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+/* Check that bitmaps can be copied, or output an error */
+static int convert_check_bitmaps(BlockDriverState *src)
+{
+BdrvDirtyBitmap *bm;
+
+if (!bdrv_supports_persistent_dirty_bitmap(src)) {
+error_report("Source lacks bitmap support");
+return -1;
+}
+FOR_EACH_DIRTY_BITMAP(src, bm) {
+const char *name;
+
+if (!bdrv_dirty_bitmap_get_persistence(bm)) {
+continue;
+}
+name = bdrv_dirty_bitmap_name(bm);
+if (bdrv_dirty_bitmap_inconsistent(bm)) {
+error_report("Cannot copy inconsistent bitmap '%s'", name);
+return -1;
+}
+}
+return 0;
+}
+
static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
{
BdrvDirtyBitmap *bm;
@@ -2127,6 +2151,7 @@ static int convert_copy_bitmaps(BlockDriverState *src,
BlockDriverState *dst)
);
if (err) {
error_reportf_err(err, "Failed to populate bitmap %s: ", name);
+qmp_block_dirty_bitmap_remove(dst->node_name, name, NULL);
return -1;
}
}
@@ -2552,9 +2577,8 @@ static int img_convert(int argc, char **argv)
ret = -1;
goto out;
}
-if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
-error_report("Source lacks bitmap support");
-ret = -1;
+ret = convert_check_bitmaps(blk_bs(s.src[0]));
+if (ret < 0) {
goto out;
}
}
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps
b/tests/qemu-iotests/tests/qemu-img-bitmaps
index 2f51651d0ce5..3fde95907515 100755
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps
@@ -141,8 +141,6 @@ $QEMU_IMG bitmap --remove "$TEST_IMG" b1
_img_info --format-specific | _filter_irrelevant_img_info
$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG" "$TEST_IMG.copy" &&
echo "unexpected success"
-# Bug - even though we failed at conversion, we left a file around with
-# a bitmap marked as not corrupt
TEST_IMG=$TEST_IMG.copy _img_info --format-specific \
| _filter_irrelevant_img_info
diff --git a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
index b762362075d1..546aaa404bba 100644
--- a/tests/qemu-iotests/tests/qemu-img-bitmaps.out
+++ b/tests/qemu-iotests/tests/qemu-img-bitmaps.out
@@ -143,22 +143,6 @@ Format specific information:
name: b4
granularity: 65536
corrupt: false
-qemu-img: Failed to populate bitmap b0: Bitmap 'b0' is inconsistent and cannot
be used
-Try block-dirty-bitmap-remove to delete this bitmap from disk
-image: TEST_DIR/t.IMGFMT.copy
-file format: IMGFMT
-virtual size: 10 MiB (10485760 bytes)
-cluster_size: 65536
-Format specific information:
-bitmaps:
-[0]:
-flags:
-name: b0
-granularity: 65536
-[1]:
-flags:
-[0]: auto
-name: b4
-granularity: 65536
-corrupt: false
+qemu-img: Cannot copy inconsistent bitmap 'b0'
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT.copy': Could not open
'TEST_DIR/t.IMGFMT.copy': No such file or directory
*** done
--
2.31.1
