Re: [PATCH 2/2] iotests/block-status-cache: New test

[Hanna Reitz]

On 17.01.22 18:57, Nir Soffer wrote:

On Mon, Jan 17, 2022 at 6:26 PM Hanna Reitz  wrote:

Add a new test to verify that want_zero=false block-status calls do not
pollute the block-status cache for want_zero=true calls.

We check want_zero=true calls and their results using `qemu-img map`
(over NBD), and want_zero=false calls also using `qemu-img map` over
NBD, but using the qemu:allocation-depth context.

(This test case cannot be integrated into nbd-qemu-allocation, because
that is a qcow2 test, and this is a raw test.)

Signed-off-by: Hanna Reitz 
---
  tests/qemu-iotests/tests/block-status-cache   | 130 ++
  .../qemu-iotests/tests/block-status-cache.out |   5 +
  2 files changed, 135 insertions(+)
  create mode 100755 tests/qemu-iotests/tests/block-status-cache
  create mode 100644 tests/qemu-iotests/tests/block-status-cache.out

diff --git a/tests/qemu-iotests/tests/block-status-cache 
b/tests/qemu-iotests/tests/block-status-cache
new file mode 100755
index 00..1f2c3109d3
--- /dev/null
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -0,0 +1,130 @@
+#!/usr/bin/env python3
+# group: rw quick
+#
+# Test cases for the block-status cache.
+#
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import signal
+import iotests
+from iotests import qemu_img_create, qemu_img_pipe, qemu_nbd
+
+
+image_size = 1 * 1024 * 1024
+test_img = os.path.join(iotests.test_dir, 'test.img')
+
+nbd_pidfile = os.path.join(iotests.test_dir, 'nbd.pid')
+nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')
+
+
+class TestBscWithNbd(iotests.QMPTestCase):
+def setUp(self) -> None:
+"""Just create an empty image with a read-only NBD server on it"""
+assert qemu_img_create('-f', iotests.imgfmt, test_img,
+   str(image_size)) == 0
+
+assert qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-t', '-A', '-r',
+f'--pid-file={nbd_pidfile}', test_img) == 0

This is a good place to comment that -A (or better --allocation-depth)
is required for this test.


Sure, why not.


+
+def tearDown(self) -> None:
+with open(nbd_pidfile, encoding='utf-8') as f:
+pid = int(f.read())
+os.kill(pid, signal.SIGTERM)
+os.remove(nbd_pidfile)
+os.remove(test_img)
+
+def test_with_zero_bug(self) -> None:
+"""
+Verify that the block-status cache is not corrupted by a
+want_zero=false call.
+We can provoke a want_zero=false call with `qemu-img map` over NBD with
+x-dirty-bitmap=qemu:allocation-depth, so we first run a normal `map`

x-dirty-bitmap=qemu:allocation-depth looks a bit dirty to me but I guess
we don't have a better way without depending on another nbd client.


Should be just fine for an iotest.  If the parameter is ever reworked, 
we can easily adjust the test.



If depending on libnbd is ok, this test can be much simpler:

$ nbdinfo --map=base:allocation nbd+unix:///?socket=/tmp/nbd.sock
  040960  data
   4096  10737377283  hole,zero
$ nbdinfo --map=qemu:allocation-depth nbd+unix:///?socket=/tmp/nbd.sock
  0  10737418241  local
$ nbdinfo --map=qemu:allocation-depth nbd+unix:///?socket=/tmp/nbd.sock
  0  10737418241  local
$ nbdinfo --map=base:allocation nbd+unix:///?socket=/tmp/nbd.sock
  0  10737418240  data


+(which results in want_zero=true), then using said
+qemu:allocation-depth context, and finally another normal `map` to
+verify that the cache has not been corrupted.
+"""
+
+nbd_img_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}'
+nbd_img_opts_alloc_depth = nbd_img_opts + \
+',x-dirty-bitmap=qemu:allocation-depth'
+
+# Normal map, results in want_zero=true.
+# This will probably detect an allocated data sector first (qemu likes
+# to allocate the first sector to facilitate alignment probing), and
+# then the rest to be zero.  The BSC will thus contain (if anything)
+# one range covering the first sector.
+map_pre = qemu_img_pipe('map', '--output=json', '--image-opts',
+nbd_img_opts)
+
+# qemu:allocation-depth maps for want_zero=false.
+# want_zero=false should (with the file 

Re: [PATCH 2/2] iotests/block-status-cache: New test

[Nir Soffer]

On Mon, Jan 17, 2022 at 6:26 PM Hanna Reitz  wrote:
>
> Add a new test to verify that want_zero=false block-status calls do not
> pollute the block-status cache for want_zero=true calls.
>
> We check want_zero=true calls and their results using `qemu-img map`
> (over NBD), and want_zero=false calls also using `qemu-img map` over
> NBD, but using the qemu:allocation-depth context.
>
> (This test case cannot be integrated into nbd-qemu-allocation, because
> that is a qcow2 test, and this is a raw test.)
>
> Signed-off-by: Hanna Reitz 
> ---
>  tests/qemu-iotests/tests/block-status-cache   | 130 ++
>  .../qemu-iotests/tests/block-status-cache.out |   5 +
>  2 files changed, 135 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/block-status-cache
>  create mode 100644 tests/qemu-iotests/tests/block-status-cache.out
>
> diff --git a/tests/qemu-iotests/tests/block-status-cache 
> b/tests/qemu-iotests/tests/block-status-cache
> new file mode 100755
> index 00..1f2c3109d3
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/block-status-cache
> @@ -0,0 +1,130 @@
> +#!/usr/bin/env python3
> +# group: rw quick
> +#
> +# Test cases for the block-status cache.
> +#
> +# Copyright (C) 2022 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +import os
> +import signal
> +import iotests
> +from iotests import qemu_img_create, qemu_img_pipe, qemu_nbd
> +
> +
> +image_size = 1 * 1024 * 1024
> +test_img = os.path.join(iotests.test_dir, 'test.img')
> +
> +nbd_pidfile = os.path.join(iotests.test_dir, 'nbd.pid')
> +nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')
> +
> +
> +class TestBscWithNbd(iotests.QMPTestCase):
> +def setUp(self) -> None:
> +"""Just create an empty image with a read-only NBD server on it"""
> +assert qemu_img_create('-f', iotests.imgfmt, test_img,
> +   str(image_size)) == 0
> +
> +assert qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-t', '-A', 
> '-r',
> +f'--pid-file={nbd_pidfile}', test_img) == 0

This is a good place to comment that -A (or better --allocation-depth)
is required for this test.

> +
> +def tearDown(self) -> None:
> +with open(nbd_pidfile, encoding='utf-8') as f:
> +pid = int(f.read())
> +os.kill(pid, signal.SIGTERM)
> +os.remove(nbd_pidfile)
> +os.remove(test_img)
> +
> +def test_with_zero_bug(self) -> None:
> +"""
> +Verify that the block-status cache is not corrupted by a
> +want_zero=false call.
> +We can provoke a want_zero=false call with `qemu-img map` over NBD 
> with
> +x-dirty-bitmap=qemu:allocation-depth, so we first run a normal `map`

x-dirty-bitmap=qemu:allocation-depth looks a bit dirty to me but I guess
we don't have a better way without depending on another nbd client.

If depending on libnbd is ok, this test can be much simpler:

$ nbdinfo --map=base:allocation nbd+unix:///?socket=/tmp/nbd.sock
 040960  data
  4096  10737377283  hole,zero
$ nbdinfo --map=qemu:allocation-depth nbd+unix:///?socket=/tmp/nbd.sock
 0  10737418241  local
$ nbdinfo --map=qemu:allocation-depth nbd+unix:///?socket=/tmp/nbd.sock
 0  10737418241  local
$ nbdinfo --map=base:allocation nbd+unix:///?socket=/tmp/nbd.sock
 0  10737418240  data

> +(which results in want_zero=true), then using said
> +qemu:allocation-depth context, and finally another normal `map` to
> +verify that the cache has not been corrupted.
> +"""
> +
> +nbd_img_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}'
> +nbd_img_opts_alloc_depth = nbd_img_opts + \
> +',x-dirty-bitmap=qemu:allocation-depth'
> +
> +# Normal map, results in want_zero=true.
> +# This will probably detect an allocated data sector first (qemu 
> likes
> +# to allocate the first sector to facilitate alignment probing), and
> +# then the rest to be zero.  The BSC will thus contain (if anything)
> +# one range covering the first sector.
> +map_pre = qemu_img_pipe('map', '--output=json', '--image-opts',
> +nbd_img_opts)
> +
> +# qemu:allocation-depth maps for want_zero=false.
> +# 

[PATCH 2/2] iotests/block-status-cache: New test

[Hanna Reitz]

Add a new test to verify that want_zero=false block-status calls do not
pollute the block-status cache for want_zero=true calls.

We check want_zero=true calls and their results using `qemu-img map`
(over NBD), and want_zero=false calls also using `qemu-img map` over
NBD, but using the qemu:allocation-depth context.

(This test case cannot be integrated into nbd-qemu-allocation, because
that is a qcow2 test, and this is a raw test.)

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/tests/block-status-cache   | 130 ++
 .../qemu-iotests/tests/block-status-cache.out |   5 +
 2 files changed, 135 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/block-status-cache
 create mode 100644 tests/qemu-iotests/tests/block-status-cache.out

diff --git a/tests/qemu-iotests/tests/block-status-cache 
b/tests/qemu-iotests/tests/block-status-cache
new file mode 100755
index 00..1f2c3109d3
--- /dev/null
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -0,0 +1,130 @@
+#!/usr/bin/env python3
+# group: rw quick
+#
+# Test cases for the block-status cache.
+#
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import signal
+import iotests
+from iotests import qemu_img_create, qemu_img_pipe, qemu_nbd
+
+
+image_size = 1 * 1024 * 1024
+test_img = os.path.join(iotests.test_dir, 'test.img')
+
+nbd_pidfile = os.path.join(iotests.test_dir, 'nbd.pid')
+nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')
+
+
+class TestBscWithNbd(iotests.QMPTestCase):
+def setUp(self) -> None:
+"""Just create an empty image with a read-only NBD server on it"""
+assert qemu_img_create('-f', iotests.imgfmt, test_img,
+   str(image_size)) == 0
+
+assert qemu_nbd('-k', nbd_sock, '-f', iotests.imgfmt, '-t', '-A', '-r',
+f'--pid-file={nbd_pidfile}', test_img) == 0
+
+def tearDown(self) -> None:
+with open(nbd_pidfile, encoding='utf-8') as f:
+pid = int(f.read())
+os.kill(pid, signal.SIGTERM)
+os.remove(nbd_pidfile)
+os.remove(test_img)
+
+def test_with_zero_bug(self) -> None:
+"""
+Verify that the block-status cache is not corrupted by a
+want_zero=false call.
+We can provoke a want_zero=false call with `qemu-img map` over NBD with
+x-dirty-bitmap=qemu:allocation-depth, so we first run a normal `map`
+(which results in want_zero=true), then using said
+qemu:allocation-depth context, and finally another normal `map` to
+verify that the cache has not been corrupted.
+"""
+
+nbd_img_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}'
+nbd_img_opts_alloc_depth = nbd_img_opts + \
+',x-dirty-bitmap=qemu:allocation-depth'
+
+# Normal map, results in want_zero=true.
+# This will probably detect an allocated data sector first (qemu likes
+# to allocate the first sector to facilitate alignment probing), and
+# then the rest to be zero.  The BSC will thus contain (if anything)
+# one range covering the first sector.
+map_pre = qemu_img_pipe('map', '--output=json', '--image-opts',
+nbd_img_opts)
+
+# qemu:allocation-depth maps for want_zero=false.
+# want_zero=false should (with the file driver, which the server is
+# using) report everything as data.  While this is sufficient for
+# want_zero=false, this is nothing that should end up in the
+# block-status cache.
+# Due to a bug, this information did end up in the cache, though, and
+# this would lead to wrong information being returned on subsequent
+# want_zero=true calls.
+#
+# We need to run this map twice: On the first call, we probably still
+# have the first sector in the cache, and so this will be served from
+# the cache; and only the subsequent range will be queried from the
+# block driver.  This subsequent range will then be entered into the
+# cache.
+# If we did a want_zero=true call at this point, we would thus get
+# correct information: The first sector is not covered by the cache, so
+# we would get fresh block-status information from the driver, which
+# would