[LTP] [PATCH v2] kernel/device-drivers/zram/zram01.sh : don't fill from /dev/zero

[Ian Wienand]

On Thu, Sep 07, 2023 at 09:26:29AM +0100, Richard Palethorpe wrote:
> > zram01.sh has been suspect from the start, because since the original
> > upstream zram commit (8e19d540d107ee897eb9a874844060c94e2376c0)
> > zero-pages have been de-duplicated and not compressed.  I think the
> > reason it minimally works is because there's some non-zero file-system
> > metadata; but it's unreliable (hence it randomly failing, and this
> > email) and not really stressing what it wants to stress, which is the
> > actual compression paths.
> 
> Maybe it's not testing what was orginally intended, but filling the FS
> with all zero's is an important corner case. We don't want to remove
> that coverage. We at least want to check that the kernel doesn't
> crash.

I don't think this is really a corner-case.  Perhaps filling the
*device*, with no file-system, might be a corner case.  But as you
say, this isn't exactly what it is doing -- it is filling a
file-system with a file that has all zeros.  The problem is that, for
the reasons expalined in [1] but also incorporated into comments in
the patch v2 [2] this is both unreliable and I do not think a good
exercise of the compression path.

The v2 patch was written to test both the same-page and compression
paths by explicitly alternating data that should hit both.  This is
essentially doing the same thing as the current test, but with
increased reliability of being able to correlate final stats.

> > zram03.c always filled with a non-zero value -- presumably to avoid
> > the zero-page deduplication -- but I think what this missed is that
> > when same-page detection was added in ~2017 (kernel
> > 8e19d540d107ee897eb9a874844060c94e2376c0).  Since this time, it is
> > really not stressing any of the compression paths either, since every
> > page is the same.
> 
> So it is testing deduplication of non-zero pages. In general these are
> testing a degenerate case.

I take the point that 'a' is different to '0', but as written the
kernel checks for zeros the same as any other value (it walks along by
unsigned-long and if the whole page matchs the first value, records
that value).  I haven't actually sent a patch to remove zram03.c;
since it doesn't try to assert anything it's not been unreilable.  But
I do think it's not really adding much useful coverage as is.

-i

[1] https://lore.kernel.org/linux-block/ZNB2kORYiKdl3vSq@fedora19.localdomain/
[2] https://lore.kernel.org/ltp/ZPpOuK9lyWr2wZWI@fedora19.localdomain/T/#m1e037003252012ac115e57285a926db77380897f