Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver

[Max Reitz]

On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/298     | 186 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/298.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 192 insertions(+)
>  create mode 100644 tests/qemu-iotests/298
>  create mode 100644 tests/qemu-iotests/298.out
> 
> diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
> new file mode 100644
> index 0000000000..fef10f6a7a
> --- /dev/null
> +++ b/tests/qemu-iotests/298

[...]

> +class TestPreallocateBase(iotests.QMPTestCase):

Perhaps a

@iotests.skip_if_unsupported(['preallocate'])

here?

> +    def setUp(self):
> +        iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))

[...]

> +class TestTruncate(iotests.QMPTestCase):

The same decorator could be placed here, although this class doesn’t
start a VM, and so is unaffected by the allowlist.  Still may be
relevant in case of block modules, I don’t know.

> +    def setUp(self):
> +        iotests.qemu_img_create('-f', iotests.imgfmt, disk, str(10 * MiB))
> +        iotests.qemu_img_create('-f', iotests.imgfmt, refdisk, str(10 * MiB))
> +
> +    def tearDown(self):
> +        os.remove(disk)
> +        os.remove(refdisk)
> +
> +    def do_test(self, prealloc_mode, new_size):
> +        ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M', 
> '-c',
> +                                     f'truncate -m {prealloc_mode} 
> {new_size}',
> +                                     drive_opts)
> +        self.assertEqual(ret, 0)
> +
> +        ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0 
> 10M',
> +                                     '-c',
> +                                     f'truncate -m {prealloc_mode} 
> {new_size}',
> +                                     refdisk)
> +        self.assertEqual(ret, 0)
> +
> +        stat = os.stat(disk)
> +        refstat = os.stat(refdisk)
> +
> +        # Probably we'll want preallocate filter to keep align to cluster 
> when
> +        # shrink preallocation, so, ignore small differece
> +        self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024)
> +
> +        # Preallocate filter may leak some internal clusters (for example, if
> +        # guest write far over EOF, skipping some clusters - they will remain
> +        # fallocated, preallocate filter don't care about such leaks, it 
> drops
> +        # only trailing preallocation.

True, but that isn’t what’s happening here.  (We only write 10M at 0, so
there are no gaps.)  Why do we need this 1M margin?

> +        self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512,
> +                        1024 * 1024)

[...]

> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index ff59cfd2d4..15d5f9619b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -307,6 +307,7 @@
>  295 rw
>  296 rw
>  297 meta
> +298 auto quick

I wouldn’t mark it as quick, there is at least one preallocate=full of
140M, and one of 40M, plus multiple 10M data writes and falloc
preallocations.

Also, since you mark it as “auto”, have you run this test on all
CI-relevant hosts?  (Among other things I can’t predict) I wonder how
preallocation behaves on macOS.  Just because that one was always a bit
weird about not-really-data areas.

Max

signature.asc
Description: OpenPGP digital signature