diff mbox series

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

Message ID 20210129194144.31299-3-pvorel@suse.cz
State New
Headers show
Series zram cleanup | expand

Commit Message

Petr Vorel Jan. 29, 2021, 7:41 p.m. UTC
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(-)

Comments

Petr Vorel Jan. 29, 2021, 8:17 p.m. UTC | #1
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
Li Wang Jan. 30, 2021, 5:37 a.m. UTC | #2
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()?
Petr Vorel Feb. 2, 2021, 7:17 a.m. UTC | #3
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
Li Wang Feb. 2, 2021, 11:16 a.m. UTC | #4
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.
Petr Vorel Feb. 6, 2021, 7:59 p.m. UTC | #5
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.
Petr Vorel Feb. 8, 2021, 7:11 a.m. UTC | #6
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
Li Wang Feb. 18, 2021, 9 a.m. UTC | #7
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.
diff mbox series

Patch

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"