[LTP] [PATCH v2 0/6] zram cleanup

[LTP] [PATCH v2 0/6] zram cleanup

[Petr Vorel]

Hi,

changes v1->v2:
* rewritten variable setup in commit
  zram01.sh: Check properly mkfs.* dependencies
  to reflect Cyril's suggestions
* 2 new commits related to timeout:
  tst_test.sh: Run _tst_setup_timer after $TST_SETUP
  zram: Increase timeout according to used devices

Kind regards,
Petr

Petr Vorel (6):
  zram: Calculate dev_num variable
  zram01.sh: Generate test setup variables in setup
  zram: Move zram_compress_alg() to zram02.sh
  zram: Move test specific functions out of zram_lib.sh
  tst_test.sh: Run _tst_setup_timer after $TST_SETUP
  zram: Increase timeout according to used devices

 doc/test-writing-guidelines.txt               |   5 +-
 .../kernel/device-drivers/zram/zram01.sh      | 113 +++++++++++++----
 .../kernel/device-drivers/zram/zram02.sh      |  77 ++++++++++--
 .../kernel/device-drivers/zram/zram_lib.sh    | 116 +++---------------
 testcases/lib/tst_test.sh                     |   4 +-
 5 files changed, 180 insertions(+), 135 deletions(-)

-- 
2.30.0

[LTP] [PATCH v2 1/6] zram: Calculate dev_num variable

[Petr Vorel]

Instead of requiring to be set.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
changes v1->v2:
Added paranoid check:
+	if [ $dev_num -le 0 ]; then
+		tst_brk TBROK "dev_num must be > 0"
+	fi

Kind regards,
Petr

 testcases/kernel/device-drivers/zram/zram01.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram02.sh   |  8 +++-----
 testcases/kernel/device-drivers/zram/zram_lib.sh | 16 ++++++++++++++--
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 8de2c0cad..a795ff89f 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.
-# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2019-2021 Petr Vorel <pvorel@suse.cz>
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 #
 # Test creates several zram devices with different filesystems on them.
@@ -11,10 +11,8 @@ TST_TESTFUNC="do_test"
 TST_NEEDS_CMDS="awk bc dd"
 . zram_lib.sh
 
-# Test will create the following number of zram devices:
-dev_num=4
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
 zram_max_streams="2 3 5 8"
 
 FS_SIZE="402653184"
diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index f97cf646c..b4d706568 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 # Copyright (c) 2015 Oracle and/or its affiliates. All Rights Reserved.
-# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+# Copyright (c) 2019-2021 Petr Vorel <pvorel@suse.cz>
 # Author: Alexey Kodanev <alexey.kodanev@oracle.com>
 #
 # Test checks that we can create swap zram device.
@@ -9,10 +9,8 @@ TST_CNT=5
 TST_TESTFUNC="do_test"
 . zram_lib.sh
 
-# Test will create the following number of zram devices:
-dev_num=1
-# This is a list of parameters for zram devices.
-# Number of items must be equal to 'dev_num' parameter.
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
 zram_max_streams="2"
 
 # The zram sysfs node 'disksize' value can be either in bytes,
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 6fa6552ca..a7e8b9f5b 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -34,7 +34,19 @@ zram_cleanup()
 
 zram_load()
 {
+	local tmp
+
+	dev_num=0
+	for tmp in $zram_max_streams; do
+		dev_num=$((dev_num+1))
+	done
+
+	if [ $dev_num -le 0 ]; then
+		tst_brk TBROK "dev_num must be > 0"
+	fi
+
 	tst_res TINFO "create '$dev_num' zram device(s)"
+
 	modprobe zram num_devices=$dev_num || \
 		tst_brk TBROK "failed to insert zram module"
 
@@ -42,9 +54,9 @@ zram_load()
 
 	if [ "$dev_num_created" -ne "$dev_num" ]; then
 		tst_brk TFAIL "unexpected num of devices: $dev_num_created"
-	else
-		tst_res TPASS "test succeeded"
 	fi
+
+	tst_res TPASS "all zram devices successfully created"
 }
 
 zram_max_streams()
-- 
2.30.0

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Generate variables in setup, based on output of tst_supported_fs.
This is more clean approach and it fixes various things:

* Error when there is no filesystem support and also mkfs.ext2
  (fallback) not installed:
/opt/ltp/testcases/bin/zram01.sh: line 198: mkfs.ext2: not found
Instead quit if there is no fs support. But this can lead to skipping
zram_compress_alg(), it will be solved next commit by moving
zram_compress_alg() to zram02.sh.

* Having ext2 as fallback could lead to run it more than once.
There is no much point to do that.

* Drop tst_require_cmds mkfs check, because mkfs is not actually needed.

Improvements:

* Test all suitable filesystems (will need increase timeout).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
changes v1->v2:
Completely rewritten.

 .../kernel/device-drivers/zram/zram01.sh      | 62 ++++++++++++++-----
 .../kernel/device-drivers/zram/zram_lib.sh    | 18 +++---
 2 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index a795ff89f..c5d4a3e51 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -8,23 +8,25 @@
 
 TST_CNT=7
 TST_TESTFUNC="do_test"
-TST_NEEDS_CMDS="awk bc dd"
+TST_NEEDS_CMDS="awk bc dd grep"
 . zram_lib.sh
+TST_SETUP="setup"
 
-# List of parameters for zram devices.
-# For each number the test creates own zram device.
-zram_max_streams="2 3 5 8"
-
-FS_SIZE="402653184"
-FS_TYPE="btrfs"
+get_btrfs_size()
+{
+	local ram_size
 
-RAM_SIZE=$(awk '/MemTotal:/ {print $2}' /proc/meminfo)
-if [ "$RAM_SIZE" -lt 1048576 ]; then
-	tst_res TINFO "not enough space for Btrfs"
-	FS_SIZE="26214400"
-	FS_TYPE="ext2"
-fi
+	ram_size=$(awk '/MemTotal:/ {print $2}' /proc/meminfo)
+	if [ "$ram_size" -lt 1048576 ]; then
+		tst_res TINFO "not enough space for Btrfs"
+		return 1
+	fi
+	return 0
+}
 
+# List of parameters for zram devices.
+# For each number the test creates own zram device.
+# NOTE about size:
 # The zram sysfs node 'disksize' value can be either in bytes,
 # or you can use mem suffixes. But in some old kernels, mem
 # suffixes are not supported, for example, in RHEL6.6GA's kernel
@@ -32,9 +34,37 @@ fi
 # not support mem suffixes, in some newer kernels, they use
 # memparse() which supports mem suffixes. So here we just use
 # bytes to make sure everything works correctly.
-zram_sizes="26214400 26214400 26214400 $FS_SIZE"
-zram_mem_limits="25M 25M 25M $((FS_SIZE/1024/1024))M"
-zram_filesystems="ext3 ext4 xfs $FS_TYPE"
+generate_vars()
+{
+	local fs limit size stream=-1
+	dev_num=0
+
+	for fs in $(tst_supported_fs | grep -v -e fat -e ntfs -e fuse); do
+		size="26214400"
+		limit="25M"
+		if [ "$fs" = "btrfs" ]; then
+			get_btrfs_size || continue
+			size="402653184"
+			limit="$((size/1024/1024))M"
+		fi
+
+		stream=$((stream+3))
+		dev_num=$((dev_num+1))
+		zram_filesystems="$zram_filesystems $fs"
+		zram_mem_limits="$zram_mem_limits $limit"
+		zram_sizes="$zram_sizes $size"
+		zram_max_streams="$zram_max_streams $stream"
+	done
+
+	[ $dev_num -eq 0 ] && \
+		tst_brk TCONF "no suitable filesystem"
+}
+
+setup()
+{
+	generate_vars
+	zram_load
+}
 
 zram_fill_fs()
 {
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index a7e8b9f5b..d4aaf0c3e 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -36,10 +36,12 @@ zram_load()
 {
 	local tmp
 
-	dev_num=0
-	for tmp in $zram_max_streams; do
-		dev_num=$((dev_num+1))
-	done
+	if [ -z "$dev_num" ]; then
+		dev_num=0
+		for tmp in $zram_max_streams; do
+			dev_num=$((dev_num+1))
+		done
+	fi
 
 	if [ $dev_num -le 0 ]; then
 		tst_brk TBROK "dev_num must be > 0"
@@ -129,6 +131,7 @@ zram_set_disksizes()
 
 		i=$(($i + 1))
 		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "test succeeded"
@@ -155,6 +158,7 @@ zram_set_memlimit()
 
 		i=$(($i + 1))
 		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "test succeeded"
@@ -191,13 +195,10 @@ zram_swapoff()
 
 zram_makefs()
 {
-	tst_require_cmds mkfs
 	local i=0
+	local fs
 
 	for fs in $zram_filesystems; do
-		# if requested fs not supported default it to ext2
-		tst_supported_fs $fs 2> /dev/null || fs=ext2
-
 		tst_res TINFO "make $fs filesystem on /dev/zram$i"
 		mkfs.$fs /dev/zram$i > err.log 2>&1
 		if [ $? -ne 0 ]; then
@@ -206,6 +207,7 @@ zram_makefs()
 		fi
 
 		i=$(($i + 1))
+		[ $i -eq $dev_num ] && break
 	done
 
 	tst_res TPASS "zram_makefs succeeded"
-- 
2.30.0

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Hi,

...
> -# List of parameters for zram devices.
> -# For each number the test creates own zram device.
> -zram_max_streams="2 3 5 8"
Not sure about the meaning of zram_max_streams.
It looks like sequence of i+3, starting from 0.
Maybe instead of having variable behaving like array,
zram_max_streams() loop could be based on $dev_num.
But that's just cosmetic.
...
> +generate_vars()
> +{
> +	local fs limit size stream=-1
> +	dev_num=0
> +
> +	for fs in $(tst_supported_fs | grep -v -e fat -e ntfs -e fuse); do
> +		size="26214400"
> +		limit="25M"
> +		if [ "$fs" = "btrfs" ]; then
> +			get_btrfs_size || continue
> +			size="402653184"
> +			limit="$((size/1024/1024))M"
> +		fi
> +
> +		stream=$((stream+3))
> +		dev_num=$((dev_num+1))
> +		zram_filesystems="$zram_filesystems $fs"
> +		zram_mem_limits="$zram_mem_limits $limit"
> +		zram_sizes="$zram_sizes $size"
> +		zram_max_streams="$zram_max_streams $stream"

...
> +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
> @@ -36,10 +36,12 @@ zram_load()
>  {
>  	local tmp

> -	dev_num=0
> -	for tmp in $zram_max_streams; do
> -		dev_num=$((dev_num+1))
> -	done
> +	if [ -z "$dev_num" ]; then
> +		dev_num=0
> +		for tmp in $zram_max_streams; do
> +			dev_num=$((dev_num+1))
> +		done
> +	fi

>  	if [ $dev_num -le 0 ]; then
>  		tst_brk TBROK "dev_num must be > 0"
> @@ -129,6 +131,7 @@ zram_set_disksizes()

>  		i=$(($i + 1))
>  		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
> +		[ $i -eq $dev_num ] && break
This check is not needed any more.
>  	done

>  	tst_res TPASS "test succeeded"
> @@ -155,6 +158,7 @@ zram_set_memlimit()

>  		i=$(($i + 1))
>  		tst_res TINFO "$sys_path = '$ds' ($i/$dev_num)"
> +		[ $i -eq $dev_num ] && break
And here.
>  	done

>  	tst_res TPASS "test succeeded"
> @@ -191,13 +195,10 @@ zram_swapoff()

>  zram_makefs()
>  {
> -	tst_require_cmds mkfs
>  	local i=0
> +	local fs

>  	for fs in $zram_filesystems; do
> -		# if requested fs not supported default it to ext2
> -		tst_supported_fs $fs 2> /dev/null || fs=ext2
> -
>  		tst_res TINFO "make $fs filesystem on /dev/zram$i"
>  		mkfs.$fs /dev/zram$i > err.log 2>&1
>  		if [ $? -ne 0 ]; then
> @@ -206,6 +207,7 @@ zram_makefs()
>  		fi

>  		i=$(($i + 1))
> +		[ $i -eq $dev_num ] && break
And here
>  	done

Kind regards,
Petr

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Li Wang]

Hi Petr,

Petr Vorel <pvorel@suse.cz> wrote:

> ...
>
> diff --git a/testcases/kernel/device-drivers/zram/zram01.sh
> b/testcases/kernel/device-drivers/zram/zram01.sh
> index a795ff89f..c5d4a3e51 100755
> --- a/testcases/kernel/device-drivers/zram/zram01.sh
> +++ b/testcases/kernel/device-drivers/zram/zram01.sh
> @@ -8,23 +8,25 @@
>
>  TST_CNT=7
>  TST_TESTFUNC="do_test"
> -TST_NEEDS_CMDS="awk bc dd"
> +TST_NEEDS_CMDS="awk bc dd grep"
>  . zram_lib.sh
> +TST_SETUP="setup"
>
> -# List of parameters for zram devices.
> -# For each number the test creates own zram device.
> -zram_max_streams="2 3 5 8"
> -
> -FS_SIZE="402653184"
> -FS_TYPE="btrfs"
> +get_btrfs_size()
>

What about renaming at_least_1G_mem() or check_space_for_btrfs()?


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210130/cf12bffe/attachment.htm>

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Hi Li,

> Hi Petr,

> Petr Vorel <pvorel@suse.cz> wrote:

> > ...

> > diff --git a/testcases/kernel/device-drivers/zram/zram01.sh
> > b/testcases/kernel/device-drivers/zram/zram01.sh
> > index a795ff89f..c5d4a3e51 100755
> > --- a/testcases/kernel/device-drivers/zram/zram01.sh
> > +++ b/testcases/kernel/device-drivers/zram/zram01.sh
> > @@ -8,23 +8,25 @@

> >  TST_CNT=7
> >  TST_TESTFUNC="do_test"
> > -TST_NEEDS_CMDS="awk bc dd"
> > +TST_NEEDS_CMDS="awk bc dd grep"
> >  . zram_lib.sh
> > +TST_SETUP="setup"

> > -# List of parameters for zram devices.
> > -# For each number the test creates own zram device.
> > -zram_max_streams="2 3 5 8"
> > -
> > -FS_SIZE="402653184"
> > -FS_TYPE="btrfs"
> > +get_btrfs_size()


> What about renaming at_least_1G_mem() or check_space_for_btrfs()?
Good point. I'm slightly for check_space_for_btrfs().

at_least_1G_mem() is also good, but for that I'd also move tst_res TINFO "not
enough space for Btrfs" out of the function and put it into generate_vars(). But
since it's used only for btrfs I slightly prefer check_space_for_btrfs(). But no
strong opinion about it.

Thanks for your review!

Kind regards,
Petr

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Li Wang]

Hi Petr,

Petr Vorel <pvorel@suse.cz> wrote:

...
> > > +get_btrfs_size()
>
>
> > What about renaming at_least_1G_mem() or check_space_for_btrfs()?
> Good point. I'm slightly for check_space_for_btrfs().
>
> at_least_1G_mem() is also good, but for that I'd also move tst_res TINFO
> "not
> enough space for Btrfs" out of the function and put it into
> generate_vars(). But
> since it's used only for btrfs I slightly prefer check_space_for_btrfs().
> But no
> strong opinion about it.
>

Agree, thanks!

Btw I suddenly think that we could have a nicer name initialize_vars()
to replace generate_vars(), because we just use it once to initiate the
test variables in the setup phase.

Anyway, it's only my feelings and also depends on your preference too.


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210202/64014556/attachment.htm>

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Hi Li,

...
> > > > +get_btrfs_size()


> > > What about renaming at_least_1G_mem() or check_space_for_btrfs()?
> > Good point. I'm slightly for check_space_for_btrfs().

> > at_least_1G_mem() is also good, but for that I'd also move tst_res TINFO
> > "not
> > enough space for Btrfs" out of the function and put it into
> > generate_vars(). But
> > since it's used only for btrfs I slightly prefer check_space_for_btrfs().
> > But no
> > strong opinion about it.


> Agree, thanks!

> Btw I suddenly think that we could have a nicer name initialize_vars()
> to replace generate_vars(), because we just use it once to initiate the
> test variables in the setup phase.
Good idea, thanks!

BTW I tested zram on all filesystems including fuse/*fat/ntfs:
zram01 4 TINFO: make ext2 filesystem on /dev/zram0
zram01 4 TINFO: make ext3 filesystem on /dev/zram1
zram01 4 TINFO: make ext4 filesystem on /dev/zram2
zram01 4 TINFO: make xfs filesystem on /dev/zram3
zram01 4 TINFO: make btrfs filesystem on /dev/zram4
zram01 4 TINFO: make vfat filesystem on /dev/zram5
zram01 4 TINFO: make exfat filesystem on /dev/zram6
zram01 4 TINFO: make ntfs filesystem on /dev/zram7

and it's working well, thus I suggest to test everything available:

-       for fs in $(tst_supported_fs | grep -v -e fat -e ntfs -e fuse); do
+       for fs in $(tst_supported_fs); do

Before sending v3 or just merge with fixes I'm also waiting for Cyril's (or
somebody else) review / suggestions on the timeout (whether simply use -1 for
timeout and drop patch "tst_test.sh: Run _tst_setup_timer after $TST_SETUP")

https://patchwork.ozlabs.org/project/ltp/patch/20210129194144.31299-6-pvorel@suse.cz/
https://patchwork.ozlabs.org/project/ltp/patch/20210129194144.31299-7-pvorel@suse.cz/

Kind regards,
Petr

> Anyway, it's only my feelings and also depends on your preference too.

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Hi Li,

> BTW I tested zram on all filesystems including fuse/*fat/ntfs:
> zram01 4 TINFO: make ext2 filesystem on /dev/zram0
> zram01 4 TINFO: make ext3 filesystem on /dev/zram1
> zram01 4 TINFO: make ext4 filesystem on /dev/zram2
> zram01 4 TINFO: make xfs filesystem on /dev/zram3
> zram01 4 TINFO: make btrfs filesystem on /dev/zram4
> zram01 4 TINFO: make vfat filesystem on /dev/zram5
> zram01 4 TINFO: make exfat filesystem on /dev/zram6
> zram01 4 TINFO: make ntfs filesystem on /dev/zram7

> and it's working well, thus I suggest to test everything available:

> -       for fs in $(tst_supported_fs | grep -v -e fat -e ntfs -e fuse); do
> +       for fs in $(tst_supported_fs); do

Although running all 8 filesystems run takes more than 10 min. If it's too long,
it might be better to keep grep and also limit number of tested filesystems.

Kind regards,
Petr

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Li Wang]

Hi Petr,

Sorry for the late reply, I was on the Spring Festival holidays last two
weeks.

On Mon, Feb 8, 2021 at 3:11 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > BTW I tested zram on all filesystems including fuse/*fat/ntfs:
> > zram01 4 TINFO: make ext2 filesystem on /dev/zram0
> > zram01 4 TINFO: make ext3 filesystem on /dev/zram1
> > zram01 4 TINFO: make ext4 filesystem on /dev/zram2
> > zram01 4 TINFO: make xfs filesystem on /dev/zram3
> > zram01 4 TINFO: make btrfs filesystem on /dev/zram4
> > zram01 4 TINFO: make vfat filesystem on /dev/zram5
> > zram01 4 TINFO: make exfat filesystem on /dev/zram6
> > zram01 4 TINFO: make ntfs filesystem on /dev/zram7
>
> > and it's working well, thus I suggest to test everything available:
>
> > -       for fs in $(tst_supported_fs | grep -v -e fat -e ntfs -e fuse);
> do
> > +       for fs in $(tst_supported_fs); do
>
> Although running all 8 filesystems run takes more than 10 min. If it's too
> long,
> it might be better to keep grep and also limit number of tested
> filesystems.
>

+1
I tend to keep grep there cause Linux users might rarely use fat/ntfs.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210218/79c358ac/attachment.htm>

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Cyril Hrubis]

Hi!
Looks good including the changes proposed in the thread.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/6] zram01.sh: Generate test setup variables in setup

[Petr Vorel]

Hi Cyril,

> Hi!
> Looks good including the changes proposed in the thread.

> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Thanks! I suppose this include keep "grep -v -e fat -e ntfs -e fuse"
to avoid fat/ntfs testing (I'd personally use it, because we don't whitelist
testing them for other tests, e.g. fanotify).

Kind regards,
Petr

[LTP] [PATCH v2 3/6] zram: Move zram_compress_alg() to zram02.sh

[Petr Vorel]

Quit at setup in case there is no fs support (change in previous commit)
can lead to skipping zram_compress_alg(). Move to zram02.sh where is no
such limitation.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
The same as in v1.

 testcases/kernel/device-drivers/zram/zram01.sh | 13 ++++++-------
 testcases/kernel/device-drivers/zram/zram02.sh | 11 ++++++-----
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index c5d4a3e51..5e99f8bb8 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -6,7 +6,7 @@
 # Test creates several zram devices with different filesystems on them.
 # It fills each device with zeros and checks that compression works.
 
-TST_CNT=7
+TST_CNT=6
 TST_TESTFUNC="do_test"
 TST_NEEDS_CMDS="awk bc dd grep"
 . zram_lib.sh
@@ -108,12 +108,11 @@ do_test()
 {
 	case $1 in
 	 1) zram_max_streams;;
-	 2) zram_compress_alg;;
-	 3) zram_set_disksizes;;
-	 4) zram_set_memlimit;;
-	 5) zram_makefs;;
-	 6) zram_mount;;
-	 7) zram_fill_fs;;
+	 2) zram_set_disksizes;;
+	 3) zram_set_memlimit;;
+	 4) zram_makefs;;
+	 5) zram_mount;;
+	 6) zram_fill_fs;;
 	esac
 }
 
diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index b4d706568..d09977ec1 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -5,7 +5,7 @@
 #
 # Test checks that we can create swap zram device.
 
-TST_CNT=5
+TST_CNT=6
 TST_TESTFUNC="do_test"
 . zram_lib.sh
 
@@ -27,10 +27,11 @@ do_test()
 {
 	case $1 in
 	 1) zram_max_streams;;
-	 2) zram_set_disksizes;;
-	 3) zram_set_memlimit;;
-	 4) zram_makeswap;;
-	 5) zram_swapoff;;
+	 2) zram_compress_alg;;
+	 3) zram_set_disksizes;;
+	 4) zram_set_memlimit;;
+	 5) zram_makeswap;;
+	 6) zram_swapoff;;
 	esac
 }
 
-- 
2.30.0

[LTP] [PATCH v2 3/6] zram: Move zram_compress_alg() to zram02.sh

[Cyril Hrubis]

Hi!
> Quit at setup in case there is no fs support (change in previous commit)
> can lead to skipping zram_compress_alg(). Move to zram02.sh where is no
> such limitation.

Actually I think that we should keep it in both for now, since the tests
do depend on each other, the zram_fill_fs actually checks compression
ration, which only makes sense if compression is enabled.

Ideally we should use different compression algorithms in the zram01
test, so I guess that we can, later on distribute the compression
algorithms between the created devices, but that shouldn't stop this
patchset.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/6] zram: Move zram_compress_alg() to zram02.sh

[Petr Vorel]

Hi Cyril,

> Hi!
> > Quit at setup in case there is no fs support (change in previous commit)
> > can lead to skipping zram_compress_alg(). Move to zram02.sh where is no
> > such limitation.

> Actually I think that we should keep it in both for now, since the tests
> do depend on each other, the zram_fill_fs actually checks compression
> ration, which only makes sense if compression is enabled.
Makes sense.

> Ideally we should use different compression algorithms in the zram01
> test, so I guess that we can, later on distribute the compression
> algorithms between the created devices, but that shouldn't stop this
> patchset.
OK. I'll change this patchset to *add* zram_compress_alg() to zram02.sh (keep it
in zram01.sh) and update following commit.

Kind regards,
Petr

[LTP] [PATCH v2 4/6] zram: Move test specific functions out of zram_lib.sh

[Petr Vorel]

Refactoring, as code is confusing enough, when use global variables and
functions which are single test specific + there is zram_fill_fs already
in zram01.sh.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
The same as in v1.

 .../kernel/device-drivers/zram/zram01.sh      | 34 +++++++
 .../kernel/device-drivers/zram/zram02.sh      | 58 ++++++++++++
 .../kernel/device-drivers/zram/zram_lib.sh    | 92 -------------------
 3 files changed, 92 insertions(+), 92 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 5e99f8bb8..695de14a1 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -66,6 +66,40 @@ setup()
 	zram_load
 }
 
+zram_makefs()
+{
+	local i=0
+	local fs
+
+	for fs in $zram_filesystems; do
+		tst_res TINFO "make $fs filesystem on /dev/zram$i"
+		mkfs.$fs /dev/zram$i > err.log 2>&1
+		if [ $? -ne 0 ]; then
+			cat err.log
+			tst_brk TFAIL "failed to make $fs on /dev/zram$i"
+		fi
+
+		i=$(($i + 1))
+		[ $i -eq $dev_num ] && break
+	done
+
+	tst_res TPASS "zram_makefs succeeded"
+}
+
+zram_mount()
+{
+	local i=0
+
+	for i in $(seq 0 $(($dev_num - 1))); do
+		tst_res TINFO "mount /dev/zram$i"
+		mkdir zram$i
+		ROD mount /dev/zram$i zram$i
+		dev_mounted=$i
+	done
+
+	tst_res TPASS "mount of zram device(s) succeeded"
+}
+
 zram_fill_fs()
 {
 	for i in $(seq 0 $(($dev_num - 1))); do
diff --git a/testcases/kernel/device-drivers/zram/zram02.sh b/testcases/kernel/device-drivers/zram/zram02.sh
index d09977ec1..803b8dc29 100755
--- a/testcases/kernel/device-drivers/zram/zram02.sh
+++ b/testcases/kernel/device-drivers/zram/zram02.sh
@@ -23,6 +23,64 @@ zram_max_streams="2"
 zram_sizes="107374182400" # 100GB
 zram_mem_limits="1M"
 
+zram_compress_alg()
+{
+	if tst_kvcmp -lt "3.15"; then
+		tst_res TCONF "device attribute comp_algorithm is"\
+			"introduced since kernel v3.15, the running kernel"\
+			"does not support it"
+		return
+	fi
+
+	local i=0
+
+	tst_res TINFO "test that we can set compression algorithm"
+	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
+	tst_res TINFO "supported algs: $algs"
+
+	local dev_max=$(($dev_num - 1))
+
+	for i in $(seq 0 $dev_max); do
+		for alg in $algs; do
+			local sys_path="/sys/block/zram${i}/comp_algorithm"
+			echo "$alg" >  $sys_path || \
+				tst_brk TFAIL "can't set '$alg' to $sys_path"
+			tst_res TINFO "$sys_path = '$alg' ($i/$dev_max)"
+		done
+	done
+
+	tst_res TPASS "test succeeded"
+}
+
+zram_makeswap()
+{
+	tst_res TINFO "make swap with zram device(s)"
+	tst_require_cmds mkswap swapon swapoff
+	local i=0
+
+	for i in $(seq 0 $(($dev_num - 1))); do
+		ROD mkswap /dev/zram$i
+		ROD swapon /dev/zram$i
+		tst_res TINFO "done with /dev/zram$i"
+		dev_makeswap=$i
+	done
+
+	tst_res TPASS "making zram swap succeeded"
+}
+
+zram_swapoff()
+{
+	tst_require_cmds swapoff
+	local i
+
+	for i in $(seq 0 $dev_makeswap); do
+		ROD swapoff /dev/zram$i
+	done
+	dev_makeswap=-1
+
+	tst_res TPASS "swapoff completed"
+}
+
 do_test()
 {
 	case $1 in
diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index d4aaf0c3e..26ed1846b 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -89,35 +89,6 @@ zram_max_streams()
 	tst_res TPASS "test succeeded"
 }
 
-zram_compress_alg()
-{
-	if tst_kvcmp -lt "3.15"; then
-		tst_res TCONF "device attribute comp_algorithm is"\
-			"introduced since kernel v3.15, the running kernel"\
-			"does not support it"
-		return
-	fi
-
-	local i=0
-
-	tst_res TINFO "test that we can set compression algorithm"
-	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
-	tst_res TINFO "supported algs: $algs"
-
-	local dev_max=$(($dev_num - 1))
-
-	for i in $(seq 0 $dev_max); do
-		for alg in $algs; do
-			local sys_path="/sys/block/zram${i}/comp_algorithm"
-			echo "$alg" >  $sys_path || \
-				tst_brk TFAIL "can't set '$alg' to $sys_path"
-			tst_res TINFO "$sys_path = '$alg' ($i/$dev_max)"
-		done
-	done
-
-	tst_res TPASS "test succeeded"
-}
-
 zram_set_disksizes()
 {
 	local i=0
@@ -163,66 +134,3 @@ zram_set_memlimit()
 
 	tst_res TPASS "test succeeded"
 }
-
-zram_makeswap()
-{
-	tst_res TINFO "make swap with zram device(s)"
-	tst_require_cmds mkswap swapon swapoff
-	local i=0
-
-	for i in $(seq 0 $(($dev_num - 1))); do
-		ROD mkswap /dev/zram$i
-		ROD swapon /dev/zram$i
-		tst_res TINFO "done with /dev/zram$i"
-		dev_makeswap=$i
-	done
-
-	tst_res TPASS "making zram swap succeeded"
-}
-
-zram_swapoff()
-{
-	tst_require_cmds swapoff
-	local i
-
-	for i in $(seq 0 $dev_makeswap); do
-		ROD swapoff /dev/zram$i
-	done
-	dev_makeswap=-1
-
-	tst_res TPASS "swapoff completed"
-}
-
-zram_makefs()
-{
-	local i=0
-	local fs
-
-	for fs in $zram_filesystems; do
-		tst_res TINFO "make $fs filesystem on /dev/zram$i"
-		mkfs.$fs /dev/zram$i > err.log 2>&1
-		if [ $? -ne 0 ]; then
-			cat err.log
-			tst_brk TFAIL "failed to make $fs on /dev/zram$i"
-		fi
-
-		i=$(($i + 1))
-		[ $i -eq $dev_num ] && break
-	done
-
-	tst_res TPASS "zram_makefs succeeded"
-}
-
-zram_mount()
-{
-	local i=0
-
-	for i in $(seq 0 $(($dev_num - 1))); do
-		tst_res TINFO "mount /dev/zram$i"
-		mkdir zram$i
-		ROD mount /dev/zram$i zram$i
-		dev_mounted=$i
-	done
-
-	tst_res TPASS "mount of zram device(s) succeeded"
-}
-- 
2.30.0

[LTP] [PATCH v2 4/6] zram: Move test specific functions out of zram_lib.sh

[Cyril Hrubis]

Hi!
This is okay, but we should keep the zram_compress_alg() in the lib if
we are going to have the test step in both tests.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH v2 5/6] tst_test.sh: Run _tst_setup_timer after $TST_SETUP

[Petr Vorel]

Postpone starting timer a bit after the execution allow to setup
timer in test setup. This is useful if test length depends on input
decided during setup.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New in v2.

Hi,

I'm ok, if you consider this risky.
Next commit can be then changed to have TST_TIMEOUT=-1.

Kind regards,
Petr

 doc/test-writing-guidelines.txt | 5 ++++-
 testcases/lib/tst_test.sh       | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index f3a55cf26..61dccd400 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2398,7 +2398,10 @@ simply by setting right '$TST_NEEDS_FOO'.
 | 'TST_TIMEOUT'      | Maximum timeout set for the test in sec. Must be int >= 1,
                        or -1 (special value to disable timeout), default is 300.
                        Variable is meant be set in tests, not by user.
-                       It's equivalent of `tst_test.timeout` in C.
+                       It's equivalent of `tst_test.timeout` in C, but (unlike
+                       the C API) it can be set also in the setup function.
+                       This is useful if test length depends on input decided
+                       during setup.
 |=============================================================================
 
 NOTE: Network tests (see testcases/network/README.md) use additional variables
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 69f007d89..4fa1674cd 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -552,8 +552,6 @@ tst_run()
 			tst_brk TCONF "test requires kernel $TST_MIN_KVER+"
 	fi
 
-	_tst_setup_timer
-
 	[ "$TST_NEEDS_DEVICE" = 1 ] && TST_NEEDS_TMPDIR=1
 
 	if [ "$TST_NEEDS_TMPDIR" = 1 ]; then
@@ -594,6 +592,8 @@ tst_run()
 		fi
 	fi
 
+	_tst_setup_timer
+
 	#TODO check that test reports some results for each test function call
 	while [ $TST_ITERATIONS -gt 0 ]; do
 		if [ -n "$TST_TEST_DATA" ]; then
-- 
2.30.0

[LTP] [RFC PATCH v2 5/6] tst_test.sh: Run _tst_setup_timer after $TST_SETUP

[Petr Vorel]

Hi,

> I'm ok, if you consider this risky.
If taken, I'll write a test for it.

Kind regards,
Petr

> Next commit can be then changed to have TST_TIMEOUT=-1.

[LTP] [RFC PATCH v2 5/6] tst_test.sh: Run _tst_setup_timer after $TST_SETUP

[Cyril Hrubis]

Hi!
We do have a function to set the timeout in the C library
(tst_set_timeout), maybe we should just add one to the shell just to
match the API.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

to avoid unexpected timeout, which occurred even on just 4 devices.
Default setup is 300, using 200 should be safe.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
New in v2.

 testcases/kernel/device-drivers/zram/zram_lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 26ed1846b..c24b83cfc 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -47,6 +47,8 @@ zram_load()
 		tst_brk TBROK "dev_num must be > 0"
 	fi
 
+	TST_TIMEOUT=$((dev_num*200))
+
 	tst_res TINFO "create '$dev_num' zram device(s)"
 
 	modprobe zram num_devices=$dev_num || \
-- 
2.30.0

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

Hi,

> +	TST_TIMEOUT=$((dev_num*200))
Actually on heavy loaded machine this is not enough due BTRFS.
I can add something like dev_num*600 or even -1 (then previous commit would not
be needed, but IMHO still useful).

Kind regards,
Petr

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

> Hi,

> > +	TST_TIMEOUT=$((dev_num*200))
> Actually on heavy loaded machine this is not enough due BTRFS.
> I can add something like dev_num*600 or even -1 (then previous commit would not
> be needed, but IMHO still useful).
And bad thing is that it breaks other zram tests, because the timer probably
does not allow to run the cleanup:

_tst_setup_timer()
{
...
	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &

I'm not sure if shell allow us to do it better. Maybe sent different signal than
SIGKILL and define 'trap _tst_do_exit' for that signal?

Kind regards,
Petr

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Li Wang]

On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi,
>
> > > +   TST_TIMEOUT=$((dev_num*200))
> > Actually on heavy loaded machine this is not enough due BTRFS.
> > I can add something like dev_num*600 or even -1 (then previous commit
> would not
> > be needed, but IMHO still useful).
>

I personally think -1 is better.



> And bad thing is that it breaks other zram tests, because the timer
> probably
> does not allow to run the cleanup:
>
> _tst_setup_timer()
> {
> ...
>         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> -$pid &
>
> I'm not sure if shell allow us to do it better. Maybe sent different
> signal than
> SIGKILL and define 'trap _tst_do_exit' for that signal?


Sounds practicable. I guess sending SIGINT could make more sense, since
sometimes we use CTRL+C stop test in debugging by manual, test should
do cleanup work for that behavior too.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210130/5616a62d/attachment-0001.htm>

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

Hi Li,

> On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:

> > > Hi,

> > > > +   TST_TIMEOUT=$((dev_num*200))
> > > Actually on heavy loaded machine this is not enough due BTRFS.
> > > I can add something like dev_num*600 or even -1 (then previous commit
> > would not
> > > be needed, but IMHO still useful).


> I personally think -1 is better.
OK. In that case we might avoid now unneeded previous commit.



> > And bad thing is that it breaks other zram tests, because the timer
> > probably
> > does not allow to run the cleanup:

> > _tst_setup_timer()
> > {
> > ...
> >         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> > running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> > -$pid &

> > I'm not sure if shell allow us to do it better. Maybe sent different
> > signal than
> > SIGKILL and define 'trap _tst_do_exit' for that signal?


> Sounds practicable. I guess sending SIGINT could make more sense, since
> sometimes we use CTRL+C stop test in debugging by manual, test should
> do cleanup work for that behavior too.
We have already SIGINT defined for main shell process:
trap "tst_brk TBROK 'test interrupted'" INT

so CTRL+C is covered. So maybe run first SIGINT and then SIGKILL for safety
reasons?

Kind regards,
Petr

diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
index 4fa1674cd..61a6fbcdd 100644
--- testcases/lib/tst_test.sh
+++ testcases/lib/tst_test.sh
@@ -459,7 +459,7 @@ _tst_setup_timer()
 
 	tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"
 
-	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9 -$pid &
+	sleep $sec && tst_res TBROK "test killed, timeout! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && { kill -2 -$pid; sleep 5; kill -9 -$pid; } &
 
 	_tst_setup_timer_pid=$!
 }

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Li Wang]

Hi Petr,

On Tue, Feb 2, 2021 at 3:38 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > On Sat, Jan 30, 2021 at 4:16 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > > Hi,
>
> > > > > +   TST_TIMEOUT=$((dev_num*200))
> > > > Actually on heavy loaded machine this is not enough due BTRFS.
> > > > I can add something like dev_num*600 or even -1 (then previous commit
> > > would not
> > > > be needed, but IMHO still useful).
>
>
> > I personally think -1 is better.
> OK. In that case we might avoid now unneeded previous commit.
>
>
>
> > > And bad thing is that it breaks other zram tests, because the timer
> > > probably
> > > does not allow to run the cleanup:
>
> > > _tst_setup_timer()
> > > {
> > > ...
> > >         sleep $sec && tst_res TBROK "test killed, timeout! If you are
> > > running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
> > > -$pid &
>
> > > I'm not sure if shell allow us to do it better. Maybe sent different
> > > signal than
> > > SIGKILL and define 'trap _tst_do_exit' for that signal?
>
>
> > Sounds practicable. I guess sending SIGINT could make more sense, since
> > sometimes we use CTRL+C stop test in debugging by manual, test should
> > do cleanup work for that behavior too.
> We have already SIGINT defined for main shell process:
> trap "tst_brk TBROK 'test interrupted'" INT
>
> so CTRL+C is covered. So maybe run first SIGINT and then SIGKILL for safety
> reasons?
>

I'm fine with using SIGINT + SIGKILL.

Or, maybe another choice is to catch a signal SIGTERM?  Once we
cancel the specified signal in KILL command, it will try with SIGTERM
by default, if the test process can not be terminated, then a SIGKILL
may be sent.

--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -22,6 +22,7 @@ export TST_LIB_LOADED=1

 # default trap function
 trap "tst_brk TBROK 'test interrupted'" INT
+trap "_tst_do_exit" TERM

 _tst_do_exit()
 {
@@ -459,7 +460,7 @@ _tst_setup_timer()

        tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s"

-       sleep $sec && tst_res TBROK "test killed, timeout! If you are
running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill -9
-$pid &
+       sleep $sec && tst_res TBROK "test terminated, timeout! If you are
running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" && kill --
-$pid &

        _tst_setup_timer_pid=$!


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210202/04007113/attachment.htm>

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Cyril Hrubis]

Hi!
> > +	TST_TIMEOUT=$((dev_num*200))
> Actually on heavy loaded machine this is not enough due BTRFS.
> I can add something like dev_num*600 or even -1 (then previous commit would not
> be needed, but IMHO still useful).

I would still prefer if we had a timeout there, -1 is for something that
cannot be predicted.

Also we do not expect machine to be heavily loaded, in that case half of
LTP tests would time out.

So I would just measure how long the test takes, then multiply it by 5
or something like that and put that in as a timeout.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

> Hi!
> > > +	TST_TIMEOUT=$((dev_num*200))
> > Actually on heavy loaded machine this is not enough due BTRFS.
> > I can add something like dev_num*600 or even -1 (then previous commit would not
> > be needed, but IMHO still useful).

> I would still prefer if we had a timeout there, -1 is for something that
> cannot be predicted.

> Also we do not expect machine to be heavily loaded, in that case half of
> LTP tests would time out.

> So I would just measure how long the test takes, then multiply it by 5
> or something like that and put that in as a timeout.
Do you mean to use high enough static timeout defined before startup (working
for maximum possible filesystems)? And create tst_set_timeout() for shell as
independent effort?

Kind regards,
Petr

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

> > Hi!
> > > > +	TST_TIMEOUT=$((dev_num*200))
> > > Actually on heavy loaded machine this is not enough due BTRFS.
> > > I can add something like dev_num*600 or even -1 (then previous commit would not
> > > be needed, but IMHO still useful).

> > I would still prefer if we had a timeout there, -1 is for something that
> > cannot be predicted.

> > Also we do not expect machine to be heavily loaded, in that case half of
> > LTP tests would time out.

> > So I would just measure how long the test takes, then multiply it by 5
> > or something like that and put that in as a timeout.
> Do you mean to use high enough static timeout defined before startup (working
> for maximum possible filesystems)? And create tst_set_timeout() for shell as
> independent effort?

BTW looking at the docs for C API tst_set_timeout() is expected to be run in the
setup function. Thus this patchset can be reused (just extended).

Kind regards,
Petr

> Kind regards,
> Petr

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Cyril Hrubis]

Hi!
> > I would still prefer if we had a timeout there, -1 is for something that
> > cannot be predicted.
> 
> > Also we do not expect machine to be heavily loaded, in that case half of
> > LTP tests would time out.
> 
> > So I would just measure how long the test takes, then multiply it by 5
> > or something like that and put that in as a timeout.
> Do you mean to use high enough static timeout defined before startup (working
> for maximum possible filesystems)? And create tst_set_timeout() for shell as
> independent effort?

I would do:

* Add tst_set_timeout for shell, so that it mirrors the C API

* Measure runtime of the test divide it by the number of supported
  filesystems, that way we would get mean runtime per filesystem

  now I would multiply this number with arbitrary constantm, e.g. 5 or
  even more, this is now timeout per iteration

  with this number the actuall timeout would be:

  number_of_filesystems * mean_max_per_run


Does this sound sane?

I guess that in the end we will end up with something similar what you
had there, but we would have some data that supports this decision.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 6/6] zram: Increase timeout according to used devices

[Petr Vorel]

> Hi!
> > > I would still prefer if we had a timeout there, -1 is for something that
> > > cannot be predicted.

> > > Also we do not expect machine to be heavily loaded, in that case half of
> > > LTP tests would time out.

> > > So I would just measure how long the test takes, then multiply it by 5
> > > or something like that and put that in as a timeout.
> > Do you mean to use high enough static timeout defined before startup (working
> > for maximum possible filesystems)? And create tst_set_timeout() for shell as
> > independent effort?

> I would do:

> * Add tst_set_timeout for shell, so that it mirrors the C API
+1. BTW C has .all_filesystems for timeout for each run, which allows not to
bother with timeout depending on number of filesystems (unlike fuzzy sync, which
also sometimes needs tweak fzsync_pair.exec_time_p). I'm for ignoring this fact,
just to let know that shell API is far behind C API.

> * Measure runtime of the test divide it by the number of supported
>   filesystems, that way we would get mean runtime per filesystem

>   now I would multiply this number with arbitrary constantm, e.g. 5 or
>   even more, this is now timeout per iteration

>   with this number the actuall timeout would be:

>   number_of_filesystems * mean_max_per_run


> Does this sound sane?
+1, thanks!

> I guess that in the end we will end up with something similar what you
> had there, but we would have some data that supports this decision.
+1

Kind regards,
Petr