[Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Eric Blake]

Series 2-4 of my byte-based conversion missed soft freeze, so they
are now 2.11 material.  However, there are some bug fixes in those
series that we should fix now in 2.10 (patch 1 from series two
on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
to byte-based iteration" from series three on block status).

I don't know if it is worth enhancing iotest 178 to probe the size
of a 2T image.  The test is simple, and fast when patched:

$ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
$ time ./qemu-img measure -O qcow2 -f qcow2 huge
required size: 335806464
fully allocated size: 2199359062016

real	0m0.021s
user	0m0.017s
sys	0m0.004s

but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
occupies 6 megabytes on disk, so it's not that invasive.

Eric Blake (2):
  dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
  qcow2: Fix sector calculation in qcow2_measure()

 block/dirty-bitmap.c | 2 +-
 block/qcow2.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[Eric Blake]

We've been documenting the value in bytes since its introduction
in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.

Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
preparation for a rewrite to a list of dirty sectors in the next
commit 21b5683 in block.c, but the new code mistakenly started
reporting in sectors.

Fixes: https://bugzilla.redhat.com/1441460

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

---
Too late for 2.9, since the regression has been unnoticed for
nine releases. But worth putting in 2.9.1 and 2.10.

v2-v4: no change
---
 block/dirty-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 543bddb9b5..30462d4f9a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -461,7 +461,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
         BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
         BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-        info->count = bdrv_get_dirty_count(bm);
+        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
         info->granularity = bdrv_dirty_bitmap_granularity(bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
-- 
2.13.3

Re: [Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[John Snow]

On 07/21/2017 02:32 PM, Eric Blake wrote:
> We've been documenting the value in bytes since its introduction
> in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
> 
> Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
> preparation for a rewrite to a list of dirty sectors in the next
> commit 21b5683 in block.c, but the new code mistakenly started
> reporting in sectors.
> 
> Fixes: https://bugzilla.redhat.com/1441460
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

I must have forgotten about this -- Didn't this get reviewed as part of 
your byte series? Is this a resend for a stable branch?

> ---
> Too late for 2.9, since the regression has been unnoticed for
> nine releases. But worth putting in 2.9.1 and 2.10.
> 
> v2-v4: no change
> ---
>   block/dirty-bitmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 543bddb9b5..30462d4f9a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -461,7 +461,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>       QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>           BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
>           BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> -        info->count = bdrv_get_dirty_count(bm);
> +        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
>           info->granularity = bdrv_dirty_bitmap_granularity(bm);
>           info->has_name = !!bm->name;
>           info->name = g_strdup(bm->name);
> 

-- 
—js

Re: [Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

On 07/25/2017 04:28 PM, John Snow wrote:
> 
> 
> On 07/21/2017 02:32 PM, Eric Blake wrote:
>> We've been documenting the value in bytes since its introduction
>> in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
>>
>> Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
>> preparation for a rewrite to a list of dirty sectors in the next
>> commit 21b5683 in block.c, but the new code mistakenly started
>> reporting in sectors.
>>
>> Fixes: https://bugzilla.redhat.com/1441460
>>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
> 
> I must have forgotten about this -- Didn't this get reviewed as part of
> your byte series? Is this a resend for a stable branch?

Yes, it was reviewed during my part 2-of-4 series on byte-base block
status (the dirty bitmap series).  Series 1 made it into 2.10, but
series 2-4 did not; ergo, I filtered out the true bug-fixes that were
still worthy post-freeze.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[John Snow]

On 07/25/2017 05:37 PM, Eric Blake wrote:
> On 07/25/2017 04:28 PM, John Snow wrote:
>>
>>
>> On 07/21/2017 02:32 PM, Eric Blake wrote:
>>> We've been documenting the value in bytes since its introduction
>>> in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
>>>
>>> Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
>>> preparation for a rewrite to a list of dirty sectors in the next
>>> commit 21b5683 in block.c, but the new code mistakenly started
>>> reporting in sectors.
>>>
>>> Fixes: https://bugzilla.redhat.com/1441460
>>>
>>> CC: qemu-stable@nongnu.org
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>
>> I must have forgotten about this -- Didn't this get reviewed as part of
>> your byte series? Is this a resend for a stable branch?
> 
> Yes, it was reviewed during my part 2-of-4 series on byte-base block
> status (the dirty bitmap series).  Series 1 made it into 2.10, but
> series 2-4 did not; ergo, I filtered out the true bug-fixes that were
> still worthy post-freeze.
> 

Thanks for the clarification. Short term memory is a little strapped 
right now due to my move.

ACK

[Qemu-devel] [PATCH 2/2] qcow2: Fix sector calculation in qcow2_measure()

[Eric Blake]

We used MAX() instead of the intended MIN() when computing how many
sectors to view in the current loop iteration of qcow2_measure(),
and passed in a value of INT_MAX sectors instead of our more usual
limit of BDRV_REQUEST_MAX_SECTORS (the latter avoids 32-bit overflow
on conversion to bytes).  For small files, the bug is harmless:
bdrv_get_block_status_above() clamps its *pnum answer to the BDS
size, regardless of any insanely larger input request.  However, for
any file at least 2T in size, we can very easily end up going into an
infinite loop (the maximum of 0x100000000 sectors and INT_MAX is a
64-bit quantity, which becomes 0 when assigned to int; once nb_sectors
is 0, we never make progress).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 048df7e88b..d7c600b5a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3670,8 +3670,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
             for (sector_num = 0;
                  sector_num < ssize / BDRV_SECTOR_SIZE;
                  sector_num += pnum) {
-                int nb_sectors = MAX(ssize / BDRV_SECTOR_SIZE - sector_num,
-                                     INT_MAX);
+                int nb_sectors = MIN(ssize / BDRV_SECTOR_SIZE - sector_num,
+                                     BDRV_REQUEST_MAX_SECTORS);
                 BlockDriverState *file;
                 int64_t ret;

-- 
2.13.3

Re: [Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Kevin Wolf]

Am 21.07.2017 um 20:32 hat Eric Blake geschrieben:
> Series 2-4 of my byte-based conversion missed soft freeze, so they
> are now 2.11 material.  However, there are some bug fixes in those
> series that we should fix now in 2.10 (patch 1 from series two
> on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
> to byte-based iteration" from series three on block status).

Thanks, applied to the block branch.

> I don't know if it is worth enhancing iotest 178 to probe the size
> of a 2T image.  The test is simple, and fast when patched:
> 
> $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
> $ time ./qemu-img measure -O qcow2 -f qcow2 huge
> required size: 335806464
> fully allocated size: 2199359062016
> 
> real	0m0.021s
> user	0m0.017s
> sys	0m0.004s
> 
> but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
> occupies 6 megabytes on disk, so it's not that invasive.

Yes, please follow up with qemu-iotests cases for both bugs. We're only
adding the test after fixing the bug, so a hang in the buggy old code
isn't a problem.

Kevin

Re: [Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On 07/24/2017 04:28 AM, Kevin Wolf wrote:
> Am 21.07.2017 um 20:32 hat Eric Blake geschrieben:
>> Series 2-4 of my byte-based conversion missed soft freeze, so they
>> are now 2.11 material.  However, there are some bug fixes in those
>> series that we should fix now in 2.10 (patch 1 from series two
>> on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
>> to byte-based iteration" from series three on block status).
> 
> Thanks, applied to the block branch.
> 
>> I don't know if it is worth enhancing iotest 178 to probe the size
>> of a 2T image.  The test is simple, and fast when patched:
>>
>> $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
>> $ time ./qemu-img measure -O qcow2 -f qcow2 huge
>> required size: 335806464
>> fully allocated size: 2199359062016
>>
>> real	0m0.021s
>> user	0m0.017s
>> sys	0m0.004s
>>
>> but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
>> occupies 6 megabytes on disk, so it's not that invasive.
> 
> Yes, please follow up with qemu-iotests cases for both bugs. We're only
> adding the test after fixing the bug, so a hang in the buggy old code
> isn't a problem.

I've submitted as separate patches since you've already queued these;
but since you haven't done a pull request yet, we could also merge them
and submit the tests in the same patches as the fixes:
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07400.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On Fri, Jul 21, 2017 at 01:32:41PM -0500, Eric Blake wrote:
> Series 2-4 of my byte-based conversion missed soft freeze, so they
> are now 2.11 material.  However, there are some bug fixes in those
> series that we should fix now in 2.10 (patch 1 from series two
> on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
> to byte-based iteration" from series three on block status).
> 
> I don't know if it is worth enhancing iotest 178 to probe the size
> of a 2T image.  The test is simple, and fast when patched:
> 
> $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
> $ time ./qemu-img measure -O qcow2 -f qcow2 huge
> required size: 335806464
> fully allocated size: 2199359062016
> 
> real	0m0.021s
> user	0m0.017s
> sys	0m0.004s
> 
> but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
> occupies 6 megabytes on disk, so it's not that invasive.
> 
> Eric Blake (2):
>   dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
>   qcow2: Fix sector calculation in qcow2_measure()
> 
>  block/dirty-bitmap.c | 2 +-
>  block/qcow2.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> 2.13.3
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On 07/21/2017 01:32 PM, Eric Blake wrote:
> Series 2-4 of my byte-based conversion missed soft freeze, so they
> are now 2.11 material.  However, there are some bug fixes in those
> series that we should fix now in 2.10 (patch 1 from series two
> on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
> to byte-based iteration" from series three on block status).
> 
> I don't know if it is worth enhancing iotest 178 to probe the size
> of a 2T image.  The test is simple, and fast when patched:
> 
> $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
> $ time ./qemu-img measure -O qcow2 -f qcow2 huge
> required size: 335806464
> fully allocated size: 2199359062016
> 
> real	0m0.021s
> user	0m0.017s
> sys	0m0.004s
> 
> but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
> occupies 6 megabytes on disk, so it's not that invasive.
> 
> Eric Blake (2):
>   dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
>   qcow2: Fix sector calculation in qcow2_measure()

Thanks; queued on my NBD branch:
git://repo.or.cz/qemu/ericb.git nbd

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On 07/24/2017 07:05 AM, Eric Blake wrote:
> On 07/21/2017 01:32 PM, Eric Blake wrote:
>> Series 2-4 of my byte-based conversion missed soft freeze, so they
>> are now 2.11 material.  However, there are some bug fixes in those
>> series that we should fix now in 2.10 (patch 1 from series two
>> on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure()
>> to byte-based iteration" from series three on block status).
>>
>> I don't know if it is worth enhancing iotest 178 to probe the size
>> of a 2T image.  The test is simple, and fast when patched:
>>
>> $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T
>> $ time ./qemu-img measure -O qcow2 -f qcow2 huge
>> required size: 335806464
>> fully allocated size: 2199359062016
>>
>> real	0m0.021s
>> user	0m0.017s
>> sys	0m0.004s
>>
>> but the inf-loop when unpatched is annoying; meanwhile, 'huge' only
>> occupies 6 megabytes on disk, so it's not that invasive.
>>
>> Eric Blake (2):
>>   dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
>>   qcow2: Fix sector calculation in qcow2_measure()
> 
> Thanks; queued on my NBD branch:
> git://repo.or.cz/qemu/ericb.git nbd

Sorry, replied to the wrong thread.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]