block/qcow2.h | 13 ++-- include/block/block.h | 2 - include/block/block_int.h | 11 ++- include/block/dirty-bitmap.h | 9 ++- block.c | 22 ------ block/dirty-bitmap.c | 71 +++++++++++++++--- block/qcow2-bitmap.c | 140 +++++++++++++++++++++-------------- block/qcow2.c | 2 +- blockdev.c | 34 ++++----- 9 files changed, 179 insertions(+), 125 deletions(-)
When adding new persistent dirty bitmaps, we only check constraints against currently stored bitmaps, and ignore the pending number and size of any bitmaps yet to be stored. Rework the "can_store" and "remove" interface to explicit "add" and "remove", and begin keeping track of the queued burden when adding new bitmaps. Reported-by: aihua liang <aliang@redhat.com> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 John Snow (5): block/qcow2-bitmap: allow bitmap_list_load to return an error code block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps block/qcow2-bitmap: Count queued bitmaps towards directory_size block/qcow2.h | 13 ++-- include/block/block.h | 2 - include/block/block_int.h | 11 ++- include/block/dirty-bitmap.h | 9 ++- block.c | 22 ------ block/dirty-bitmap.c | 71 +++++++++++++++--- block/qcow2-bitmap.c | 140 +++++++++++++++++++++-------------- block/qcow2.c | 2 +- blockdev.c | 34 ++++----- 9 files changed, 179 insertions(+), 125 deletions(-) -- 2.20.1
Patchew URL: https://patchew.org/QEMU/20190606184159.979-1-jsnow@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
Type: series
Message-id: 20190606184159.979-1-jsnow@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20190606184159.979-1-jsnow@redhat.com -> patchew/20190606184159.979-1-jsnow@redhat.com
Switched to a new branch 'test'
6d00dc95dd block/qcow2-bitmap: Count queued bitmaps towards directory_size
aaf1723431 block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
475f5eef64 block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
0698e46266 block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
e94f44cb88 block/qcow2-bitmap: allow bitmap_list_load to return an error code
=== OUTPUT BEGIN ===
1/5 Checking commit e94f44cb88bb (block/qcow2-bitmap: allow bitmap_list_load to return an error code)
ERROR: do not use assignment in if condition
#151: FILE: block/qcow2-bitmap.c:1130:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
ERROR: do not use assignment in if condition
#164: FILE: block/qcow2-bitmap.c:1189:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 2 errors, 0 warnings, 166 lines checked
Patch 1/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/5 Checking commit 0698e462661d (block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap)
ERROR: do not use assignment in if condition
#166: FILE: block/qcow2-bitmap.c:1656:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 1 errors, 0 warnings, 200 lines checked
Patch 2/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/5 Checking commit 475f5eef64a7 (block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap)
ERROR: do not use assignment in if condition
#91: FILE: block/qcow2-bitmap.c:1421:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 1 errors, 0 warnings, 143 lines checked
Patch 3/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/5 Checking commit aaf172343148 (block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps)
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 97 lines checked
Patch 4/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/5 Checking commit 6d00dc95ddc0 (block/qcow2-bitmap: Count queued bitmaps towards directory_size)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190606184159.979-1-jsnow@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 6/6/19 5:54 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190606184159.979-1-jsnow@redhat.com/
>
>
>
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Subject: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
> Type: series
> Message-id: 20190606184159.979-1-jsnow@redhat.com
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
>
> From https://github.com/patchew-project/qemu
> * [new tag] patchew/20190606184159.979-1-jsnow@redhat.com -> patchew/20190606184159.979-1-jsnow@redhat.com
> Switched to a new branch 'test'
> 6d00dc95dd block/qcow2-bitmap: Count queued bitmaps towards directory_size
> aaf1723431 block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
> 475f5eef64 block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
> 0698e46266 block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
> e94f44cb88 block/qcow2-bitmap: allow bitmap_list_load to return an error code
>
> === OUTPUT BEGIN ===
> 1/5 Checking commit e94f44cb88bb (block/qcow2-bitmap: allow bitmap_list_load to return an error code)
> ERROR: do not use assignment in if condition
> #151: FILE: block/qcow2-bitmap.c:1130:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> ERROR: do not use assignment in if condition
> #164: FILE: block/qcow2-bitmap.c:1189:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 2 errors, 0 warnings, 166 lines checked
>
> Patch 1/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 2/5 Checking commit 0698e462661d (block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap)
> ERROR: do not use assignment in if condition
> #166: FILE: block/qcow2-bitmap.c:1656:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 1 errors, 0 warnings, 200 lines checked
>
> Patch 2/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 3/5 Checking commit 475f5eef64a7 (block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap)
> ERROR: do not use assignment in if condition
> #91: FILE: block/qcow2-bitmap.c:1421:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 1 errors, 0 warnings, 143 lines checked
>
> Patch 3/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 4/5 Checking commit aaf172343148 (block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps)
> ERROR: Missing Signed-off-by: line(s)
>
> total: 1 errors, 0 warnings, 97 lines checked
>
> Patch 4/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 5/5 Checking commit 6d00dc95ddc0 (block/qcow2-bitmap: Count queued bitmaps towards directory_size)
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> The full log is available at
> http://patchew.org/logs/20190606184159.979-1-jsnow@redhat.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
>
Whoops, script keeps dropping SoB for some reason. I also forgot that we
don't like assignment conditionals in QEMU, sorry :(
I'll fix these up, but I'll wait for other critique first so I don't
clog mailboxes.
--js
On 6/6/19 1:41 PM, John Snow wrote: > When adding new persistent dirty bitmaps, we only check constraints > against currently stored bitmaps, and ignore the pending number and size > of any bitmaps yet to be stored. > > Rework the "can_store" and "remove" interface to explicit "add" and "remove", > and begin keeping track of the queued burden when adding new bitmaps. > > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 > > John Snow (5): > block/qcow2-bitmap: allow bitmap_list_load to return an error code > block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap > block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap > block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps > block/qcow2-bitmap: Count queued bitmaps towards directory_size Is this series worth reviving as a v2, as it solves a corner-case bug? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 10/9/19 2:57 PM, Eric Blake wrote: > On 6/6/19 1:41 PM, John Snow wrote: >> When adding new persistent dirty bitmaps, we only check constraints >> against currently stored bitmaps, and ignore the pending number and size >> of any bitmaps yet to be stored. >> >> Rework the "can_store" and "remove" interface to explicit "add" and >> "remove", >> and begin keeping track of the queued burden when adding new bitmaps. >> >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636 >> >> John Snow (5): >> block/qcow2-bitmap: allow bitmap_list_load to return an error code >> block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap >> block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap >> block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps >> block/qcow2-bitmap: Count queued bitmaps towards directory_size > > Is this series worth reviving as a v2, as it solves a corner-case bug? > > Yes, but IIRC there were some disagreements about the methodology for the fix, but can't recall exactly what right now. The way I remember it is that I wanted to make our qcow2 functions more like "do_store_bitmap" and "do_remove_bitmap" for direct addition and removal as I find that conceptual model 'simpler'. (I think it had something to do with additional complexities in the different contexts that list_load is used in for when and if it performs certain consistency checks...?) I think Vladimir wanted to use a pending-flush style cache to check requirements against instead of actual writes. --js
09.10.2019 23:44, John Snow wrote:
>
>
> On 10/9/19 2:57 PM, Eric Blake wrote:
>> On 6/6/19 1:41 PM, John Snow wrote:
>>> When adding new persistent dirty bitmaps, we only check constraints
>>> against currently stored bitmaps, and ignore the pending number and size
>>> of any bitmaps yet to be stored.
>>>
>>> Rework the "can_store" and "remove" interface to explicit "add" and
>>> "remove",
>>> and begin keeping track of the queued burden when adding new bitmaps.
>>>
>>> Reported-by: aihua liang <aliang@redhat.com>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>>>
>>> John Snow (5):
>>> block/qcow2-bitmap: allow bitmap_list_load to return an error code
>>> block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
>>> block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
>>> block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
>>> block/qcow2-bitmap: Count queued bitmaps towards directory_size
>>
>> Is this series worth reviving as a v2, as it solves a corner-case bug?
>>
>>
>
> Yes, but IIRC there were some disagreements about the methodology for
> the fix, but can't recall exactly what right now.
>
> The way I remember it is that I wanted to make our qcow2 functions more
> like "do_store_bitmap" and "do_remove_bitmap" for direct addition and
> removal as I find that conceptual model 'simpler'.
>
> (I think it had something to do with additional complexities in the
> different contexts that list_load is used in for when and if it performs
> certain consistency checks...?)
>
> I think Vladimir wanted to use a pending-flush style cache to check
> requirements against instead of actual writes.
>
Actually, here is my counter-proposal:
[PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap
<20190607184802.100945-1-vsementsov@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01696.html
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.