[PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug

Alexander Ivanov posted 12 patches 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230424093147.197643-1-alexander.ivanov@virtuozzo.com
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, "Denis V. Lunev" <den@openvz.org>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
block/parallels.c | 190 +++++++++++++++++++++++++++++++++-------------
1 file changed, 136 insertions(+), 54 deletions(-)
[PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug
Posted by Alexander Ivanov 1 year ago
Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for simplier code.

Fix incorrect condition in out-of-image check.

v11:
1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image size in sectors.
7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.

v10:
8: Add a comment.
9: Exclude unrelated changes.

v9:
3: Add (high_off == 0) case handling.
7: Move res->image_end_offset setting to parallels_check_outside_image().
8: Add a patch with a statistics calculation fix.
9: Remove redundant high_off calculation.
12: Change the condition to (off + s->cluster_size > size).

v8: Rebase on the top of the current master branch.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
   to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last cluster.
3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
   Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
   in parallels_open() and the fix in parallels_co_check() to two patches.
2: A new patch - a part of the patch 1.
   Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: Patch order was changed so the replacement is done in parallels_co_check.
   Now we use a helper to set BAT entry and mark the block dirty.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

Alexander Ivanov (12):
  parallels: Out of image offset in BAT leads to image inflation
  parallels: Fix high_off calculation in parallels_co_check()
  parallels: Fix image_end_offset and data_end after out-of-image check
  parallels: create parallels_set_bat_entry_helper() to assign BAT value
  parallels: Use generic infrastructure for BAT writing in
    parallels_co_check()
  parallels: Move check of unclean image to a separate function
  parallels: Move check of cluster outside image to a separate function
  parallels: Fix statistics calculation
  parallels: Move check of leaks to a separate function
  parallels: Move statistic collection to a separate function
  parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
  parallels: Incorrect condition in out-of-image check

 block/parallels.c | 190 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 136 insertions(+), 54 deletions(-)

-- 
2.34.1
Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug
Posted by Hanna Czenczek 1 year ago
On 24.04.23 11:31, Alexander Ivanov wrote:
> Fix image inflation when offset in BAT is out of image.
>
> Replace whole BAT syncing by flushing only dirty blocks.
>
> Move all the checks outside the main check function in
> separate functions
>
> Use WITH_QEMU_LOCK_GUARD for simplier code.
>
> Fix incorrect condition in out-of-image check.

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug
Posted by Denis V. Lunev 1 year ago
On 4/24/23 11:31, Alexander Ivanov wrote:
> Fix image inflation when offset in BAT is out of image.
>
> Replace whole BAT syncing by flushing only dirty blocks.
>
> Move all the checks outside the main check function in
> separate functions
>
> Use WITH_QEMU_LOCK_GUARD for simplier code.
>
> Fix incorrect condition in out-of-image check.
>
> v11:
> 1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image size in sectors.
> 7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.
>
> v10:
> 8: Add a comment.
> 9: Exclude unrelated changes.
>
> v9:
> 3: Add (high_off == 0) case handling.
> 7: Move res->image_end_offset setting to parallels_check_outside_image().
> 8: Add a patch with a statistics calculation fix.
> 9: Remove redundant high_off calculation.
> 12: Change the condition to (off + s->cluster_size > size).
>
> v8: Rebase on the top of the current master branch.
>
> v7:
> 1,2: Fix string lengths in the commit messages.
> 3: Fix a typo in the commit message.
>
> v6:
> 1: Move the error check inside the loop. Move file size getting
>     to the function beginning. Skip out-of-image offsets.
> 2: A new patch - don't let high_off be more than the end of the last cluster.
> 3: Set data_end without any condition.
> 7: Move data_end setting to parallels_check_outside_image().
> 8: Remove s->data_end setting from parallels_check_leak().
>     Fix 'i' type.
>
> v5:
> 2: Change the way of data_end fixing.
> 6,7: Move data_end check to parallels_check_leak().
>
> v4:
> 1: Move s->data_end fix to parallels_co_check(). Split the check
>     in parallels_open() and the fix in parallels_co_check() to two patches.
> 2: A new patch - a part of the patch 1.
>     Add a fix for data_end to parallels_co_check().
> 3: Move offset convertation to parallels_set_bat_entry().
> 4: Fix 'ret' rewriting by bdrv_co_flush() results.
> 7: Keep 'i' as uint32_t.
>
> v3:
>
> 1-8: Fix commit message.
>
> v2:
>
> 2: A new patch - a part of the splitted patch 2.
> 3: Patch order was changed so the replacement is done in parallels_co_check.
>     Now we use a helper to set BAT entry and mark the block dirty.
> 4: Revert the condition with s->header_unclean.
> 5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
> 7: Move fragmentation counting code to this function too.
> 8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
>
> Alexander Ivanov (12):
>    parallels: Out of image offset in BAT leads to image inflation
>    parallels: Fix high_off calculation in parallels_co_check()
>    parallels: Fix image_end_offset and data_end after out-of-image check
>    parallels: create parallels_set_bat_entry_helper() to assign BAT value
>    parallels: Use generic infrastructure for BAT writing in
>      parallels_co_check()
>    parallels: Move check of unclean image to a separate function
>    parallels: Move check of cluster outside image to a separate function
>    parallels: Fix statistics calculation
>    parallels: Move check of leaks to a separate function
>    parallels: Move statistic collection to a separate function
>    parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
>    parallels: Incorrect condition in out-of-image check
>
>   block/parallels.c | 190 +++++++++++++++++++++++++++++++++-------------
>   1 file changed, 136 insertions(+), 54 deletions(-)
>
I am a little bit puzzled - there are 2 series v11 sent within
15 minutes. Which one is correct?

Den
Re: [PATCH v11 00/12] parallels: Refactor the code of images checks and fix a bug
Posted by Alexander Ivanov 1 year ago
The patchests are the same, but the first one has incorrect receivers. 
Please just ignore it.
I've sent a email about it, but i mistook twice and sent it to 
qemu-devel@nongnu.org only.


On 4/26/23 17:07, Denis V. Lunev wrote:
> On 4/24/23 11:31, Alexander Ivanov wrote:
>> Fix image inflation when offset in BAT is out of image.
>>
>> Replace whole BAT syncing by flushing only dirty blocks.
>>
>> Move all the checks outside the main check function in
>> separate functions
>>
>> Use WITH_QEMU_LOCK_GUARD for simplier code.
>>
>> Fix incorrect condition in out-of-image check.
>>
>> v11:
>> 1: Use bdrv_nb_sectors() instead of bdrv_getlength() to get image 
>> size in sectors.
>> 7,9: Add coroutine_fn and GRAPH_RDLOCK annotations.
>>
>> v10:
>> 8: Add a comment.
>> 9: Exclude unrelated changes.
>>
>> v9:
>> 3: Add (high_off == 0) case handling.
>> 7: Move res->image_end_offset setting to 
>> parallels_check_outside_image().
>> 8: Add a patch with a statistics calculation fix.
>> 9: Remove redundant high_off calculation.
>> 12: Change the condition to (off + s->cluster_size > size).
>>
>> v8: Rebase on the top of the current master branch.
>>
>> v7:
>> 1,2: Fix string lengths in the commit messages.
>> 3: Fix a typo in the commit message.
>>
>> v6:
>> 1: Move the error check inside the loop. Move file size getting
>>     to the function beginning. Skip out-of-image offsets.
>> 2: A new patch - don't let high_off be more than the end of the last 
>> cluster.
>> 3: Set data_end without any condition.
>> 7: Move data_end setting to parallels_check_outside_image().
>> 8: Remove s->data_end setting from parallels_check_leak().
>>     Fix 'i' type.
>>
>> v5:
>> 2: Change the way of data_end fixing.
>> 6,7: Move data_end check to parallels_check_leak().
>>
>> v4:
>> 1: Move s->data_end fix to parallels_co_check(). Split the check
>>     in parallels_open() and the fix in parallels_co_check() to two 
>> patches.
>> 2: A new patch - a part of the patch 1.
>>     Add a fix for data_end to parallels_co_check().
>> 3: Move offset convertation to parallels_set_bat_entry().
>> 4: Fix 'ret' rewriting by bdrv_co_flush() results.
>> 7: Keep 'i' as uint32_t.
>>
>> v3:
>>
>> 1-8: Fix commit message.
>>
>> v2:
>>
>> 2: A new patch - a part of the splitted patch 2.
>> 3: Patch order was changed so the replacement is done in 
>> parallels_co_check.
>>     Now we use a helper to set BAT entry and mark the block dirty.
>> 4: Revert the condition with s->header_unclean.
>> 5: Move unrelated helper parallels_set_bat_entry creation to a 
>> separate patch.
>> 7: Move fragmentation counting code to this function too.
>> 8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
>>
>> Alexander Ivanov (12):
>>    parallels: Out of image offset in BAT leads to image inflation
>>    parallels: Fix high_off calculation in parallels_co_check()
>>    parallels: Fix image_end_offset and data_end after out-of-image check
>>    parallels: create parallels_set_bat_entry_helper() to assign BAT 
>> value
>>    parallels: Use generic infrastructure for BAT writing in
>>      parallels_co_check()
>>    parallels: Move check of unclean image to a separate function
>>    parallels: Move check of cluster outside image to a separate function
>>    parallels: Fix statistics calculation
>>    parallels: Move check of leaks to a separate function
>>    parallels: Move statistic collection to a separate function
>>    parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
>>    parallels: Incorrect condition in out-of-image check
>>
>>   block/parallels.c | 190 +++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 136 insertions(+), 54 deletions(-)
>>
> I am a little bit puzzled - there are 2 series v11 sent within
> 15 minutes. Which one is correct?
>
> Den