Re: [LTP] [PATCH v3] kernel/device-drivers/zram/zram01.sh : fill with compressible data

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

> Hello,
>
> I have a system (virtualized aarch64, 4.18.0 kernel) that is
> consistently failing the zram01.sh test as it tries to divide the
> memory stats by zero.  This has been reported before at [1] without
> resolution.  f045fd1de6420cc6d7db2bda0cd86fb56ff5b1c1 put a retry loop
> around the read of this value; this is essentially reverted here for
> the reasons described below.

This looks like a much better solution except that removing the retry
loop seems to open up the possibility of random failures due to
implementation details of write-backs and counter updates. Meanwhile the
rest of your changes seem perfectly compatible with a retry loop. In the
worst case it just has no effect or slows down failures.

There is no sync in zram_fill_fs and that is nice because we can see
what happens without forcing a sync. OTOH is it not an implementation
detail when the data will be written? Or rather when the same page
counters will be updated.

>
> After some investigation [2] my conclusion is that this zero value
> represents the pages allocated for compressed storage in the zram
> device, and due to same-page deduplication the extant method of
> filling with all-zeros can indeed lead us to not having any compressed
> data to measure.
>
> This is visible with the test being unstable with a divide-by-zero
> error, but in the bigger picture means this test is not exercising the
> compression path as desired.
>
> The approach here is to separate the test into two parts, one that
> keeps the existing behaviour but it now targeted explicitly at testing
> the page de-deuplication path.  For the reasons described above, there
> is no point in asserting the compression ratio of this test, so it is
> modified do a sanity check on the count of de-deuplicated pages.
>
> A second test is added that explicitly writes compressible data.  This
> also adds the sync, as discussed in prior version [3] to increase the
> reliability of testing.  We should not need to wait or re-read this
> value, as we have explicitly made data suitable to be stored
> compressed.
>
> [1] https://lists.linux.it/pipermail/ltp/2019-July/013028.html
> [2] https://lists.linux.it/pipermail/ltp/2023-August/034585.html
> [3] https://lists.linux.it/pipermail/ltp/2023-August/034560.html
>
> Signed-off-by: Ian Wienand <iwienand@redhat.com>
> ---
> V2 -> V3: separate into two distinct tests
>
>  .../kernel/device-drivers/zram/zram01.sh      | 118 +++++++++++++-----
>  1 file changed, 85 insertions(+), 33 deletions(-)
>
> diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
> index 6bc305f2c..22c5e1927 100755
> --- a/testcases/kernel/device-drivers/zram/zram01.sh
> +++ b/testcases/kernel/device-drivers/zram/zram01.sh
> @@ -4,9 +4,9 @@
>  # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
>  #
>  # Test creates several zram devices with different filesystems on them.
> -# It fills each device with zeros and checks that compression works.
> +# It fills each device and checks that compression works.
>  
> -TST_CNT=7
> +TST_CNT=8
>  TST_TESTFUNC="do_test"
>  TST_NEEDS_CMDS="awk bc dd"
>  TST_SETUP="setup"
> @@ -105,36 +105,77 @@ zram_mount()
>  	tst_res TPASS "mount of zram device(s) succeeded"
>  }
>  
> -read_mem_used_total()
> -{
> -	echo $(awk '{print $3}' $1)
> -}
> -
> -# Reads /sys/block/zram*/mm_stat until mem_used_total is not 0.
> -check_read_mem_used_total()
> +# Fill the filesystem with a file full of zeros.  This is to test the
> +# same-page/deduplication path in the kernel.  After filling the file
> +# with the same value, we should have a lot of pages recorded as
> +# "same_pages"; we arbitrarily test against a small value here to
> +# validate pages were deduplicated.
> +zram_fill_fs()
>  {
> -	local file="$1"
> -	local mem_used_total
> +    local mm_stat same_pages
> +    local b i
> +
> +    for i in $(seq $dev_start $dev_end); do
> +	tst_res TINFO "filling zram$i with zero value"
> +	b=0
> +	while true; do
> +	    dd conv=notrunc if=/dev/zero of=zram${i}/file \
> +	       oflag=append count=1 bs=1024 status=none \
> +	       >/dev/null 2>err.txt || break
> +	    b=$(($b + 1))
> +	done
> +	if [ $b -eq 0 ]; then
> +	    [ -s err.txt ] && tst_res TWARN "dd error: $(cat err.txt)"
> +	    tst_brk TBROK "cannot fill zram with zero value $i"
> +	fi
> +	rm zram${i}/file
> +	tst_res TPASS "zram$i was filled with '$b' KB"
> +
> +	if [ ! -f "/sys/block/zram$i/mm_stat" ]; then
> +	    if [ $i -eq 0 ]; then
> +		tst_res TCONF "zram compression ratio test requires zram mm_stat sysfs file"
> +	    fi
> +	    continue
> +	fi
>  
> -	tst_res TINFO "$file"
> -	cat $file >&2
> +	mm_stat=$(cat "/sys/block/zram$i/mm_stat")
> +	tst_res TINFO "stats for zram$i : $mm_stat"
>  
> -	mem_used_total=$(read_mem_used_total $file)
> -	[ "$mem_used_total" -eq 0 ] && return 1
> +	same_pages=`echo $mm_stat | awk '{print $6}'`
> +	if [ "$same_pages" -lt 10 ]; then
> +	    tst_res TFAIL "insufficient same_pages: $same_pages < 10"

Why 10?

I would think that it should be >= to the number of whole pages we
wrote or else just > the value before we wrote any pages.

The rest looks good.

-- 
Thank you,
Richard.

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