RE: [PATCH] block: Don't inactivate bs if it is aleady inactive
> -Original Message-
> From: Vladimir Sementsov-Ogievskiy [mailto:vsement...@virtuozzo.com]
> Sent: Saturday, November 28, 2020 4:48 PM
> To: tuguoyi (Cloud) ; Kevin Wolf ;
> Max Reitz ; qemu-block@nongnu.org
> Cc: qemu-de...@nongnu.org; wangyong (Cloud)
> Subject: Re: [PATCH] block: Don't inactivate bs if it is aleady inactive
>
> 24.11.2020 13:04, Tuguoyi wrote:
> > The following steps will cause qemu assertion failure:
> > - pause vm
> > - save memory snapshot into local file through fd migration
> > - do the above operation again will cause qemu assertion failure
>
> Hi!
>
> Why do you need such scenario? Isn't it more correct and safer to just
> error-out on savevm if disks are already inactive? Inactive disks are a sign,
> that vm may be migrated to other host and already running, so creating any
> kind of snapshots of this old state may be bad idea. I mean, you try to allow
> a
> doubtful feature to avoid an assertion. If you don't have strong reasons for
> the
> feature, it's better to turn a crash into clean error-out.
This scenario is triggered by auto test tool which has a snapshot policy that
create
external snapshots of disk and memory periodically by virsh. But during the
test,
the virtual machine get paused due to storage shortage, so the second external
snapshot
after that point will cause qemu crash.
I agree that it's better to error-out when that case happens.
> As far as I remember, bdrv_inactive_all() is the only source of inactivation,
> so
> actually, it's more like "inactive" state of the vm, not just some disks are
> inactive.. And you change the code in a way that it looks like that some disks
> may be inactive and some not, which would actually be unexpected behavior.
Thanks for your comments, and I'll send a new patch
> >
> > The backtrace looks like:
> > #0 0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > #1 0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > #2 0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
> > #3 0x7fbf958beca2 in __assert_fail () from
> /lib/x86_64-linux-gnu/libc.so.6
> > #4 0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400)
> at /build/qemu-5.0/block.c:5724
> > #5 0x55ca8dece967 in bdrv_inactivate_all () at
> /build//qemu-5.0/block.c:5792
> > #6 0x55ca8de5539d in
> qemu_savevm_state_complete_precopy_non_iterable (inactivate_disks=true,
> in_postcopy=false, f=0x55ca907044b0)
> > at /build/qemu-5.0/migration/savevm.c:1401
> > #7 qemu_savevm_state_complete_precopy (f=0x55ca907044b0,
> iterable_only=iterable_only@entry=false,
> inactivate_disks=inactivate_disks@entry=true)
> > at /build/qemu-5.0/migration/savevm.c:1453
> > #8 0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:2941
> > #9 migration_iteration_run (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3295
> > #10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3459
> > #11 0x55ca8dfc6716 in qemu_thread_start (args=) at
> /build/qemu-5.0/util/qemu-thread-posix.c:519
> > #12 0x7fbf95c5f184 in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> > #13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6
> >
> > When the first migration completes, bs->open_flags will set
> BDRV_O_INACTIVE flag
> > by bdrv_inactivate_recurse(), and during the second migration the
> > bdrv_inactivate_recurse assert that the bs->open_flags is already
> BDRV_O_INACTIVE
> > enabled which cause crash.
> >
> > This patch just make the bdrv_inactivate_all() to don't inactivate the bs
> > if it is
> > already inactive
> >
> > Signed-off-by: Tuguoyi
> > ---
> > block.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/block.c b/block.c
> > index f1cedac..02361e1 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5938,6 +5938,11 @@ static int
> bdrv_inactivate_recurse(BlockDriverState *bs)
> > return 0;
> > }
> >
> > +static bool bdrv_is_inactive(BlockDriverState *bs)
> > +{
> > +return bs->open_flags & BDRV_O_INACTIVE;
> > +}
> > +
> > int bdrv_inactivate_all(void)
> > {
> > BlockDriverState *bs = NULL;
> > @@ -5958,7 +5963,7 @@ int bdrv_inactivate_all(void)
> > /* Nodes with BDS parents are covered by recursion from the
> last
> >* parent that gets inactivated. Don't inactivate them a second
> >* time if that has already happened. */
> > -if (bdrv_has_bds_parent(bs, false)) {
> > +if (bdrv_has_bds_parent(bs, false) || bdrv_is_inactive(bs)) {
> > continue;
> > }
> > ret = bdrv_inactivate_recurse(bs);
> >
>
>
>
> --
> Best regards,
> Vladimir
RE: [PATCH] block: Don't inactivate bs if it is aleady inactive
> The following steps will cause qemu assertion failure:
> - pause vm
> - save memory snapshot into local file through fd migration
> - do the above operation again will cause qemu assertion failure
>
> The backtrace looks like:
> #0 0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> #1 0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
> #2 0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
> #3 0x7fbf958beca2 in __assert_fail () from
> /lib/x86_64-linux-gnu/libc.so.6
> #4 0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400) at
> /build/qemu-5.0/block.c:5724
> #5 0x55ca8dece967 in bdrv_inactivate_all () at
> /build//qemu-5.0/block.c:5792
> #6 0x55ca8de5539d in
> qemu_savevm_state_complete_precopy_non_iterable (inactivate_disks=true,
> in_postcopy=false, f=0x55ca907044b0)
> at /build/qemu-5.0/migration/savevm.c:1401
> #7 qemu_savevm_state_complete_precopy (f=0x55ca907044b0,
> iterable_only=iterable_only@entry=false,
> inactivate_disks=inactivate_disks@entry=true)
> at /build/qemu-5.0/migration/savevm.c:1453
> #8 0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:2941
> #9 migration_iteration_run (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3295
> #10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3459
> #11 0x55ca8dfc6716 in qemu_thread_start (args=) at
> /build/qemu-5.0/util/qemu-thread-posix.c:519
> #12 0x7fbf95c5f184 in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> #13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6
>
> When the first migration completes, bs->open_flags will set
> BDRV_O_INACTIVE flag
> by bdrv_inactivate_recurse(), and during the second migration the
> bdrv_inactivate_recurse assert that the bs->open_flags is already
> BDRV_O_INACTIVE
> enabled which cause crash.
>
> This patch just make the bdrv_inactivate_all() to don't inactivate the bs if
> it is
> already inactive
>
> Signed-off-by: Tuguoyi
> ---
> block.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index f1cedac..02361e1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5938,6 +5938,11 @@ static int bdrv_inactivate_recurse(BlockDriverState
> *bs)
> return 0;
> }
>
> +static bool bdrv_is_inactive(BlockDriverState *bs)
> +{
> +return bs->open_flags & BDRV_O_INACTIVE;
> +}
> +
> int bdrv_inactivate_all(void)
> {
> BlockDriverState *bs = NULL;
> @@ -5958,7 +5963,7 @@ int bdrv_inactivate_all(void)
> /* Nodes with BDS parents are covered by recursion from the last
> * parent that gets inactivated. Don't inactivate them a second
> * time if that has already happened. */
> -if (bdrv_has_bds_parent(bs, false)) {
> +if (bdrv_has_bds_parent(bs, false) || bdrv_is_inactive(bs)) {
> continue;
> }
> ret = bdrv_inactivate_recurse(bs);
> --
> 2.7.4
Ping
[PATCH] block: Don't inactivate bs if it is aleady inactive
The following steps will cause qemu assertion failure:
- pause vm
- save memory snapshot into local file through fd migration
- do the above operation again will cause qemu assertion failure
The backtrace looks like:
#0 0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3 0x7fbf958beca2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#4 0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400) at
/build/qemu-5.0/block.c:5724
#5 0x55ca8dece967 in bdrv_inactivate_all () at
/build//qemu-5.0/block.c:5792
#6 0x55ca8de5539d in qemu_savevm_state_complete_precopy_non_iterable
(inactivate_disks=true, in_postcopy=false, f=0x55ca907044b0)
at /build/qemu-5.0/migration/savevm.c:1401
#7 qemu_savevm_state_complete_precopy (f=0x55ca907044b0,
iterable_only=iterable_only@entry=false,
inactivate_disks=inactivate_disks@entry=true)
at /build/qemu-5.0/migration/savevm.c:1453
#8 0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at
/build/qemu-5.0/migration/migration.c:2941
#9 migration_iteration_run (s=0x55ca8f64d9f0) at
/build/qemu-5.0/migration/migration.c:3295
#10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at
/build/qemu-5.0/migration/migration.c:3459
#11 0x55ca8dfc6716 in qemu_thread_start (args=) at
/build/qemu-5.0/util/qemu-thread-posix.c:519
#12 0x7fbf95c5f184 in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
#13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6
When the first migration completes, bs->open_flags will set BDRV_O_INACTIVE flag
by bdrv_inactivate_recurse(), and during the second migration the
bdrv_inactivate_recurse assert that the bs->open_flags is already
BDRV_O_INACTIVE
enabled which cause crash.
This patch just make the bdrv_inactivate_all() to don't inactivate the bs if it
is
already inactive
Signed-off-by: Tuguoyi
---
block.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index f1cedac..02361e1 100644
--- a/block.c
+++ b/block.c
@@ -5938,6 +5938,11 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
return 0;
}
+static bool bdrv_is_inactive(BlockDriverState *bs)
+{
+return bs->open_flags & BDRV_O_INACTIVE;
+}
+
int bdrv_inactivate_all(void)
{
BlockDriverState *bs = NULL;
@@ -5958,7 +5963,7 @@ int bdrv_inactivate_all(void)
/* Nodes with BDS parents are covered by recursion from the last
* parent that gets inactivated. Don't inactivate them a second
* time if that has already happened. */
-if (bdrv_has_bds_parent(bs, false)) {
+if (bdrv_has_bds_parent(bs, false) || bdrv_is_inactive(bs)) {
continue;
}
ret = bdrv_inactivate_recurse(bs);
--
2.7.4
--
Best regards,
Guoyi
RE: [PATCH] block: Fix integer promotion error in bdrv_getlength()
On Wed, 2019-07-03 at 10:13 -0500, Eric Blake wrote: > On 11/5/20 2:31 AM, Max Reitz wrote: > > On 05.11.20 06:40, Tuguoyi wrote: > >> As BDRV_SECTOR_SIZE is of type uint64_t, the expression will > >> automatically convert the @ret to uint64_t. When an error code > >> returned from bdrv_nb_sectors(), the promoted @ret will be a very > >> large number, as a result the -EFBIG will be returned which is not the > >> real error code. > >> > >> Signed-off-by: Guoyi Tu > >> --- > >> block.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Thanks, applied to my block branch: > > > > https://git.xanclic.moe/XanClic/qemu/commits/branch/block > > > > I actually preferred the v1 solution, rather than this v2, as it avoided > a cast. There are several type promotion bugs(commits '570542ec') found recently, so i think explicitly casting the integer type can give a hint that there is a potential type promotion if you don't do that. Actually your solution look much simple and clear, and it's ok for me -- Best regards, Guoyi
[PATCH] block: Fix integer promotion error in bdrv_getlength()
As BDRV_SECTOR_SIZE is of type uint64_t, the expression will
automatically convert the @ret to uint64_t. When an error code
returned from bdrv_nb_sectors(), the promoted @ret will be a very
large number, as a result the -EFBIG will be returned which is not the
real error code.
Signed-off-by: Guoyi Tu
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 430edf7..f14254c 100644
--- a/block.c
+++ b/block.c
@@ -5082,7 +5082,7 @@ int64_t bdrv_getlength(BlockDriverState *bs)
{
int64_t ret = bdrv_nb_sectors(bs);
-ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
+ret = ret > INT64_MAX / (int64_t)BDRV_SECTOR_SIZE ? -EFBIG : ret;
return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
}
--
2.7.4
Please ignore the [PATCH] block: Return the real error code in bdrv_getlength
--
Best regards,
Guoyi
RE: [PATCH] block: Return the real error code in bdrv_getlength
Sorry, please ignore this patch, it's not a right fix
--
Best regards,
Guoyi
> -Original Message-
> From: tuguoyi (Cloud)
> Sent: Thursday, November 05, 2020 11:11 AM
> To: 'Kevin Wolf' ; 'Max Reitz' ;
> 'qemu-block@nongnu.org'
> Cc: 'qemu-de...@nongnu.org'
> Subject: [PATCH] block: Return the real error code in bdrv_getlength
>
> The return code from bdrv_nb_sectors() should be checked before doing
> the following sanity check.
>
> Signed-off-by: Guoyi Tu
> ---
> block.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/block.c b/block.c
> index 430edf7..19ebbc0 100644
> --- a/block.c
> +++ b/block.c
> @@ -5082,6 +5082,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
> {
> int64_t ret = bdrv_nb_sectors(bs);
>
> +if (ret < 0) {
> +return ret;
> +}
> +
> ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
> return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
> }
> --
> 2.7.4
>
>
> --
> Best regards,
> Guoyi
>
[PATCH] block: Return the real error code in bdrv_getlength
The return code from bdrv_nb_sectors() should be checked before doing
the following sanity check.
Signed-off-by: Guoyi Tu
---
block.c | 4
1 file changed, 4 insertions(+)
diff --git a/block.c b/block.c
index 430edf7..19ebbc0 100644
--- a/block.c
+++ b/block.c
@@ -5082,6 +5082,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
{
int64_t ret = bdrv_nb_sectors(bs);
+if (ret < 0) {
+return ret;
+}
+
ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
}
--
2.7.4
--
Best regards,
Guoyi
[PATCH] qemu-img: Make sure @sn_opts can be deleted in all error cases
@sn_opts is initialized at the beginning, so it should be deleted
after jumping to the lable 'fail_getopt'
Signed-off-by: Guoyi Tu
---
qemu-img.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index 2103507..229cdf9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2719,7 +2719,6 @@ out:
qemu_progress_end();
qemu_opts_del(opts);
qemu_opts_free(create_opts);
-qemu_opts_del(sn_opts);
qobject_unref(open_opts);
blk_unref(s.target);
if (s.src) {
@@ -2731,6 +2730,7 @@ out:
g_free(s.src_sectors);
g_free(s.src_alignment);
fail_getopt:
+qemu_opts_del(sn_opts);
g_free(options);
return !!ret;
--
2.7.4
--
Best regards,
Guoyi
[PATCH] qcow2-cluster: Fix integer left shift error in qcow2_alloc_cluster_link_l2()
When calculating the offset, the result of left shift operation will be promoted
to type int64 automatically because the left operand of + operator is uint64_t.
but the result after integer promotion may be produce an error value for us and
trigger the following asserting error.
For example, consider i=0x2000, cluster_bits=18, the result of left shift
operation will be 0x8000. Cause argument i is of signed integer type,
the result is automatically promoted to 0x8000 which is not
we expected
The way to trigger the assertion error:
qemu-img create -f qcow2 -o preallocation=full,cluster_size=256k tmpdisk 10G
This patch fix it by casting @i to uint64_t before doing left shift operation
Signed-off-by: Guoyi Tu
---
block/qcow2-cluster.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a677ba9..550850b 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -980,7 +980,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs,
QCowL2Meta *m)
assert(l2_index + m->nb_clusters <= s->l2_slice_size);
for (i = 0; i < m->nb_clusters; i++) {
-uint64_t offset = cluster_offset + (i << s->cluster_bits);
+uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);
/* if two concurrent writes happen to the same unallocated cluster
* each write allocates separate cluster and writes data concurrently.
* The first one to complete updates l2 table with pointer to its
--
2.7.4
--
Best regards,
Guoyi
RE: util/async: File descriptor leak in aio_context_unref()?
OK, I got it, it will be released in aio_ctx_finalize(). Please ignore this mail. > -Original Message- > From: tuguoyi (Cloud) > Sent: Sunday, January 19, 2020 4:15 PM > To: 'Stefan Hajnoczi' ; 'Fam Zheng' > ; 'qemu-block@nongnu.org' > Cc: 'qemu-de...@nongnu.org' ; changlimin (Cloud) > ; chengchiwen (Cloud) ; > wangyongqing (Cloud) ; wangyong (Cloud) > > Subject: util/async: File descriptor leak in aio_context_unref()? > > In aio_context_new(), the @notifier variable will be initialized, but it > don't get cleaned up in aio_context_unref() when reference count reaches > to 0, it will cause file descriptor leak. > -- > Best regards, > Guoyi
util/async: File descriptor leak in aio_context_unref()?
In aio_context_new(), the @notifier variable will be initialized, but it don't get cleaned up in aio_context_unref() when reference count reaches to 0, it will cause file descriptor leak. -- Best regards, Guoyi
[PATCH v2] qcow2: Move error check of local_err near its assignment
The local_err check outside of the if block was necessary
when it was introduced in commit d1258dd0c87 because it needed to be
executed even if qcow2_load_autoloading_dirty_bitmaps() returned false.
After some modifications that all required the error check to remain
where it is, commit 9c98f145dfb finally moved the
qcow2_load_dirty_bitmaps() call into the if block, so now the error
check should be there, too.
Signed-off-by: Guoyi Tu
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721..ce3db29 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1705,14 +1705,14 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
update_header = update_header && !header_updated;
}
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail;
-}
if (update_header) {
ret = qcow2_update_header(bs);
--
2.7.4
RE: [PATCH] qcow2: Move error check of local_err near its assignment
On 18.12.2019 17:33 Kevin Wolf wrote:
>
> Am 18.12.2019 um 03:26 hat Tuguoyi geschrieben:
> >
> > Signed-off-by: Guoyi Tu
>
> Empty commit messages are rarely acceptable. You should explain here why
> you are making the change and why it's correct (for example by comparing
> with when it was introduced).
>
> In this case, the local_err check outside of the if block was necessary when
> it
> was introduced in commit d1258dd0c87 because it needed to be executed
> even if qcow2_load_autoloading_dirty_bitmaps() returned false.
>
> After some modifications that all required the error check to remain where it
> is, commit 9c98f145dfb finally moved the
> qcow2_load_dirty_bitmaps() call into the if block, so now the error check
> should be there, too.
>
> This is information that should be in the commit message rather than
> requiring each reader to do the research.
>
> Please also make sure to CC the author of the code that you're modifying, in
> this case Vladimir.
>
> > diff --git a/block/qcow2.c b/block/qcow2.c index 7c18721..ce3db29
> > 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1705,14 +1705,14 @@ static int coroutine_fn
> qcow2_do_open(BlockDriverState *bs, QDict *options,
> > if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
> > /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer.
> */
> > bool header_updated = qcow2_load_dirty_bitmaps(bs,
> > &local_err);
> > +if (local_err != NULL) {
> > +error_propagate(errp, local_err);
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> >
> > update_header = update_header && !header_updated;
> > }
> > -if (local_err != NULL) {
> > -error_propagate(errp, local_err);
> > -ret = -EINVAL;
> > -goto fail;
> > -}
> >
> > if (update_header) {
> > ret = qcow2_update_header(bs);
>
> The change itself looks good to me, but I'll let Vladimir have a look as
> well. If
> there are no more comments, I'm looking forward to a v2 patch with a
> non-empty commit message.
>
> Kevin
Thanks for pointing it out, I will send the v2 patch
--
Best regards,
Guoyi
[PATCH] qcow2: Move error check of local_err near its assignment
Signed-off-by: Guoyi Tu
---
block/qcow2.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721..ce3db29 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1705,14 +1705,14 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
update_header = update_header && !header_updated;
}
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail;
-}
if (update_header) {
ret = qcow2_update_header(bs);
--
2.7.4
RE: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
On 01.11.2019 18:09, Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 12:34, Tuguoyi wrote:
> > On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote:
> >> 01.11.2019 10:37, Tuguoyi wrote:
> >>> There are two issues in In check_constraints_on_bitmap(),
> >>> 1) The sanity check on the granularity will cause uint64_t integer
> >>> left-shift overflow when cluster_size is 2M and the granularity is
> >>> BIGGER than 32K.
> >>> 2) The way to calculate image size that the maximum bitmap supported
> >>> can map to is a bit incorrect.
> >>> This patch fix it by add a helper function to calculate the number
> >>> of bytes needed by a normal bitmap in image and compare it to the
> >>> maximum bitmap bytes supported by qemu.
> >>>
> >>> Fixes: 5f72826e7fc62167cf3a
> >>> Signed-off-by: Guoyi Tu
> >>
> >> You forget my
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
> >
> > Sorry for that, it's my first time to submit patch to qemu, and should I
> > send
> another patch or not ?
>
> Good start!
>
> No, you shouldn't. Maintainer will take such marks (and may be other
> proposed improvements for commit message like this "Fixes: ", when applying,
> so no reason to resend.
>
> Also, when sending a new version of patch, summarize what was changed
> since previous version (you may do it under "---" which follows commit
> message, or in cover letter if it's a patch set with several patches.
OK, I got it, thanks a lot.
> >
> >> (I don't see changes except add "Fixes: " to commit msg and put
> >> get_bitmap_bytes_needed definition header into one line.)
> >
> > Yes, Only minor changes are made, including removing 'inline' keyword.
> >
> > Thanks for your patience in reviewing >
> >>> ---
> >>>block/qcow2-bitmap.c | 14 +++---
> >>>1 file changed, 11 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> >>> 98294a7..ef9ef62 100644
> >>> --- a/block/qcow2-bitmap.c
> >>> +++ b/block/qcow2-bitmap.c
> >>> @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry,
> >>> int
> >> cluster_size)
> >>>return 0;
> >>>}
> >>>
> >>> +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t
> >>> +granularity) {
> >>> +int64_t num_bits = DIV_ROUND_UP(len, granularity);
> >>> +
> >>> +return DIV_ROUND_UP(num_bits, 8); }
> >>> +
> >>>static int check_constraints_on_bitmap(BlockDriverState *bs,
> >>> const char *name,
> >>> uint32_t granularity,
> @@
> >>> -150,6 +157,7 @@ static int
> >>> check_constraints_on_bitmap(BlockDriverState
> >> *bs,
> >>>BDRVQcow2State *s = bs->opaque;
> >>>int granularity_bits = ctz32(granularity);
> >>>int64_t len = bdrv_getlength(bs);
> >>> +int64_t bitmap_bytes;
> >>>
> >>>assert(granularity > 0);
> >>>assert((granularity & (granularity - 1)) == 0); @@ -171,9
> >>> +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
> >>>return -EINVAL;
> >>>}
> >>>
> >>> -if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> >>> -(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> >>> - granularity_bits))
> >>> +bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
> >>> +if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
> >>> +(bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE *
> >>> + s->cluster_size))
> >>>{
> >>>error_setg(errp, "Too much space will be occupied by the
> >> bitmap. "
> >>> "Use larger granularity");
> >>>
> >>
> >>
> >> --
> >> Best regards,
> >> Vladimir
>
>
> --
> Best regards,
> Vladimir
RE: [PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
On 01.11.2019 17:25 Vladimir Sementsov-Ogievskiy wrote:
> 01.11.2019 10:37, Tuguoyi wrote:
> > There are two issues in In check_constraints_on_bitmap(),
> > 1) The sanity check on the granularity will cause uint64_t integer
> > left-shift overflow when cluster_size is 2M and the granularity is
> > BIGGER than 32K.
> > 2) The way to calculate image size that the maximum bitmap supported
> > can map to is a bit incorrect.
> > This patch fix it by add a helper function to calculate the number of
> > bytes needed by a normal bitmap in image and compare it to the maximum
> > bitmap bytes supported by qemu.
> >
> > Fixes: 5f72826e7fc62167cf3a
> > Signed-off-by: Guoyi Tu
>
> You forget my
> Reviewed-by: Vladimir Sementsov-Ogievskiy
Sorry for that, it's my first time to submit patch to qemu, and should I send
another patch or not ?
> (I don't see changes except add "Fixes: " to commit msg and put
> get_bitmap_bytes_needed definition header into one line.)
Yes, Only minor changes are made, including removing 'inline' keyword.
Thanks for your patience in reviewing
> > ---
> > block/qcow2-bitmap.c | 14 +++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> > 98294a7..ef9ef62 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int
> cluster_size)
> > return 0;
> > }
> >
> > +static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t
> > +granularity) {
> > +int64_t num_bits = DIV_ROUND_UP(len, granularity);
> > +
> > +return DIV_ROUND_UP(num_bits, 8); }
> > +
> > static int check_constraints_on_bitmap(BlockDriverState *bs,
> > const char *name,
> > uint32_t granularity, @@
> > -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState
> *bs,
> > BDRVQcow2State *s = bs->opaque;
> > int granularity_bits = ctz32(granularity);
> > int64_t len = bdrv_getlength(bs);
> > +int64_t bitmap_bytes;
> >
> > assert(granularity > 0);
> > assert((granularity & (granularity - 1)) == 0); @@ -171,9 +179,9
> > @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
> > return -EINVAL;
> > }
> >
> > -if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> > -(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> > - granularity_bits))
> > +bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
> > +if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
> > +(bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE *
> > + s->cluster_size))
> > {
> > error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> > "Use larger granularity");
> >
>
>
> --
> Best regards,
> Vladimir
[PATCH v4] qcow2-bitmap: Fix uint64_t left-shift overflow
There are two issues in In check_constraints_on_bitmap(),
1) The sanity check on the granularity will cause uint64_t
integer left-shift overflow when cluster_size is 2M and the
granularity is BIGGER than 32K.
2) The way to calculate image size that the maximum bitmap
supported can map to is a bit incorrect.
This patch fix it by add a helper function to calculate the
number of bytes needed by a normal bitmap in image and compare
it to the maximum bitmap bytes supported by qemu.
Fixes: 5f72826e7fc62167cf3a
Signed-off-by: Guoyi Tu
---
block/qcow2-bitmap.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7..ef9ef62 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int
cluster_size)
return 0;
}
+static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t granularity)
+{
+int64_t num_bits = DIV_ROUND_UP(len, granularity);
+
+return DIV_ROUND_UP(num_bits, 8);
+}
+
static int check_constraints_on_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
@@ -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
int granularity_bits = ctz32(granularity);
int64_t len = bdrv_getlength(bs);
+int64_t bitmap_bytes;
assert(granularity > 0);
assert((granularity & (granularity - 1)) == 0);
@@ -171,9 +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
return -EINVAL;
}
-if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
- granularity_bits))
+bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
+if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
+(bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size))
{
error_setg(errp, "Too much space will be occupied by the bitmap. "
"Use larger granularity");
--
2.7.4
[Patch v3]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07989.html
[Patch v2]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07490.html
[Patch v1]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07336.html
[PATCH v3] qcow2-bitmap: Fix uint64_t left-shift overflow
There are two issues in In check_constraints_on_bitmap(),
1) The sanity check on the granularity will cause uint64_t
integer left-shift overflow when cluster_size is 2M and the
granularity is BIGGER than 32K.
2) The way to calculate image size that the maximum bitmap
supported can map to is a bit incorrect.
This patch fix it by add a helper function to calculate the
number of bytes needed by a normal bitmap in image and compare
it to the maximum bitmap bytes supported by qemu.
Signed-off-by: Guoyi Tu
---
block/qcow2-bitmap.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7..34935bb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -142,6 +142,14 @@ static int check_table_entry(uint64_t entry, int
cluster_size)
return 0;
}
+static inline int64_t get_bitmap_bytes_needed(int64_t len,
+ uint32_t granularity)
+{
+int64_t num_bits = DIV_ROUND_UP(len, granularity);
+
+return DIV_ROUND_UP(num_bits, 8);
+}
+
static int check_constraints_on_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
@@ -150,6 +158,7 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
int granularity_bits = ctz32(granularity);
int64_t len = bdrv_getlength(bs);
+int64_t bitmap_bytes;
assert(granularity > 0);
assert((granularity & (granularity - 1)) == 0);
@@ -171,9 +180,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
return -EINVAL;
}
-if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
- granularity_bits))
+bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
+if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
+(bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size))
{
error_setg(errp, "Too much space will be occupied by the bitmap. "
"Use larger granularity");
--
2.7.4
[Patch v2]: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07490.html
RE: [PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow
On 29.10.2019 19:57 Vladimir Sementsov-Ogievskiy wrote:
> 29.10.2019 14:14, Max Reitz wrote:
> > On 28.10.19 07:33, Tuguoyi wrote:
> >> In check_constraints_on_bitmap(), the sanity check on the granularity
> >> will cause uint64_t integer left-shift overflow when cluster_size is
> >> 2M and the granularity is BIGGER than 32K. As a result, for a qcow2
> >> disk with cluster_size set to 2M, we could not even create a dirty
> >> bitmap with default granularity. This patch fix the issue by dividing
> >> @len by granularity instead.
> >>
> >> Signed-off-by: Guoyi Tu
> >> ---
> >> block/qcow2-bitmap.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> >> 98294a7..71ac822 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -172,8 +172,8 @@ static int
> check_constraints_on_bitmap(BlockDriverState *bs,
> >> }
> >>
> >> if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
Here is the same problem, we need also to calculate the number of bits instead
of bytes.
> >> -(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> >> - granularity_bits))
> >> +(DIV_ROUND_UP(len, granularity) >
> (uint64_t)BME_MAX_TABLE_SIZE *
> >> +s->cluster_size))
> >
> > This didn’t change because of this patch, but doesn’t this comparison
> > need a conversion of bits to bytes somewhere?
> >
> > len / granularity gives us the number of bits needed for the bitmap.
> >
> > BME_MAX_TABLE_SIZE is, as far as I can see, a number of bitmap
> > clusters, so multiplying it by the cluster size gives the number of
> > bytes in the bitmap. But the number of bits is eight times higher.
>
> I think you are right. It would be better to fix it in the same patch..
>
I think it would be better to add a inline function to calculate the max bytes
of bitmap with given virtual size and granularity.
I'll send another patch later
> >
> > Another topic: Isn’t BME_MAX_TABLE_SIZE too big?
>
> Maybe) Still, I don't sure that we need to change it..
>
> > As it is, bitmap
> > tables can have a size of 1 GB, and that’s the table alone. Depending
> > on the cluster size, the bitmap would take up at least 64 GB and cover
> > at least 32 TB (at a granularity of 512 bytes).
> >
> > Max
> >
> >> {
> >> error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> >> "Use larger granularity");
> >>
> >
> >
>
>
> --
> Best regards,
> Vladimir
[PATCH v2] qcow2-bitmap: Fix uint64_t left-shift overflow
In check_constraints_on_bitmap(), the sanity check on the
granularity will cause uint64_t integer left-shift overflow
when cluster_size is 2M and the granularity is BIGGER than
32K. As a result, for a qcow2 disk with cluster_size set to
2M, we could not even create a dirty bitmap with default
granularity. This patch fix the issue by dividing @len by
granularity instead.
Signed-off-by: Guoyi Tu
---
block/qcow2-bitmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7..71ac822 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
}
if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
- granularity_bits))
+(DIV_ROUND_UP(len, granularity) > (uint64_t)BME_MAX_TABLE_SIZE *
+s->cluster_size))
{
error_setg(errp, "Too much space will be occupied by the bitmap. "
"Use larger granularity");
--
2.7.4
答复: [PATCH] qcow2-bitmap: Fix uint64_t left-shift overflow
> -邮件原件-
> 发件人: Vladimir Sementsov-Ogievskiy [mailto:vsement...@virtuozzo.com]
> 发送时间: 2019年10月27日 0:50
> 收件人: tuguoyi (Cloud) ; kw...@redhat.com;
> mre...@redhat.com; qemu-block@nongnu.org
> 抄送: chengchiwen (Cloud) ;
> qemu-de...@nongnu.org; wangyongqing (Cloud) ;
> changlimin (Cloud) ; gaoliang (Cloud)
> ; wangyong (Cloud)
> 主题: Re: [PATCH] qcow2-bitmap: Fix uint64_t left-shift overflow
>
> 26.10.2019 12:19, Tuguoyi wrote:
> > In check_constraints_on_bitmap(), the sanity check on the granularity
> > will cause uint64_t integer left-shift overflow when cluster_size is
> > 2M and the granularity is bigger than 32K which is even smaller than
> > the default value for a qcow2 disk with cluster_size set to 64k or
> > bigger. This patch fix the issue by right-shift @len instead.
> >
> > Signed-off-by: Guoyi Tu
> > ---
> > block/qcow2-bitmap.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index
> > 98294a7..2a1d789 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -172,8 +172,8 @@ static int
> check_constraints_on_bitmap(BlockDriverState *bs,
> > }
> >
> > if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> > -(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> > - granularity_bits))
>
> Hmm.
> BME_MAX_TABLE_SIZE = 0x800
>
> 0x800 * 1024 * 1024 * 2 << 16 = 2 ** 64, so for 64k granularity it
> owerflows..
> But for 32k doesn't. Or am I wrong?
You are right, it doesn't overflow for 32K.
>
> Anyway, thanks for fixing!
>
> > +((len >> granularity_bits) > (uint64_t)BME_MAX_TABLE_SIZE *
> > +s->cluster_size))
>
> It's a bit incorrect, as len may be unaligned, we need ((len + granularity -
> 1) >>
> granularity_bits) on the left, or better DIV_ROUNTD_UP(len, granularity).
Yes, @len should be ROUND-UP, thanks for pointing it out, and I'll fix it and
send another patch
>
> > {
> > error_setg(errp, "Too much space will be occupied by the
> bitmap. "
> > "Use larger granularity");
> > --
> > 2.7.4
> > --
> > ---
> > 本邮件及其附件含有新华三集团的保密信息，仅限于发送给上面地址中
> 列出
> > 的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或
> 部分地泄露、复制、
> > 或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件
> 通知发件人并删除本
> > 邮件！
> > This e-mail and its attachments contain confidential information from
> > New H3C, which is intended only for the person or entity whose address
> > is listed above. Any use of the information contained herein in any
> > way (including, but not limited to, total or partial disclosure,
> > reproduction, or dissemination) by persons other than the intended
> > recipient(s) is prohibited. If you receive this e-mail in error,
> > please notify the sender by phone or email immediately and delete it!
> >
>
> Not sure that this is possible, as it's automatically available here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07336.html
>
>
> --
> Best regards,
> Vladimir
Thanks a lot for reviewing it
[PATCH] qcow2-bitmap: Fix uint64_t left-shift overflow
In check_constraints_on_bitmap(), the sanity check on the
granularity will cause uint64_t integer left-shift overflow
when cluster_size is 2M and the granularity is bigger than
32K which is even smaller than the default value for a qcow2
disk with cluster_size set to 64k or bigger. This patch fix
the issue by right-shift @len instead.
Signed-off-by: Guoyi Tu
---
block/qcow2-bitmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7..2a1d789 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -172,8 +172,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
}
if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
- granularity_bits))
+((len >> granularity_bits) > (uint64_t)BME_MAX_TABLE_SIZE *
+s->cluster_size))
{
error_setg(errp, "Too much space will be occupied by the bitmap. "
"Use larger granularity");
--
2.7.4
-
本邮件及其附件含有新华三集团的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from New H3C,
which is
intended only for the person or entity whose address is listed above. Any use
of the
information contained herein in any way (including, but not limited to, total
or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify
the sender
by phone or email immediately and delete it!
