[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

The minimal size for Btrfs filesystem on ppc64le is around 139MB, most
likely because of larger page size, hence the test fails.

This commit adds tst_min_fs_size.sh library, that parses mkfs.btrfs
output which includes minimal filesystem size in case of a failure.

The code also falls back to 150MB if the mkfs.btrfs output wasn't parsed
correctly.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/device-drivers/zram/zram01.sh |  8 ++++--
 testcases/lib/tst_min_fs_size.sh               | 35 ++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 testcases/lib/tst_min_fs_size.sh

diff --git a/testcases/kernel/device-drivers/zram/zram01.sh b/testcases/kernel/device-drivers/zram/zram01.sh
index 64284d5..f0dc83c 100755
--- a/testcases/kernel/device-drivers/zram/zram01.sh
+++ b/testcases/kernel/device-drivers/zram/zram01.sh
@@ -24,6 +24,7 @@ TCID="zram01"
 TST_TOTAL=8
 
 . test.sh
+. tst_min_fs_size.sh
 . zram_lib.sh
 
 # Test will create the following number of zram devices:
@@ -39,8 +40,11 @@ zram_max_streams="2 3 5 8"
 # 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 41943040" # 25MB, 40MB for btrfs
-zram_mem_limits="25M 25M 25M 40M"
+min_btrfs_size=$(tst_min_btrfs_size)
+min_btrfs_size_mb=$((min_btrfs_size/1024/1024))
+
+zram_sizes="26214400 26214400 26214400 $min_btrfs_size" # 25MB, 40MB for btrfs
+zram_mem_limits="25M 25M 25M ${min_btrfs_size_mb}M"
 zram_filesystems="ext3 ext4 xfs btrfs"
 zram_algs="lzo lzo lzo lzo"
 
diff --git a/testcases/lib/tst_min_fs_size.sh b/testcases/lib/tst_min_fs_size.sh
new file mode 100644
index 0000000..9428ff9
--- /dev/null
+++ b/testcases/lib/tst_min_fs_size.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+#
+# Copyright (c) Linux Test Project, 2017
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Written by Cyril Hrubis <chrubis@suse.cz>
+#
+
+tst_min_btrfs_size()
+{
+	local output
+
+	output=$(mkfs.btrfs -q /dev/null 2>&1)
+	output=$(echo $output | awk -F' ' '{print substr($NF, 1, length($NF) - 1)}' -)
+
+	if [ "$output" -a "$output" -eq "$output" ] 2>/dev/null; then
+		echo "$output"
+	else
+		echo "Invalid size ($output) reported by mkfs.btrfs, defaulting to 150MB" 1>&2
+		echo $((150 * 1024 * 1024))
+	fi
+}
-- 
2.7.3

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: ltp@lists.linux.it
> Sent: Tuesday, 31 January, 2017 2:44:33 PM
> Subject: [LTP] [PATCH] [RFC] zram01: Fix on ppc64le
> 
> The minimal size for Btrfs filesystem on ppc64le is around 139MB, most
> likely because of larger page size, hence the test fails.
> 
> This commit adds tst_min_fs_size.sh library, that parses mkfs.btrfs
> output which includes minimal filesystem size in case of a failure.
> 
> The code also falls back to 150MB if the mkfs.btrfs output wasn't parsed
> correctly.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/device-drivers/zram/zram01.sh |  8 ++++--
>  testcases/lib/tst_min_fs_size.sh               | 35
>  ++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>  create mode 100644 testcases/lib/tst_min_fs_size.sh

This is ~4 years old comment from Zach Brown, when I hit an issue on ppc,
where I could alloc only 1/2 of the volume size:

"That small volume mkfs warning is issued for devices less than a gig.  It indicates that btrfs has gone in to a weird degenerate allocation scheme.  We'd only support volumes much larger than that, though I have no quick rule to say how large starts to be reasonable.  Multiple gig, certainly."

I'm running with 384M since then, so far successfully. If we don't allocate
too much data on it, we might be OK, but still I'd go with minimum default of 256M.

Regards,
Jan

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> This is ~4 years old comment from Zach Brown, when I hit an issue on ppc,
> where I could alloc only 1/2 of the volume size:
> 
> "That small volume mkfs warning is issued for devices less than a gig.  It indicates that btrfs has gone in to a weird degenerate allocation scheme.  We'd only support volumes much larger than that, though I have no quick rule to say how large starts to be reasonable.  Multiple gig, certainly."
> 
> I'm running with 384M since then, so far successfully. If we don't allocate
> too much data on it, we might be OK, but still I'd go with minimum default of 256M.

What exactly do you have in mind? Using 256MB by default for any Btrfs
filesystem or fallback to 256MB if mkfs.btrfs output cannot be parsed?

I guess that for any other testcase it would be fine enough to bump the
minimal device size to 256MB unconditionally, but in this case we create
the data in RAM albeit compressed, and so I would like to keep it as
small as possible, since otherwise it may fail on embedded hardware.
Maybe we should just remove Btrfs from the zram01.sh test so that we
don't have to keep bumping the minimal size each time the minimal Btrfs
size calculation changes...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 1 February, 2017 10:45:41 AM
> Subject: Re: [LTP] [PATCH] [RFC] zram01: Fix on ppc64le
> 
> Hi!
> > This is ~4 years old comment from Zach Brown, when I hit an issue on ppc,
> > where I could alloc only 1/2 of the volume size:
> > 
> > "That small volume mkfs warning is issued for devices less than a gig.  It
> > indicates that btrfs has gone in to a weird degenerate allocation scheme.
> > We'd only support volumes much larger than that, though I have no quick
> > rule to say how large starts to be reasonable.  Multiple gig, certainly."
> > 
> > I'm running with 384M since then, so far successfully. If we don't allocate
> > too much data on it, we might be OK, but still I'd go with minimum default
> > of 256M.
> 
> What exactly do you have in mind? Using 256MB by default for any Btrfs
> filesystem or fallback to 256MB if mkfs.btrfs output cannot be parsed?

I meant default size.

> 
> I guess that for any other testcase it would be fine enough to bump the
> minimal device size to 256MB unconditionally, but in this case we create
> the data in RAM albeit compressed, and so I would like to keep it as
> small as possible, since otherwise it may fail on embedded hardware.

I didn't have a look at zram01, but can't we detect this and TCONF?
We can try with minimum and see how frequently it changes, I just
wanted to share Zach's quote and my experience with tiny btrfs volumes.

Regards,
Jan

> Maybe we should just remove Btrfs from the zram01.sh test so that we
> don't have to keep bumping the minimal size each time the minimal Btrfs
> size calculation changes...
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> > > This is ~4 years old comment from Zach Brown, when I hit an issue on ppc,
> > > where I could alloc only 1/2 of the volume size:
> > > 
> > > "That small volume mkfs warning is issued for devices less than a gig.  It
> > > indicates that btrfs has gone in to a weird degenerate allocation scheme.
> > > We'd only support volumes much larger than that, though I have no quick
> > > rule to say how large starts to be reasonable.  Multiple gig, certainly."
> > > 
> > > I'm running with 384M since then, so far successfully. If we don't allocate
> > > too much data on it, we might be OK, but still I'd go with minimum default
> > > of 256M.
> > 
> > What exactly do you have in mind? Using 256MB by default for any Btrfs
> > filesystem or fallback to 256MB if mkfs.btrfs output cannot be parsed?
> 
> I meant default size.

Then we should increase the default size for the LTP_DEV as well.
Unfortunately it's defined at three different places at this point. We
have it in runltp script, the C library and the shell library. I was
thinking of exporting the C library as a binary for the shell tests to
use in order to reduce the complexity. I will look into that and also
bump the minimal size as well.

> > I guess that for any other testcase it would be fine enough to bump the
> > minimal device size to 256MB unconditionally, but in this case we create
> > the data in RAM albeit compressed, and so I would like to keep it as
> > small as possible, since otherwise it may fail on embedded hardware.
> 
> I didn't have a look at zram01, but can't we detect this and TCONF?

I guess that we can look at total amount of RAM and enable/disable the
test with Btrfs filesystem if it's deemed too small.

> We can try with minimum and see how frequently it changes, I just
> wanted to share Zach's quote and my experience with tiny btrfs volumes.

I've been adjusting the minimal size (of the TST_DEVICE) two times
already, we bumped it to 100MB then to 150MB. The first one was
because mkfs.btrfs change, the second was because the size depends on
PAGE_SIZE and 100MB wasn't enough for ppc64le. I guess that bumping it
to 256MB is not unreasonable, and hopefully that would be enough for
some time.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> > > What exactly do you have in mind? Using 256MB by default for any Btrfs
> > > filesystem or fallback to 256MB if mkfs.btrfs output cannot be parsed?
> > 
> > I meant default size.
> 
> Then we should increase the default size for the LTP_DEV as well.
> Unfortunately it's defined at three different places at this point. We
> have it in runltp script, the C library and the shell library. I was
> thinking of exporting the C library as a binary for the shell tests to
> use in order to reduce the complexity. I will look into that and also
> bump the minimal size as well.

And apparenlty it was defined in four different places, since we have two
versions of shell library each with it's own code to acquire device.

I've send a patchset that exports the acquire/release interface from C
as a single binary and makes use of that in the tst_test.sh.

There is still one unsolved problem since the TWARN messages from the
tst_device.c will trigger TBROK in tst_test.c since the IPC is not
initialized. I'm still unsure how to fix that, maybe we should allow to
allocate non-shared structure for the results in the special case that
the library code is being reused in shell helpers.

Finally the last patch of the series bumps the minima size to 256MB and
touched only two places, the runltp script and tst_device.c, which is
way better than four...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> There is still one unsolved problem since the TWARN messages from the
> tst_device.c will trigger TBROK in tst_test.c since the IPC is not
> initialized. I'm still unsure how to fix that, maybe we should allow to
> allocate non-shared structure for the results in the special case that
> the library code is being reused in shell helpers.

Maybe we should just add an API to redirect tst_brk() and tst_res() so
that we could use library code in the various non-test utilities. Maybe
we could patch things up so that we could use SAFE_MACROS() in cleanup
as well...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Thursday, 9 February, 2017 1:02:45 PM
> Subject: Re: [LTP] [PATCH] [RFC] zram01: Fix on ppc64le
> 
> Hi!
> > There is still one unsolved problem since the TWARN messages from the
> > tst_device.c will trigger TBROK in tst_test.c since the IPC is not
> > initialized. I'm still unsure how to fix that, maybe we should allow to
> > allocate non-shared structure for the results in the special case that
> > the library code is being reused in shell helpers.
> 
> Maybe we should just add an API to redirect tst_brk() and tst_res() so
> that we could use library code in the various non-test utilities.

Or drop that TBROK?

diff --git a/lib/tst_test.c b/lib/tst_test.c
index e78b412cbd9b..52f85c7af891 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -146,10 +146,8 @@ void tst_reinit(void)
 
 static void update_results(const char *file, unsigned int lineno, int ttype)
 {
-       if (!results) {
-               tst_brk(TBROK,
-                       "%s: %d: Results IPC not initialized!", file, lineno);
-       }
+       if (!results)
+               return;
 
        switch (ttype) {
        case TCONF:

There's no way "results" can be NULL when in test:
main
 tst_run_tcases
  do_setup
   setup_ipc
    SAFE_MMAP

> Maybe
> we could patch things up so that we could use SAFE_MACROS() in cleanup
> as well...

This should be doable with some flag we set in do_test_cleanup(), to skip
further calls.

Regards,
Jan

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> > > There is still one unsolved problem since the TWARN messages from the
> > > tst_device.c will trigger TBROK in tst_test.c since the IPC is not
> > > initialized. I'm still unsure how to fix that, maybe we should allow to
> > > allocate non-shared structure for the results in the special case that
> > > the library code is being reused in shell helpers.
> > 
> > Maybe we should just add an API to redirect tst_brk() and tst_res() so
> > that we could use library code in the various non-test utilities.
> 
> Or drop that TBROK?

It's especially there for code that defines TST_NO_DEFAULT_MAIN but one
can argue that this is special case and if you define that macro you
should know what you are doing anyway...

But still I prefer to throw error message instead of segfaulting while
trying to deference NULL pointer.

> > Maybe
> > we could patch things up so that we could use SAFE_MACROS() in cleanup
> > as well...
> 
> This should be doable with some flag we set in do_test_cleanup(), to skip
> further calls.

I'm nearly finished with RFC patch. The main problem is the
__attribute__((noreturn)) that has been added for various tst_brk*
variants. So in the end it looks like only solution is to do the
tst_brk_() redirection in the safe_macros.c, since we cannot return from
tst_brkm_() since the return address is not stored on stack because it
has noreturn attribute. And dropping the noreturn attribute from
tst_brkm_() is not an option either, since that generates a ton of
"control reaches end of non-void function" warnings.

But it's quite easy to define a macro or two that does the same
redirection as we do in tst_brkm_() so that we never reach the oldlib
code from safe_macros.c if newlib test is running.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Thursday, 9 February, 2017 3:00:42 PM
> Subject: Re: [LTP] [PATCH] [RFC] zram01: Fix on ppc64le
> 
> Hi!
> > > > There is still one unsolved problem since the TWARN messages from the
> > > > tst_device.c will trigger TBROK in tst_test.c since the IPC is not
> > > > initialized. I'm still unsure how to fix that, maybe we should allow to
> > > > allocate non-shared structure for the results in the special case that
> > > > the library code is being reused in shell helpers.
> > > 
> > > Maybe we should just add an API to redirect tst_brk() and tst_res() so
> > > that we could use library code in the various non-test utilities.
> > 
> > Or drop that TBROK?
> 
> It's especially there for code that defines TST_NO_DEFAULT_MAIN but one
> can argue that this is special case and if you define that macro you
> should know what you are doing anyway...
> 
> But still I prefer to throw error message instead of segfaulting while
> trying to deference NULL pointer.

I was suggesting we skip update_results(), not to let it segfault.

> 
> > > Maybe
> > > we could patch things up so that we could use SAFE_MACROS() in cleanup
> > > as well...
> > 
> > This should be doable with some flag we set in do_test_cleanup(), to skip
> > further calls.
> 
> I'm nearly finished with RFC patch. The main problem is the
> __attribute__((noreturn)) that has been added for various tst_brk*
> variants. So in the end it looks like only solution is to do the
> tst_brk_() redirection in the safe_macros.c, since we cannot return from
> tst_brkm_() since the return address is not stored on stack because it
> has noreturn attribute. And dropping the noreturn attribute from
> tst_brkm_() is not an option either, since that generates a ton of
> "control reaches end of non-void function" warnings.

Vast majority is from single header file:

$ grep "reaches end of non-void" log.txt | sort | uniq | wc -l
328

$ grep "reaches end of non-void" log.txt | sort | uniq | grep compat_16.h | wc -l
289

> 
> But it's quite easy to define a macro or two that does the same
> redirection as we do in tst_brkm_() so that we never reach the oldlib
> code from safe_macros.c if newlib test is running.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> > It's especially there for code that defines TST_NO_DEFAULT_MAIN but one
> > can argue that this is special case and if you define that macro you
> > should know what you are doing anyway...
> > 
> > But still I prefer to throw error message instead of segfaulting while
> > trying to deference NULL pointer.
> 
> I was suggesting we skip update_results(), not to let it segfault.

Sorry I was blind. That makes much more sense. Let's go with that one,
instead of the:

"[LTP] [PATCH 1/6] tst_test: Allow priting TINFO without initialized IPC"

> > > > Maybe
> > > > we could patch things up so that we could use SAFE_MACROS() in cleanup
> > > > as well...
> > > 
> > > This should be doable with some flag we set in do_test_cleanup(), to skip
> > > further calls.
> > 
> > I'm nearly finished with RFC patch. The main problem is the
> > __attribute__((noreturn)) that has been added for various tst_brk*
> > variants. So in the end it looks like only solution is to do the
> > tst_brk_() redirection in the safe_macros.c, since we cannot return from
> > tst_brkm_() since the return address is not stored on stack because it
> > has noreturn attribute. And dropping the noreturn attribute from
> > tst_brkm_() is not an option either, since that generates a ton of
> > "control reaches end of non-void function" warnings.
> 
> Vast majority is from single header file:
> 
> $ grep "reaches end of non-void" log.txt | sort | uniq | wc -l
> 328
> 
> $ grep "reaches end of non-void" log.txt | sort | uniq | grep compat_16.h | wc -l
> 289

That is still 39 occurences to fix all over the place plus the
compat_16.h header. Have a look at the patch I've just send, I think
that it's a bit easier than patching all the tests to avoids warnings...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
Looks like we went off-topic and the zram01.sh test wasn't fixed in the
end and it still fails on ppc64le.

I guess I can prepare a patch that checks if there is enough free RAM
for a minimal Btrfs fs but I would still like to obtain the minimal size
from mkfs.btrfs. Does that sound reasonable?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> Hi!
> Looks like we went off-topic and the zram01.sh test wasn't fixed in the
> end and it still fails on ppc64le.
> 
> I guess I can prepare a patch that checks if there is enough free RAM
> for a minimal Btrfs fs but I would still like to obtain the minimal size
> from mkfs.btrfs. Does that sound reasonable?

Do you mean these messages from mkfs.btrfs:
  "ERROR: minimum size for each btrfs device is 41943040"
or some other way to find minimal supported size?

Regards,
Jan

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> > Looks like we went off-topic and the zram01.sh test wasn't fixed in the
> > end and it still fails on ppc64le.
> > 
> > I guess I can prepare a patch that checks if there is enough free RAM
> > for a minimal Btrfs fs but I would still like to obtain the minimal size
> > from mkfs.btrfs. Does that sound reasonable?
> 
> Do you mean these messages from mkfs.btrfs:
>   "ERROR: minimum size for each btrfs device is 41943040"
> or some other way to find minimal supported size?

Do do not know other way that the one used in the original patch, i.e.
running mkfs.btrfs on /dev/null and parsing the output.

Or is there something better than that?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Jan Stancek]

----- Original Message -----
> Hi!
> > > Looks like we went off-topic and the zram01.sh test wasn't fixed in the
> > > end and it still fails on ppc64le.
> > > 
> > > I guess I can prepare a patch that checks if there is enough free RAM
> > > for a minimal Btrfs fs but I would still like to obtain the minimal size
> > > from mkfs.btrfs. Does that sound reasonable?
> > 
> > Do you mean these messages from mkfs.btrfs:
> >   "ERROR: minimum size for each btrfs device is 41943040"
> > or some other way to find minimal supported size?
> 
> Do do not know other way that the one used in the original patch, i.e.
> running mkfs.btrfs on /dev/null and parsing the output.
> 
> Or is there something better than that?

Other than picking a number by ourselves, nothing comes to mind.
Numbers from mkfs.btrfs error message seem too low, even 2.5x
larger number still fails:

# modprobe brd rd_nr=1 rd_size=100000
# mkfs.btrfs /dev/ram0 
btrfs-progs v4.9.1
See http://btrfs.wiki.kernel.org for more information.

ERROR: not enough free space to allocate chunk

Regards,
Jan

[LTP] [PATCH] [RFC] zram01: Fix on ppc64le

[Cyril Hrubis]

Hi!
> Other than picking a number by ourselves, nothing comes to mind.
> Numbers from mkfs.btrfs error message seem too low, even 2.5x
> larger number still fails:
> 
> # modprobe brd rd_nr=1 rd_size=100000
> # mkfs.btrfs /dev/ram0 
> btrfs-progs v4.9.1
> See http://btrfs.wiki.kernel.org for more information.
> 
> ERROR: not enough free space to allocate chunk

Now that you mention it I remember some patches for mkfs.btrfs that
fixed the minimal FS size computation. So I guess that hardcoding 384MB
would be a bit safer.

-- 
Cyril Hrubis
chrubis@suse.cz