From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-block@nongnu.org
Cc: kwolf@redhat.com, fam@euphon.net, qemu-devel@nongnu.org,
mreitz@redhat.com, stefanha@redhat.com, den@openvz.org
Subject: Re: [PATCH v2 5/5] iotests: add commit top->base cases to 274
Date: Tue, 19 May 2020 16:13:20 -0500 [thread overview]
Message-ID: <b8b80d2b-492b-4121-a6eb-8a26aa0c70d4@redhat.com> (raw)
In-Reply-To: <20200519195501.29071-6-vsementsov@virtuozzo.com>
On 5/19/20 2:55 PM, Vladimir Sementsov-Ogievskiy wrote:
> These cases are fixed by previous patches around block_status and
> is_allocated.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/274 | 20 ++++++++++++
> tests/qemu-iotests/274.out | 65 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 85 insertions(+)
Okay, so this test fails when applied in isolation without the rest of
your series.
>
> diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
> index 5d1bf34dff..e910455f13 100755
> --- a/tests/qemu-iotests/274
> +++ b/tests/qemu-iotests/274
> @@ -115,6 +115,26 @@ with iotests.FilePath('base') as base, \
> iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
>
> + iotests.log('=== Testing qemu-img commit (top -> base) ===')
> +
> + create_chain()
> + iotests.qemu_img_log('commit', '-b', base, top)
> + iotests.img_info_log(base)
> + iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
> + iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), base)
So if I understand it, we are going from:
base 11111111
mid ----
top --------
guest 11110000
and we want to go to:
base 11110000
except that we are not properly writing the zeroes into base, because we
grabbed the wrong status, ending up with:
base 11111111
The status of top from 1M onwards is unallocated, and if we were to
commit to just mid, Kevin's truncate fixes solve that (we now zero out
the tail of mid as part of resizing it to be large enough). But you are
instead skipping mid, and committing all the way to base. So we need
_something_ that can tell qemu-img commit that even though the region
1m-2m is unallocated in top, we must behave as though the status of mid
reports it as allocated (because when reading beyond EOF in mid, we DO
read zero). Since the data is allocated not in top, but acts as though
it was allocated in mid, which is above base, then the commit operation
has to do something to preserve that allocation.
Okay, you've convinced me we have a bug. However, I'm still not sold
that patches 1 and 4 are quite the right fix. Going back to the
original setup, unpatched qemu.git head reports:
$ ./qemu-img map --output=json top.qcow2
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data":
false}]
I think what we really want is:
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 1, "zero": true, "data":
false}]
because then we would be _accurately_ reporting that the zeroes that we
read from 1m-2m come _because_ we read from mid (beyond EOF), which is
different from our current answer that the zeroes come from top (they
don't, because top deferred to mid). If we fix up qemu-img map output
to correctly report zeroes beyond EOF from the correct layer, will that
also fix up the bug we are seeing in qemu-img commit?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
next prev parent reply other threads:[~2020-05-19 21:14 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-19 19:54 [PATCH v2 0/5] fix & merge block_status_above and is_allocated_above Vladimir Sementsov-Ogievskiy
2020-05-19 19:54 ` [PATCH v2 1/5] block/io: fix bdrv_co_block_status_above Vladimir Sementsov-Ogievskiy
2020-05-19 20:41 ` Eric Blake
2020-05-19 21:13 ` Vladimir Sementsov-Ogievskiy
2020-05-19 21:48 ` Eric Blake
2020-05-20 6:16 ` Vladimir Sementsov-Ogievskiy
2020-05-19 19:54 ` [PATCH v2 2/5] block/io: bdrv_common_block_status_above: support include_base Vladimir Sementsov-Ogievskiy
2020-05-19 19:54 ` [PATCH v2 3/5] block/io: bdrv_common_block_status_above: support bs == base Vladimir Sementsov-Ogievskiy
2020-05-19 19:55 ` [PATCH v2 4/5] block/io: fix bdrv_is_allocated_above Vladimir Sementsov-Ogievskiy
2020-05-19 20:45 ` Eric Blake
2020-05-19 19:55 ` [PATCH v2 5/5] iotests: add commit top->base cases to 274 Vladimir Sementsov-Ogievskiy
2020-05-19 21:13 ` Eric Blake [this message]
2020-05-19 21:25 ` Vladimir Sementsov-Ogievskiy
2020-05-19 21:49 ` Eric Blake
2020-05-19 20:21 ` [PATCH v2 0/5] fix & merge block_status_above and is_allocated_above Eric Blake
2020-05-19 20:28 ` Vladimir Sementsov-Ogievskiy
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=b8b80d2b-492b-4121-a6eb-8a26aa0c70d4@redhat.com \
--to=eblake@redhat.com \
--cc=den@openvz.org \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.