[LTP] [PATCHv2 0/3] zram/zram_lib.sh: fix zram_compress_alg() test for zram01

[LTP] [PATCHv2 0/3] zram/zram_lib.sh: fix zram_compress_alg() test for zram01

[Po-Hsu Lin]

The zram_compress_alg() test in zram01 is not working properly, this
patchset will address the following issues:
  1. Inconsistent variable name for compression algorithms, and the
     algorithm name needs to be sanitized before use.
  2. Hard-coded block device numbers to iterate with. If the number of
     the available zram compression algorithms is larger than the
     number of block devices, the test will fail.
  3. Restore the compression algorithm back to the default one.

Po-Hsu Lin (3):
  zram/zram_lib.sh: fix variable name and algorithm retrieval
  zram/zram_lib.sh: iterate through all available compression algorithms
    for all zram block devices
  zram/zram_lib.sh: set the compression algorithms back to default after
    test

 .../kernel/device-drivers/zram/zram_lib.sh    | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

-- 
2.17.1

[LTP] [PATCHv2 1/3] zram/zram_lib.sh: fix variable name and algorithm retrieval

[Po-Hsu Lin]

The compression algorithm was stored into a local variable "algs",
however the variable name zram_algs was used in the for loop later.

Unify them with algs so the default zram_algs defined in zram01 won't
be altered.

Also, use sed to get rid of the square brackets that indicates the
compression algorithm currently in use.
    $ cat /sys/block/zram0/comp_algorithm
    [lzo] lz4 lz4hc 842 zstd

Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 testcases/kernel/device-drivers/zram/zram_lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index d0e7704a8..599e5f0f3 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -98,10 +98,10 @@ zram_compress_alg()
 
 	tst_resm TINFO "test that we can set compression algorithm"
 
-	local algs="$(cat /sys/block/zram0/comp_algorithm)"
+	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
 	tst_resm TINFO "supported algs: $algs"
 	local i=0
-	for alg in $zram_algs; do
+	for alg in $algs; do
 		local sys_path="/sys/block/zram${i}/comp_algorithm"
 		echo "$alg" >  $sys_path || \
 			tst_brkm TFAIL "can't set '$alg' to $sys_path"
-- 
2.17.1

[LTP] [PATCHv2 1/3] zram/zram_lib.sh: fix variable name and algorithm retrieval

[Petr Vorel]

Hi,

> The compression algorithm was stored into a local variable "algs",
> however the variable name zram_algs was used in the for loop later.

> Unify them with algs so the default zram_algs defined in zram01 won't
> be altered.

> Also, use sed to get rid of the square brackets that indicates the
> compression algorithm currently in use.
>     $ cat /sys/block/zram0/comp_algorithm
>     [lzo] lz4 lz4hc 842 zstd

> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
>  testcases/kernel/device-drivers/zram/zram_lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

> diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
> index d0e7704a8..599e5f0f3 100755
> --- a/testcases/kernel/device-drivers/zram/zram_lib.sh
> +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
> @@ -98,10 +98,10 @@ zram_compress_alg()

>  	tst_resm TINFO "test that we can set compression algorithm"

> -	local algs="$(cat /sys/block/zram0/comp_algorithm)"
> +	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
>  	tst_resm TINFO "supported algs: $algs"
>  	local i=0
> -	for alg in $zram_algs; do
> +	for alg in $algs; do
>  		local sys_path="/sys/block/zram${i}/comp_algorithm"
>  		echo "$alg" >  $sys_path || \
>  			tst_brkm TFAIL "can't set '$alg' to $sys_path"

Sorry for a late reply.

What is the purpose of zram_algs="lzo lzo lzo lzo in zram01.sh?
It should be removed now, right? (as you decided not to set the algorithms to the ones defined in the zram01.sh
test at the end of this function as Cyril suggested at [1]

Kind regards,
Petr

[1] http://lists.linux.it/pipermail/ltp/2019-July/012674.html

[LTP] [PATCHv2 1/3] zram/zram_lib.sh: fix variable name and algorithm retrieval

[Po-Hsu Lin]

Hello Petr,

thanks for the reply, and sorry for the late response too, need some
time to throw myself back in time.

To my understanding, the zram_algs="lzo lzo lzo lzo" in zram01.sh is a
dummy mapping (placeholder?) for 4 compression algorithms with 4
different setup, one for (zram_sizes=26214400, zram_mem_limits=25M,
zram_filesystems=ext3), and one for (zram_sizes=26214400,
zram_mem_limits=25M, zram_filesystems=ext4) and so on.

With this patch the test will be more comprehensive, as it's not
trying to set the algorithm to "lzo" 4 times (as defined in zram_algs
from zram01.sh), but try to switch to all supported algorithm reported
back from  /sys/block/zram0/comp_algorithm

So yes, this zram_algs in zram01.sh will not be used at all after
applying my patch here, maybe it can be removed but I am not sure if
we should keep it there as a placeholder.

Cheers


On Fri, Feb 21, 2020 at 1:59 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi,
>
> > The compression algorithm was stored into a local variable "algs",
> > however the variable name zram_algs was used in the for loop later.
>
> > Unify them with algs so the default zram_algs defined in zram01 won't
> > be altered.
>
> > Also, use sed to get rid of the square brackets that indicates the
> > compression algorithm currently in use.
> >     $ cat /sys/block/zram0/comp_algorithm
> >     [lzo] lz4 lz4hc 842 zstd
>
> > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> > ---
> >  testcases/kernel/device-drivers/zram/zram_lib.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> > diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
> > index d0e7704a8..599e5f0f3 100755
> > --- a/testcases/kernel/device-drivers/zram/zram_lib.sh
> > +++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
> > @@ -98,10 +98,10 @@ zram_compress_alg()
>
> >       tst_resm TINFO "test that we can set compression algorithm"
>
> > -     local algs="$(cat /sys/block/zram0/comp_algorithm)"
> > +     local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
> >       tst_resm TINFO "supported algs: $algs"
> >       local i=0
> > -     for alg in $zram_algs; do
> > +     for alg in $algs; do
> >               local sys_path="/sys/block/zram${i}/comp_algorithm"
> >               echo "$alg" >  $sys_path || \
> >                       tst_brkm TFAIL "can't set '$alg' to $sys_path"
>
> Sorry for a late reply.
>
> What is the purpose of zram_algs="lzo lzo lzo lzo in zram01.sh?
> It should be removed now, right? (as you decided not to set the algorithms to the ones defined in the zram01.sh
> test at the end of this function as Cyril suggested at [1]
>
> Kind regards,
> Petr
>
> [1] http://lists.linux.it/pipermail/ltp/2019-July/012674.html

[LTP] [PATCHv2 1/3] zram/zram_lib.sh: fix variable name and algorithm retrieval

[Petr Vorel]

Hi Po-Hsu,

> thanks for the reply, and sorry for the late response too, need some
> time to throw myself back in time.

> To my understanding, the zram_algs="lzo lzo lzo lzo" in zram01.sh is a
> dummy mapping (placeholder?) for 4 compression algorithms with 4
> different setup, one for (zram_sizes=26214400, zram_mem_limits=25M,
> zram_filesystems=ext3), and one for (zram_sizes=26214400,
> zram_mem_limits=25M, zram_filesystems=ext4) and so on.

> With this patch the test will be more comprehensive, as it's not
> trying to set the algorithm to "lzo" 4 times (as defined in zram_algs
> from zram01.sh), but try to switch to all supported algorithm reported
> back from  /sys/block/zram0/comp_algorithm

> So yes, this zram_algs in zram01.sh will not be used at all after
> applying my patch here, maybe it can be removed but I am not sure if
> we should keep it there as a placeholder.

Also sorry for the delay. I rebased and merged these 2 commits, removed
$zram_algs (as it's was not used any more).
Although this and second commit should be probably as a single commit (to be an
atomic change), I kept them separate.

Thanks for your contributions.

Kind regards,
Petr

[LTP] [PATCHv2 2/3] zram/zram_lib.sh: iterate through all available compression algorithms for all zram block devices

[Po-Hsu Lin]

The test was designed to switch up to 4 zram compression algorithms,
one of them on a zram block device at a time through 4 devices as
defined in zram01.sh.

As the number of zram block devices are hard-coded in zram01.sh, the
test will fail if your system supports more than 4 compression
algorithms because it will try to access a non-existing block device:
    zram01.sh: cannot create /sys/block/zram4/comp_algorithm: Directory nonexistent
    zram01 2 TFAIL: can't set 'zstd' to /sys/block/zram4/comp_algorithm

Fix this by using the number of devices defined in zram01.sh. And
iterate all the available compression algorithms on each block device.
This ensures all the algorithm can be assigned on all of the zram block
devices and thus improves the test coverage.

Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 testcases/kernel/device-drivers/zram/zram_lib.sh | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 599e5f0f3..7773f4338 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -101,12 +101,15 @@ zram_compress_alg()
 	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
 	tst_resm TINFO "supported algs: $algs"
 	local i=0
-	for alg in $algs; do
-		local sys_path="/sys/block/zram${i}/comp_algorithm"
-		echo "$alg" >  $sys_path || \
-			tst_brkm TFAIL "can't set '$alg' to $sys_path"
-		i=$(($i + 1))
-		tst_resm TINFO "$sys_path = '$alg' ($i/$dev_num)"
+	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_brkm TFAIL "can't set '$alg' to $sys_path"
+			tst_resm TINFO "$sys_path = '$alg' ($i/$dev_max)"
+		done
 	done
 
 	tst_resm TPASS "test succeeded"
-- 
2.17.1

[LTP] [PATCH 3/3] zram/zram_lib.sh: set the compression algorithms back to default after test

[Po-Hsu Lin]

Set the compression algorithm back to the default one in the end of the
zram_compress_alg() test.

Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
---
 testcases/kernel/device-drivers/zram/zram_lib.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/testcases/kernel/device-drivers/zram/zram_lib.sh b/testcases/kernel/device-drivers/zram/zram_lib.sh
index 7773f4338..e2b6eb8ec 100755
--- a/testcases/kernel/device-drivers/zram/zram_lib.sh
+++ b/testcases/kernel/device-drivers/zram/zram_lib.sh
@@ -99,6 +99,7 @@ zram_compress_alg()
 	tst_resm TINFO "test that we can set compression algorithm"
 
 	local algs="$(sed 's/[][]//g' /sys/block/zram0/comp_algorithm)"
+	local alg_default="$(grep -oP '(?<=\[).*(?=\])' /sys/block/zram0/comp_algorithm)"
 	tst_resm TINFO "supported algs: $algs"
 	local i=0
 	local dev_max=$(($dev_num - 1))
@@ -110,6 +111,12 @@ zram_compress_alg()
 				tst_brkm TFAIL "can't set '$alg' to $sys_path"
 			tst_resm TINFO "$sys_path = '$alg' ($i/$dev_max)"
 		done
+		# Restore the compression algorithm to the default one
+		if [ -n "$alg_default" ]; then
+			echo "$alg_default" >  $sys_path || \
+				tst_brkm TFAIL "can't restore '$alg_default' to $sys_path"
+			tst_resm TINFO "Set $sys_path back to $alg_default"
+		fi
 	done
 
 	tst_resm TPASS "test succeeded"
-- 
2.17.1

[LTP] [PATCH 3/3] zram/zram_lib.sh: set the compression algorithms back to default after test

[Petr Vorel]

Hi Po-Hsu,

> Set the compression algorithm back to the default one in the end of the
> zram_compress_alg() test.

Why is this needed? We remove zram in zram_cleanup() anyway.
This would make sense if we detect whether zram was loaded before testing and
try to restore the setup of originally loaded module instead of always removing it.

Kind regards,
Petr

[LTP] [PATCH 3/3] zram/zram_lib.sh: set the compression algorithms back to default after test

[Po-Hsu Lin]

Hello Petr,

Yes you're right, the zram_cleanup() will kick in after zram01.sh finishes,
so this can be dropped for the moment I think.

Thanks for the review!


On Wed, Mar 18, 2020 at 2:21 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Po-Hsu,
>
> > Set the compression algorithm back to the default one in the end of the
> > zram_compress_alg() test.
>
> Why is this needed? We remove zram in zram_cleanup() anyway.
> This would make sense if we detect whether zram was loaded before testing and
> try to restore the setup of originally loaded module instead of always removing it.
>
> Kind regards,
> Petr