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

[Vladimir Sementsov-Ogievskiy]

25.09.2020 11:26, Max Reitz wrote:
> 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.

Or just global test skip at file top

> > +    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?

We write 10M, but qcow2 also writes metadata as it wants

> > +        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.

Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this.. If I don't put 
new test in "auto", is there any chance that test would be automatically run 
somewhere?


--
Best regards,
Vladimir