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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello Ian,

Ian Wienand <iwienand@redhat.com> writes:

> On Wed, Aug 30, 2023 at 09:20:44AM +0100, Richard Palethorpe wrote:
>> > This is visible in the occasional divide-by-zero error, but in the
>> > bigger picture means this test is not exercising the compression path
>> > as desired.
>
>> Do zram{02,03} already do something similar?
>
> Let's go backwards and try to find a path forward...
>
> In Jan 19 2011, ecd667ecb5118a6a2805caca30823f18a355bbe2 added
> testcases/kernel/mem/zram/zram01.c to test r/w compressed block
> devices.
>
> In Apr 23 2015, 433445f6beeaa38f5ffbd723a8f392a6880b7e11 created two
> more tests
>   zram01.sh creates general purpose ram disks with different filesystems
>   zram02.sh creates block device for swap
>
> In Jun 2015, af0470f65abc62090ad22583b40c27923c48b038 moved the
> original testcases/kernel/mem/zram/zram01.c ->
> kernel/device-drivers/zram/zram03.c
>
> zram02.sh creates and adds/removes swap devices; I think this is
> sufficiently different to stand alone (I mean, maybe it could be given
> some intrinsic documentation by being called something descriptive
> like 'zram-swap-test.sh' but anyway).
>
> zram03.c (the original zram test, renamed) makes a device and fills it
> with the character 'a'.  It reads it back but doens't validate any
> statistics from the stats.
>
> zram01.sh is in concept fairly similar, but instead it makes various
> file-systems on the device and (as-is) writes zeros.  It reads back
> the stats and tries to infer correct operation from that.
>
> 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.

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

>
>> In any case I'd prefer to see a zram04 written in C if some coverage is
>> missing.
>
> I don't think adding another test really helps.
>
> I think the best course here is to fix zram01.sh to write enough
> random data to stress the compression paths and further sync to make
> it reliable.  This is what the patch proposes.
>
> If there's some agreement that the investigation above is valid, we
> could probably remove zram03.c.  It's not really doing anything
> zram01.sh doesn't do and it is not really stressing anything either.
>
> -i

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp