From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqL4o-0002BT-DY for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:06:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqL4i-00015V-0h for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:06:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39041) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqL4h-00015R-Ph for qemu-devel@nongnu.org; Mon, 17 Nov 2014 07:06:31 -0500 Message-ID: <5469E4C0.8040509@redhat.com> Date: Mon, 17 Nov 2014 13:06:24 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415970374-16811-1-git-send-email-mreitz@redhat.com> <1415970374-16811-22-git-send-email-mreitz@redhat.com> <5467682E.1030604@redhat.com> In-Reply-To: <5467682E.1030604@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 21/21] iotests: Add test for different refcount widths List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi On 2014-11-15 at 15:50, Eric Blake wrote: > On 11/14/2014 06:06 AM, Max Reitz wrote: >> Add a test for conversion between different refcount widths and errors >> specific to certain widths (i.e. snapshots with refcount_width=3D1). >> >> Signed-off-by: Max Reitz >> --- >> tests/qemu-iotests/112 | 252 +++++++++++++++++++++++++++++++++++= ++++++++++ >> tests/qemu-iotests/112.out | 131 +++++++++++++++++++++++ >> tests/qemu-iotests/group | 1 + >> 3 files changed, 384 insertions(+) >> create mode 100755 tests/qemu-iotests/112 >> create mode 100644 tests/qemu-iotests/112.out >> >> +echo >> +echo '=3D=3D=3D Testing too many references for check =3D=3D=3D' >> +echo >> + >> +IMGOPTS=3D"$IMGOPTS,refcount_width=3D1" _make_test_img 64M >> +print_refcount_width >> + >> +# This cluster should be created at 0x50000 >> +$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io >> +# Now make the second L2 entry (the L2 table should be at 0x40000) po= int to that >> +# cluster, so we have two references >> +poke_file "$TEST_IMG" $((0x40008)) "\x80\x00\x00\x00\x00\x05\x00\x00" >> + >> +# This should say "please use amend" >> +_check_test_img -r all >> + >> +# So we do that >> +$QEMU_IMG amend -o refcount_width=3D2 "$TEST_IMG" >> +print_refcount_width >> + >> +# And try again >> +_check_test_img -r all > I think this section also deserves a test that fuzzes an image with > width=3D64 to intentionally set the most significant bit of one of the > refcounts, and make sure that we gracefully diagnose it as invalid. > >> + >> +echo >> +echo '=3D=3D=3D Multiple walks necessary during amend =3D=3D=3D' >> +echo >> + >> +IMGOPTS=3D"$IMGOPTS,refcount_width=3D1,cluster_size=3D512" _make_test= _img 64k >> + >> +# Cluster 0 is the image header, clusters 1 to 4 are used by the L1 t= able, a >> +# single L2 table, the reftable and a single refblock. This creates 5= 8 data >> +# clusters (actually, the L2 table is created here, too), so in total= there are >> +# then 63 used clusters in the image. With a refcount width of 64, on= e refblock >> +# describes 64 clusters (512 bytes / 64 bits/entry =3D 64 entries), s= o this will >> +# make the first target refblock have exactly one free entry. >> +$QEMU_IO -c "write 0 $((58 * 512))" "$TEST_IMG" | _filter_qemu_io >> + >> +# Now change the refcount width; since the first target refblock has = exactly one >> +# free entry, that entry will be used to store its own reference. No = other >> +# refblocks are needed, so then the new reftable will be allocated; s= ince the >> +# first target refblock is completely filled up, this will require a = new >> +# refblock which is why the refcount width changing function will nee= d to run >> +# through everything one more time until the allocations are stable. >> +$QEMU_IMG amend -o refcount_width=3D64 "$TEST_IMG" >> +print_refcount_width > Umm, that sounds backwards from what you document. It's a good test of > the _new_ reftable needing a second round of allocations. So keep it > with corrected comments. But I think you _intended_ to write a test > that starts with a refcount_width=3D64 image and resize to a > refcount_width=3D1, where the _old_ reftable then suffers a reallocatio= n > as part of allocating refblocks for the new table. It may even help if > you add a tracepoint for every iteration through the walk function > callback, to prove we are indeed executing it 3 times instead of the > usual 2, for these test cases. I'm currently thinking about a way to test the old reftable reallocation=20 issue, and I can't find any. So, for the old reftable to require a=20 reallocation it must grow. For it to grow we need some allocation beyond=20 what it can currently represent. For this to happen during the refblock=20 allocation walk, this allocation must be the allocation of a new refblock. If the refblock is allocated beyond the current reftable's limit, this=20 means that either all clusters between free_cluster_index and that point=20 are already taken. If the reftable is then reallocated, it will=20 therefore *always* be allocated behind that refblock, which is beyond=20 its old limit. Therefore, that walk through the old reftable will never=20 miss that new allocation. So the issue can only occur if the old reftable is resized after the=20 walk through it, that is, when allocating the new reftable. That is=20 indeed an issue but I think it manifests itself basically like the issue=20 I'm testing here: There is now an area in the old refcount structures=20 which was free before but has is used now, and the allocation causing=20 that was the allocation of the new reftable. The only difference is=20 whether the it's the old or the new reftable that resides in the=20 previously free area. Thus, I think I'll leave it at this test =E2=80=93 = but if=20 you can describe to me how to create an image for a different "rewalk"=20 path, I'm all ears. Max