Re: [RFC PATCH 2/2] qemu-img convert: Fix sparseness detection

[Vladimir Sementsov-Ogievskiy]

15.04.2021 18:22, Kevin Wolf wrote:
> In order to avoid RMW cycles, is_allocated_sectors() treats zeroed areas
> like non-zero data if the end of the checked area isn't aligned. This
> can improve the efficiency of the conversion and was introduced in
> commit 8dcd3c9b91a.
>
> However, it comes with a correctness problem: qemu-img convert is
> supposed to sparsify areas that contain only zeros, which it doesn't do
> any more. It turns out that this even happens when not only the
> unaligned area is zeroed, but also the blocks before and after it. In
> the bug report, conversion of a fragmented 10G image containing only
> zeros resulted in an image consuming 2.82 GiB even though the expected
> size is only 4 KiB.
>
> As a tradeoff between both, let's ignore zeroed sectors only after
> non-zero data to fix the alignment, but if we're only looking at zeros,
> keep them as such, even if it may mean additional RMW cycles.

Hmm.. If I understand correctly, we are going to do unaligned write-zero. And 
that helps. Doesn't that mean that alignment is wrongly detected?

> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1882917
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qemu-img.c                 | 18 ++++--------------
>   tests/qemu-iotests/122.out | 12 ++++--------
>   2 files changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index a5993682aa..ca4eba2dd1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1168,20 +1168,10 @@ static int is_allocated_sectors(const uint8_t *buf, int 
> n, int *pnum,
>       }
>   
>       tail = (sector_num + i) & (alignment - 1);
> -    if (tail) {
> -        if (is_zero && i <= tail) {
> -            /* treat unallocated areas which only consist
> -             * of a small tail as allocated. */
> -            is_zero = false;
> -        }
> -        if (!is_zero) {
> -            /* align up end offset of allocated areas. */
> -            i += alignment - tail;
> -            i = MIN(i, n);
> -        } else {
> -            /* align down end offset of zero areas. */
> -            i -= tail;
> -        }
> +    if (tail && !is_zero) {
> +        /* align up end offset of allocated areas. */
> +        i += alignment - tail;
> +        i = MIN(i, n);
>       }
>       *pnum = i;
>       return !is_zero;
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index dcc44a2304..fe0ea34164 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -199,11 +199,9 @@ convert -S 4k
>   [{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
>   { "start": 4096, "length": 4096, "depth": 0, "zero": true, "data": false},
>   { "start": 8192, "length": 4096, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 12288, "length": 4096, "depth": 0, "zero": true, "data": false},
> -{ "start": 16384, "length": 4096, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 20480, "length": 46080, "depth": 0, "zero": true, "data": false},
> -{ "start": 66560, "length": 1024, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 67584, "length": 67041280, "depth": 0, "zero": true, "data": false}]
> +{ "start": 12288, "length": 5120, "depth": 0, "zero": true, "data": false},
> +{ "start": 17408, "length": 3072, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> +{ "start": 20480, "length": 67088384, "depth": 0, "zero": true, "data": false}]
>   
>   convert -c -S 4k
>   [{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},
> @@ -215,9 +213,7 @@ convert -c -S 4k
>   
>   convert -S 8k
>   [{ "start": 0, "length": 24576, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 24576, "length": 41984, "depth": 0, "zero": true, "data": false},
> -{ "start": 66560, "length": 1024, "depth": 0, "zero": false, "data": true, 
> "offset": OFFSET},
> -{ "start": 67584, "length": 67041280, "depth": 0, "zero": true, "data": false}]
> +{ "start": 24576, "length": 67084288, "depth": 0, "zero": true, "data": false}]
>   
>   convert -c -S 8k
>   [{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},


--
Best regards,
Vladimir