| Message ID | 20210129194144.31299-3-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | zram cleanup | expand |
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
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()?
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
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.
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.
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
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.
Hi!
Looks good including the changes proposed in the thread.
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
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
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"
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(-)