From: Peter Lieven <pl@kamp.de>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 2/2] qemu-img convert: Fix sparseness detection
Date: Fri, 3 Dec 2021 12:17:14 +0100 [thread overview]
Message-ID: <f8ef0127-ea3b-6eb4-a83b-7765c764a024@kamp.de> (raw)
In-Reply-To: <YKVBc4BwX0YuKnrR@merkur.fritz.box>
Am 19.05.21 um 18:48 schrieb Kevin Wolf:
> Am 19.05.2021 um 15:24 hat Peter Lieven geschrieben:
>> Am 20.04.21 um 18:52 schrieb Vladimir Sementsov-Ogievskiy:
>>> 20.04.2021 18:04, Kevin Wolf wrote:
>>>> Am 20.04.2021 um 16:31 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 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.
>>>> This can happen (mostly raw images on block devices, I think?), but
>>>> usually it just means skipping the write because we know that the target
>>>> image is already zeroed.
>>>>
>>>> What it does mean is that if the next part is data, we'll have an
>>>> unaligned data write.
>>>>
>>>>> Doesn't that mean that alignment is wrongly detected?
>>>> The problem is that you can have bdrv_block_status_above() return the
>>>> same allocation status multiple times in a row, but *pnum can be
>>>> unaligned for the conversion.
>>>>
>>>> We only look at a single range returned by it when detecting the
>>>> alignment, so it could be that we have zero buffers for both 0-11 and
>>>> 12-16 and detect two misaligned ranges, when both together are a
>>>> perfectly aligned zeroed range.
>>>>
>>>> In theory we could try to do some lookahead and merge ranges where
>>>> possible, which should give us the perfect result, but it would make the
>>>> code considerably more complicated. (Whether we want to merge them
>>>> doesn't only depend on the block status, but possibly also on the
>>>> content of a DATA range.)
>>>>
>>>> Kevin
>>>>
>>> Oh, I understand now the problem, thanks for explanation.
>>>
>>> Hmm, yes that means, that if the whole buf is zero, is_allocated_sectors must not align it down, to be possibly "merged" with next chunk if it is zero too.
>>>
>>> But it's still good to align zeroes down, if data starts somewhere inside the buf, isn't it?
>>>
>>> what about something like this:
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index babb5573ab..d1704584a0 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1167,19 +1167,39 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum,
>>> }
>>> }
>>>
>>> + if (i == n) {
>>> + /*
>>> + * The whole buf is the same.
>>> + *
>>> + * if it's data, just return it. It's the old behavior.
>>> + *
>>> + * if it's zero, just return too. It will work good if target is alredy
>>> + * zeroed. And if next chunk is zero too we'll have no RMW and no reason
>>> + * to write data.
>>> + */
>>> + *pnum = i;
>>> + return !is_zero;
>>> + }
>>> +
>>> 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. */
>>> + /*
>>> + * For sure next sector after i is data, and it will rewrite this
>>> + * tail anyway due to RMW. So, let's just write data now.
>>> + */
>>> is_zero = false;
>>> }
>>> if (!is_zero) {
>>> - /* align up end offset of allocated areas. */
>>> + /* If possible, align up end offset of allocated areas. */
>>> i += alignment - tail;
>>> i = MIN(i, n);
>>> } else {
>>> - /* align down end offset of zero areas. */
>>> + /*
>>> + * For sure next sector after i is data, and it will rewrite this
>>> + * tail anyway due to RMW. Better is avoid RMW and write zeroes up
>>> + * to aligned bound.
>>> + */
>>> i -= tail;
>>> }
>>> }
>> I think we forgot to follow up on this. Has anyone tested this
>> suggestion?
>>
>> Otherwise, I would try to rerun the tests I did with the my old and
>> Kevins suggestion.
> I noticed earlier this week that these patches are still in my
> development branch, but didn't actually pick it up again yet. So feel
> free to try it out.
It seems this time I forgot to follow up. Is this topic still open?
Best,
Peter
next prev parent reply other threads:[~2021-12-03 11:19 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-15 15:22 [RFC PATCH 0/2] qemu-img convert: Fix sparseness detection Kevin Wolf
2021-04-15 15:22 ` [RFC PATCH 1/2] iotests: Test qemu-img convert of zeroed data cluster Kevin Wolf
2021-04-15 15:22 ` [RFC PATCH 2/2] qemu-img convert: Fix sparseness detection Kevin Wolf
2021-04-20 14:31 ` Vladimir Sementsov-Ogievskiy
2021-04-20 15:04 ` Kevin Wolf
2021-04-20 16:52 ` Vladimir Sementsov-Ogievskiy
2021-05-19 13:24 ` Peter Lieven
2021-05-19 16:48 ` Kevin Wolf
2021-12-03 11:17 ` Peter Lieven [this message]
2021-12-03 23:04 ` Vladimir Sementsov-Ogievskiy
2021-12-04 9:53 ` Peter Lieven
2021-12-17 14:43 ` Peter Lieven
2021-04-19 8:36 ` [RFC PATCH 0/2] " Peter Lieven
2021-04-19 9:13 ` Peter Lieven
2021-04-19 12:31 ` Kevin Wolf
2021-04-19 17:12 ` Peter Lieven
2021-04-20 6:49 ` Kevin Wolf
2021-04-19 11:22 ` Kevin Wolf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f8ef0127-ea3b-6eb4-a83b-7765c764a024@kamp.de \
--to=pl@kamp.de \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).