[LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[Ian Wienand]

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.  It seems like I'm not the only one, this has
been reported before at [1] without resolution.

This test does a lot of 1k writes into zram devices of size 25, 25,
25, 300 and 25 mb.  On this particular system, I consistently see the
final 25mb testing fail as it examines /sys/block/zramX/mm_stat and
finds the mem_used_total as zero.

For example, I instrumented the test to put a "sync" call in between
examining the mm_stat value if it was zero; before and after it looks
like:

 zram01 7 TINFO: /sys/block/zram34/mm_stat is zero : 10092544        0        0 26214400   196608      154        0        0
 <sync>
 zram01 7 TINFO: /sys/block/zram34/mm_stat is zero (post sync) : 26214400     2841    65536 26214400   196608      399        0        0

This is an otherwise quiet system and nothing else seems out of order
(weird logs, load average, etc.).  I think that given the 300mb worth
of writes from the prior step, the system just needs a little time to
catch up on the compression to make this test more reliable.

I considered checking if the value is zero and arbitrarily just
waiting a few seconds which did work.  I then replaced this with a
sync which also worked.  Rather than worrying about checking for zero
values, it seems more reliable and simpler to just sync after the
writes in all cases.  I could not replicate this issue with the sync
as proposed in this patch.

Additionally this does a check for a zero value to hopefully provide a
more structured message if the test fails in the same way.

[1] https://lists.linux.it/pipermail/ltp/2019-July/013028.html

Signed-off-by: Ian Wienand <iwienand@redhat.com>
---
 testcases/kernel/device-drivers/zram/zram01.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 58d233f91..5f3964f6c 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -119,6 +119,9 @@ zram_fill_fs()
 				>/dev/null 2>err.txt || break
 			b=$(($b + 1))
 		done
+                # Give the system time to catch up, otherwise the mm_stat
+                # checking below can give a false negative
+		sync
 		if [ $b -eq 0 ]; then
 			[ -s err.txt ] && tst_res TWARN "dd error: $(cat err.txt)"
 			tst_brk TBROK "cannot fill zram $i"
@@ -134,6 +137,9 @@ zram_fill_fs()
 		fi
 
 		mem_used_total=`awk '{print $3}' "/sys/block/zram$i/mm_stat"`
+		if [ $mem_used_total -eq 0 ]; then
+			test_res FAIL "/sys/block/zram$i/mm_stat reports 0 size : $(cat /sys/block/zram$i/mm_stat)"
+		fi
 		v=$((100 * 1024 * $b / $mem_used_total))
 		r=`echo "scale=2; $v / 100 " | bc`
 
-- 
2.41.0


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

Re: [LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
> index 58d233f91..5f3964f6c 100755
> --- a/testcases/kernel/device-drivers/zram/zram01.sh
> +++ b/testcases/kernel/device-drivers/zram/zram01.sh
> @@ -119,6 +119,9 @@ zram_fill_fs()
>  				>/dev/null 2>err.txt || break
>  			b=$(($b + 1))
>  		done
> +                # Give the system time to catch up, otherwise the mm_stat
> +                # checking below can give a false negative
> +		sync

I guess that the files written by the dd above end up in page cache and
are not written out until much later. Does it fix the problem if you add
fdatasync to the dd commandline? That should be faster than using the
big hammer and sysncing the whole system.

>  		if [ $b -eq 0 ]; then
>  			[ -s err.txt ] && tst_res TWARN "dd error: $(cat err.txt)"
>  			tst_brk TBROK "cannot fill zram $i"
> @@ -134,6 +137,9 @@ zram_fill_fs()
>  		fi
>  
>  		mem_used_total=`awk '{print $3}' "/sys/block/zram$i/mm_stat"`
> +		if [ $mem_used_total -eq 0 ]; then
> +			test_res FAIL "/sys/block/zram$i/mm_stat reports 0 size : $(cat /sys/block/zram$i/mm_stat)"
> +		fi
>  		v=$((100 * 1024 * $b / $mem_used_total))
>  		r=`echo "scale=2; $v / 100 " | bc`
>  
> -- 
> 2.41.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[Martin Doucha]

On 03. 08. 23 12:52, Cyril Hrubis wrote:
> Hi!
>> diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
>> index 58d233f91..5f3964f6c 100755
>> --- a/testcases/kernel/device-drivers/zram/zram01.sh
>> +++ b/testcases/kernel/device-drivers/zram/zram01.sh
>> @@ -119,6 +119,9 @@ zram_fill_fs()
>>   				>/dev/null 2>err.txt || break
>>   			b=$(($b + 1))
>>   		done
>> +                # Give the system time to catch up, otherwise the mm_stat
>> +                # checking below can give a false negative
>> +		sync
> 
> I guess that the files written by the dd above end up in page cache and
> are not written out until much later. Does it fix the problem if you add
> fdatasync to the dd commandline? That should be faster than using the
> big hammer and sysncing the whole system.

This is actually a kernel bug reported upstream:
https://lore.kernel.org/linux-block/20221107191136.18048-1-pvorel@suse.cz/

Not only the test files stay in page cache, but the whole VFAT 
superblock seems to disappear from the device as well until sync.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[Cyril Hrubis]

Hi!
> This is actually a kernel bug reported upstream:
> https://lore.kernel.org/linux-block/20221107191136.18048-1-pvorel@suse.cz/
> 
> Not only the test files stay in page cache, but the whole VFAT 
> superblock seems to disappear from the device as well until sync.

So I guess that we should add a message to the test pointing out to the
upstream bug if the size ends up being zero instead.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] kernel/device-drivers/zram/zram01.sh : add a sync

[Ian Wienand]

On Thu, Aug 03, 2023 at 12:59:25PM +0200, Martin Doucha wrote:
> > I guess that the files written by the dd above end up in page cache and
> > are not written out until much later. Does it fix the problem if you add
> > fdatasync to the dd commandline? That should be faster than using the
> > big hammer and sysncing the whole system.
> 
> This is actually a kernel bug reported upstream:
> https://lore.kernel.org/linux-block/20221107191136.18048-1-pvorel@suse.cz/
> 
> Not only the test files stay in page cache, but the whole VFAT superblock
> seems to disappear from the device as well until sync.

Interesting, thanks for the link!  Reading through that, it doesn't
seem resolved, and I guess new information is that I saw this on an
arm64 host rather than a ppc64le.

It doesn't seem like there was a firm conclusion reached in that
thread -- the machine I could repeat this on has been removed, but I
will try to recreate it with this new information in mind ...

-i


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

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

[Ian Wienand]

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.

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 in the occasional 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 mix the write values to make something
compressible but avoid the same-page deduplication.  However, to cover
both paths, as described inline, we write out a few pages worth and
double-check the stats reflect deduplicated pages.

This also adds the sync, as discussed in prior version [3] to increase
the reliability of testing.

[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>
---
 .../kernel/device-drivers/zram/zram01.sh      | 56 ++++++++++++++++---
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 58d233f91..619d47f93 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -4,7 +4,7 @@
 # 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_TESTFUNC="do_test"
@@ -107,14 +107,39 @@ zram_mount()
 
 zram_fill_fs()
 {
-	local mem_used_total
-	local b i r v
+	local mm_stat mem_used_total same_pages
+	local b f i r v
+
+	# zram has "same page" detection that will flag pages filled
+	# with the same value.  These pages are not compressed; there
+	# is just a record left in the allocation table "page full of
+	# <x>".  mem_used_total is the number of pages held by the
+	# allocator to store store compressed data.  If we write a
+	# consistent value in the dd loop below (e.g. just /dev/zero)
+	# the only data to compress is the filesystem metadata.  It is
+	# possible the fs is not quiescent, thus this metadata is in
+	# memory (i.e. not compressed).  Consequently we could have no
+	# compressed data stored and the ratio (data/pages held for
+	# compressed data) would be a divide by zero.
+	#
+	# To make sure we are actually testing both the same-page and
+	# compression paths, we first pad with zeros but then fill
+	# with a compressible series of alternatiting 0x00 and 0xFF.
+	# This should assure we stress the compression path and can
+	# calculate the compression level reliabily.
+	dd if=/dev/zero count=1 bs=512 count=1 2>err.txt | tr "\000" "\377" > compressible
+	dd if=/dev/zero count=1 bs=512 count=1 >> compressible 2>err.txt
 
 	for i in $(seq $dev_start $dev_end); do
-		tst_res TINFO "filling zram$i (it can take long time)"
+		f='/dev/zero'
+		tst_res TINFO "zero filling zram$i"
 		b=0
 		while true; do
-			dd conv=notrunc if=/dev/zero of=zram${i}/file \
+			if [ $b == 1024 ]; then
+				f='compressible'
+				tst_res TINFO "filling zram$i with compressible data (this may take a while)"
+			fi
+			dd conv=notrunc if=$f of=zram${i}/file \
 				oflag=append count=1 bs=1024 status=none \
 				>/dev/null 2>err.txt || break
 			b=$(($b + 1))
@@ -133,7 +158,18 @@ zram_fill_fs()
 			continue
 		fi
 
-		mem_used_total=`awk '{print $3}' "/sys/block/zram$i/mm_stat"`
+		# The compression needs time to catch up so we get
+		# useful stats
+		sync
+
+		mm_stat=$(cat "/sys/block/zram$i/mm_stat")
+		tst_res TINFO "stats for zram$i : $mm_stat"
+
+		mem_used_total=`echo $mm_stat | awk '{print $3}'`
+		if [ $mem_used_total -eq 0 ]; then
+			test_res FAIL "/sys/block/zram$i/mm_stat reports 0 size : $(cat /sys/block/zram$i/mm_stat)"
+			return
+		fi
 		v=$((100 * 1024 * $b / $mem_used_total))
 		r=`echo "scale=2; $v / 100 " | bc`
 
@@ -141,9 +177,15 @@ zram_fill_fs()
 			tst_res TFAIL "compression ratio: $r:1"
 			break
 		fi
-
 		tst_res TPASS "compression ratio: $r:1"
+
+		same_pages=`echo $mm_stat | awk '{print $6}'`
+		if [ "$same_pages" -lt 10 ]; then
+		    tst_res TFAIL "insufficient same_pages: $same_pages < 10"
+		fi
+		tst_res TPASS "same_pages: $same_pages"
 	done
+	rm compressible
 }
 
 do_test()
-- 
2.41.0


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

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

[Richard Palethorpe]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

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

In any case I'd prefer to see a zram04 written in C if some coverage is
missing.

-- 
Thank you,
Richard.

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

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

[Ian Wienand]

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.

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.

> 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


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

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

[Richard Palethorpe]

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

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

[Martin Doucha]

On 07. 09. 23 8:46, Ian Wienand wrote:
> 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.

Please do not completely rewrite test scenarios to hide one failure due 
to filesystem specifics. If this is not a kernel bug, the correct way to 
deal with this is to disable testing on vfat in initialize_vars():

for fs in $(tst_supported_fs -s tmpfs,vfat); do
...

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

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

[Ian Wienand]

On Thu, Sep 07, 2023 at 12:18:38PM +0200, Martin Doucha wrote:
> On 07. 09. 23 8:46, Ian Wienand wrote:
> > 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.
> 
> Please do not completely rewrite test scenarios to hide one failure due to
> filesystem specifics. If this is not a kernel bug, the correct way to deal
> with this is to disable testing on vfat in initialize_vars():
> 
> for fs in $(tst_supported_fs -s tmpfs,vfat); do

I don't think this is the correct way to deal with it.  I think that
you're probably referring to earlier mail where there was a suggestion
that this was a ppc64/vfat specific issue [1].  I was seeing this in a
different context, and I believe the zeros are explained by no data
actually being in the compressed buffers, as explained at [2].  Hence
I think we need to come to some conclusion on actually writing data
during testing.

-i

[1] https://lore.kernel.org/lkml/3ac740c0-954b-5e68-b413-0adc7bc5a2b5@suse.cz/#t
[2] https://lore.kernel.org/lkml/ZNB2kORYiKdl3vSq@fedora19.localdomain/


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

Re: [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


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

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

[Martin Doucha]

On 08. 09. 23 0:29, Ian Wienand wrote:
> I don't think this is the correct way to deal with it.  I think that
> you're probably referring to earlier mail where there was a suggestion
> that this was a ppc64/vfat specific issue [1].  I was seeing this in a
> different context, and I believe the zeros are explained by no data
> actually being in the compressed buffers, as explained at [2].  Hence
> I think we need to come to some conclusion on actually writing data
> during testing.

Well then, did you see the failure on any other filesystem than vfat? 
I've read this whole e-mail thread again but there is no mention of 
which filesystems trigger this issue.

Alternatively, if kernel developers say that the caching behavior in 
zram is correct, then your v1 patch (adding sync) is the right solution.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

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

[Ian Wienand]

On Fri, Sep 08, 2023 at 11:21:47AM +0200, Martin Doucha wrote:
> On 08. 09. 23 0:29, Ian Wienand wrote:
> > I don't think this is the correct way to deal with it.  I think that
> > you're probably referring to earlier mail where there was a suggestion
> > that this was a ppc64/vfat specific issue [1].  I was seeing this in a
> > different context, and I believe the zeros are explained by no data
> > actually being in the compressed buffers, as explained at [2].  Hence
> > I think we need to come to some conclusion on actually writing data
> > during testing.
> 
> Well then, did you see the failure on any other filesystem than vfat? I've
> read this whole e-mail thread again but there is no mention of which
> filesystems trigger this issue.
> 

I see this on vfat; the test stops after the failure.  But I think
focusing on the file-system is a red-herring in this case.  As
explained at [1] I think the problem is more that there's insufficient
data written due to the de-deuplication.  This probably exhibits most
easily on vfat if it's not doing something like writing superblocks in
other areas of the disk, etc.

> Alternatively, if kernel developers say that the caching behavior in zram is
> correct, then your v1 patch (adding sync) is the right solution.

I think this explains why it is the test, not the kernel behaviour,
that is causing the failure.

-i

[1] https://lore.kernel.org/linux-block/ZNB2kORYiKdl3vSq@fedora19.localdomain/#t


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

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

[Richard Palethorpe]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

> On Fri, Sep 08, 2023 at 11:21:47AM +0200, Martin Doucha wrote:
>> On 08. 09. 23 0:29, Ian Wienand wrote:
>> > I don't think this is the correct way to deal with it.  I think that
>> > you're probably referring to earlier mail where there was a suggestion
>> > that this was a ppc64/vfat specific issue [1].  I was seeing this in a
>> > different context, and I believe the zeros are explained by no data
>> > actually being in the compressed buffers, as explained at [2].  Hence
>> > I think we need to come to some conclusion on actually writing data
>> > during testing.
>> 
>> Well then, did you see the failure on any other filesystem than vfat? I've
>> read this whole e-mail thread again but there is no mention of which
>> filesystems trigger this issue.
>> 
>
> I see this on vfat; the test stops after the failure.  But I think
> focusing on the file-system is a red-herring in this case.  As
> explained at [1] I think the problem is more that there's insufficient
> data written due to the de-deuplication.  This probably exhibits most
> easily on vfat if it's not doing something like writing superblocks in
> other areas of the disk, etc.
>
>> Alternatively, if kernel developers say that the caching behavior in zram is
>> correct, then your v1 patch (adding sync) is the right solution.
>
> I think this explains why it is the test, not the kernel behaviour,
> that is causing the failure.
>
> -i
>
> [1] https://lore.kernel.org/linux-block/ZNB2kORYiKdl3vSq@fedora19.localdomain/#t

I would suggest just using sync, but Petr originally suggested using a
wait loop. Then reported that the bug was still reproducible with the
loop:

https://lore.kernel.org/linux-block/Y3s+Czg8sBsGYO+1@pevik/

Then said it wasn't reproducible. The problem is that if using a loop
doesn't solve it then possibly the VFAT meta-data doesn't get written to
disk in the absence of any pressure.

So instead I'd suggest resurrecting Petr's original patch or at least
his approach. If we merge that and still see failures then maybe it's
worth investigating more with tracing/debugging.

-- 
Thank you,
Richard.

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

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

[Ian Wienand]

On Wed, Sep 13, 2023 at 03:35:18PM +0100, Richard Palethorpe wrote:
> I would suggest just using sync, but Petr originally suggested using a
> wait loop. Then reported that the bug was still reproducible with the
> loop:
> 
> https://lore.kernel.org/linux-block/Y3s+Czg8sBsGYO+1@pevik/
> 
> Then said it wasn't reproducible. The problem is that if using a loop
> doesn't solve it then possibly the VFAT meta-data doesn't get written to
> disk in the absence of any pressure.
> 
> So instead I'd suggest resurrecting Petr's original patch or at least
> his approach. If we merge that and still see failures then maybe it's
> worth investigating more with tracing/debugging.

I do not think the original patch [1] is the correct solution in light
of the further investigation that has happened after it was proposed.

[2] is about the clearest explaination I can come up with, other than
the patch description and comments added in the v2 patch [3].  I am of
the opinion that to be useful these tests need to explicitly make sure
they are not just writing data that can be de-duplicated.  I do not
believe the the intent of these tests was to have the only data
managed by the disk a very small amount of file-system metadata.

Sorry to sound like a broken record, but I spent some time
investigating the paths taken with [2] and confirming the stats that
were coming out were not due to some kernel issue, but it really was
that the backing area had no pages allocated to it at all.

Looping on a sync might make the test pass in more cases, but I hope
we can agree the idea is to make the test better, not just pass so we
can continue to ignore it.

-i

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


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

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

[Richard Palethorpe]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

> On Wed, Sep 13, 2023 at 03:35:18PM +0100, Richard Palethorpe wrote:
>> I would suggest just using sync, but Petr originally suggested using a
>> wait loop. Then reported that the bug was still reproducible with the
>> loop:
>> 
>> https://lore.kernel.org/linux-block/Y3s+Czg8sBsGYO+1@pevik/
>> 
>> Then said it wasn't reproducible. The problem is that if using a loop
>> doesn't solve it then possibly the VFAT meta-data doesn't get written to
>> disk in the absence of any pressure.
>> 
>> So instead I'd suggest resurrecting Petr's original patch or at least
>> his approach. If we merge that and still see failures then maybe it's
>> worth investigating more with tracing/debugging.
>
> I do not think the original patch [1] is the correct solution in light
> of the further investigation that has happened after it was proposed.
>
> [2] is about the clearest explaination I can come up with, other than
> the patch description and comments added in the v2 patch [3].  I am of
> the opinion that to be useful these tests need to explicitly make sure
> they are not just writing data that can be de-duplicated.  I do not
> believe the the intent of these tests was to have the only data
> managed by the disk a very small amount of file-system metadata.
>
> Sorry to sound like a broken record, but I spent some time
> investigating the paths taken with [2] and confirming the stats that
> were coming out were not due to some kernel issue, but it really was
> that the backing area had no pages allocated to it at all.
>
> Looping on a sync might make the test pass in more cases, but I hope
> we can agree the idea is to make the test better, not just pass so we
> can continue to ignore it.

We don't want to remove coverage of ZRAM_SAME! A bug in ZRAM_SAME is a
potential expoit or data-corruption.

If you want to change the test you have to show where ZRAM_SAME is being
covered instead.

It's not that I think ZRAM_SAME is any more or less important than the
true compression path. It's that if we never allow coverage to be
swapped out or removed, then we systematically increase coverage.

>
> -i
>
> [1] https://lore.kernel.org/linux-block/20221107191136.18048-2-pvorel@suse.cz/
> [2] https://lore.kernel.org/linux-block/ZNB2kORYiKdl3vSq@fedora19.localdomain/
> [3] https://lore.kernel.org/ltp/ZPpOuK9lyWr2wZWI@fedora19.localdomain/T/#m1e037003252012ac115e57285a926db77380897f


-- 
Thank you,
Richard.

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

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

[Ian Wienand]

On Thu, Sep 14, 2023 at 08:37:46AM +0100, Richard Palethorpe wrote:
> We don't want to remove coverage of ZRAM_SAME! A bug in ZRAM_SAME is a
> potential expoit or data-corruption.
>
> If you want to change the test you have to show where ZRAM_SAME is being
> covered instead.

The patch v2 has always had the comment and intent

+	# To make sure we are actually testing both the same-page and
+	# compression paths, we first pad with zeros but then fill
+	# with a compressible series of alternatiting 0x00 and 0xFF.
+	# This should assure we stress the compression path and can
+	# calculate the compression level reliabily.

I believe this tests both paths, and in a more rigorous manner than the
extant test.

-i


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

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

[Richard Palethorpe]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

> On Thu, Sep 14, 2023 at 08:37:46AM +0100, Richard Palethorpe wrote:
>> We don't want to remove coverage of ZRAM_SAME! A bug in ZRAM_SAME is a
>> potential expoit or data-corruption.
>>
>> If you want to change the test you have to show where ZRAM_SAME is being
>> covered instead.
>
> The patch v2 has always had the comment and intent
>
> +	# To make sure we are actually testing both the same-page and
> +	# compression paths, we first pad with zeros but then fill
> +	# with a compressible series of alternatiting 0x00 and 0xFF.
> +	# This should assure we stress the compression path and can
> +	# calculate the compression level reliabily.
>
> I believe this tests both paths, and in a more rigorous manner than the
> extant test.
>
> -i

I did miss that, however it's actually more rigorous (read "complete")
to test these things seperately. Or even better to test them seperately
then together. Because if only writing out same-page's and a single page
with some meta-data in results in a bug, then your method would not find
that.

You're still swapping one type of coverage for another.

So I'm still in favor of accepting Petr's original patch and of course I
would welcome what you are proposing as additional coverage.

-- 
Thank you,
Richard.

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

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

[Ian Wienand]

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.

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"
+	fi
+	tst_res TPASS "same_pages: $same_pages"
 
-	return 0
+    done
 }
 
-zram_fill_fs()
+# Fill a file with compressible data.  The same-page deduplication
+# works on matching a single (unsigned long) value in a page, so by
+# splitting this up into (smaller than page size) 1KiB of alternating
+# 0x00 and 0xFF we avoid this, but make something that should be
+# highly compressible.  We check the stats after filling the file to
+# ensure that we see a high compression ratio.
+#
+# TODO: fio has an option to create binary data with a given level of
+#   compressibility.  For now we use a static value to avoid growing
+#   dependencies, but might be useful in the future to validate the
+#   compression behaviour more thoroughly.
+zram_fill_fs_compress()
 {
-	local mem_used_total
-	local b i r v
+	local mm_stat mem_used_total same_pages
+	local b f i r v
+
+	# convert all the 0x00's to 0xFF's.
+	dd if=/dev/zero count=1 bs=512 count=1 2>err.txt | tr "\000" "\377" > compressible
+	dd if=/dev/zero count=1 bs=512 count=1 >> compressible 2>err.txt
 
 	for i in $(seq $dev_start $dev_end); do
-		tst_res TINFO "filling zram$i (it can take long time)"
+		f='/dev/zero'
+		tst_res TINFO "filling zram$i with compressible data"
 		b=0
 		while true; do
-			dd conv=notrunc if=/dev/zero of=zram${i}/file \
+			dd conv=notrunc if=compressible of=zram${i}/file \
 				oflag=append count=1 bs=1024 status=none \
 				>/dev/null 2>err.txt || break
 			b=$(($b + 1))
@@ -145,18 +186,28 @@ zram_fill_fs()
 		fi
 		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
+		# The compression needs time to catch up so we get
+		# useful stats
+		sync
+
+		mm_stat=$(cat "/sys/block/zram$i/mm_stat")
+		tst_res TINFO "stats for zram$i : $mm_stat"
+
+		# This should not happen.  mem_used_total is the
+		# number of pages in the zsmalloc pool asssigned to
+		# this device.  If pages are active, they won't be
+		# stored compressed in this pool; so this might be
+		# zero if, for example, you have a single file of the
+		# same value that is mostly de-deuplicated, and you
+		# have the FS metadata active.  This testing path is
+		# deliberately making data that should be compressed
+		# and stored in the zsmalloc pool, however, so we do
+		# not expect to hit this case.
+		mem_used_total=`echo $mm_stat | awk '{print $3}'`
+		if [ $mem_used_total -eq 0 ]; then
+			test_res FAIL "/sys/block/zram$i/mm_stat reports 0 size : $(cat /sys/block/zram$i/mm_stat)"
+			return
 		fi
-
-		TST_RETRY_FUNC "check_read_mem_used_total /sys/block/zram$i/mm_stat" 0
-		mem_used_total=$(read_mem_used_total /sys/block/zram$i/mm_stat)
-		tst_res TINFO "mem_used_total: $mem_used_total"
-
 		v=$((100 * 1024 * $b / $mem_used_total))
 		r=$(echo "scale=2; $v / 100 " | bc)
 
@@ -164,9 +215,9 @@ zram_fill_fs()
 			tst_res TFAIL "compression ratio: $r:1"
 			break
 		fi
-
 		tst_res TPASS "compression ratio: $r:1"
 	done
+	rm compressible
 }
 
 do_test()
@@ -179,6 +230,7 @@ do_test()
 	 5) zram_makefs;;
 	 6) zram_mount;;
 	 7) zram_fill_fs;;
+	 8) zram_fill_fs_compress;;
 	esac
 }
 
-- 
2.41.0


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

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

[Ian Wienand]

On Mon, Sep 18, 2023 at 09:24:47AM +0100, Richard Palethorpe wrote:
> I did miss that, however it's actually more rigorous (read "complete")
> to test these things seperately. Or even better to test them seperately
> then together. Because if only writing out same-page's and a single page
> with some meta-data in results in a bug, then your method would not find
> that.

I've just posted v3 [1] that separates out the two testing paths which
I hope addresses these concerns.

Thanks

-i

[1] https://lore.kernel.org/ltp/ZQuYiFMOSq1tMTBs@fedora19.localdomain/T/#u


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

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

[Richard Palethorpe]

Hello,

Ian Wienand <iwienand@redhat.com> writes:

> On Mon, Sep 18, 2023 at 09:24:47AM +0100, Richard Palethorpe wrote:
>> I did miss that, however it's actually more rigorous (read "complete")
>> to test these things seperately. Or even better to test them seperately
>> then together. Because if only writing out same-page's and a single page
>> with some meta-data in results in a bug, then your method would not find
>> that.
>
> I've just posted v3 [1] that separates out the two testing paths which
> I hope addresses these concerns.

Great! please note that in the mean time I merged Petr's patch. So you
may need to rebase. I haven't looked at it yet due to the upcoming
release.

>
> Thanks
>
> -i
>
> [1] https://lore.kernel.org/ltp/ZQuYiFMOSq1tMTBs@fedora19.localdomain/T/#u


-- 
Thank you,
Richard.

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

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

[Richard Palethorpe]

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