[PATCH v2] tests: Don't run f2fs test on systems with PAGE_SIZE more than 4096 bytes

[PATCH v2] tests: Don't run f2fs test on systems with PAGE_SIZE more than 4096 bytes

[Anatoly Pugachev]

Don't run f2fs test on systems with PAGE_SIZE > 4096 bytes, since f2fs is not
supported on these systems and trying to mount a f2fs filesystem would fail.

v2 changes:

- fix compare
- quotes around variable expansion

Signed-off-by: Anatoly Pugachev <matorola@gmail.com>
Reviewed-by: Mike Gilbert <floppym@gentoo.org>

---
 tests/f2fs_test.in | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/f2fs_test.in b/tests/f2fs_test.in
index 1ea77c826..3da1dad57 100644
--- a/tests/f2fs_test.in
+++ b/tests/f2fs_test.in
@@ -15,5 +15,11 @@ if ! which mkfs.f2fs >/dev/null 2>&1; then
  exit 77
 fi
 
+PAGE_SIZE=$(getconf PAGE_SIZE)
+F2FS_BLKSIZE=4096
+if [ "$PAGE_SIZE" -gt "$F2FS_BLKSIZE" ]; then
+ printf "f2fs is not supported on PAGE_SIZE(%d) != %d\n" $PAGE_SIZE $F2FS_BLKSIZE
+ exit 77
+fi
 
 "@builddir@/grub-fs-tester" f2fs
-- 
2.26.2

Re: [PATCH v2] tests: Don't run f2fs test on systems with PAGE_SIZE more than 4096 bytes

[Hans Ulrich Niedermann]

On Wed, 29 Apr 2020 17:06:36 +0300
Anatoly Pugachev <matorola@gmail.com> wrote:

> Don't run f2fs test on systems with PAGE_SIZE > 4096 bytes, since
> f2fs is not supported on these systems and trying to mount a f2fs
> filesystem would fail.

"Skip the f2fs test on ..." might be better wording, both in this
paragraph and the Subject.

Exit code 77 is certainly documented with the word "skip", and "exit
77" will show up in the "make check" output as "SKIP" as well.

> v2 changes:
> 
> - fix compare
> - quotes around variable expansion
> 
> Signed-off-by: Anatoly Pugachev <matorola@gmail.com>
> Reviewed-by: Mike Gilbert <floppym@gentoo.org>
> 
> ---
>  tests/f2fs_test.in | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/f2fs_test.in b/tests/f2fs_test.in
> index 1ea77c826..3da1dad57 100644
> --- a/tests/f2fs_test.in
> +++ b/tests/f2fs_test.in
> @@ -15,5 +15,11 @@ if ! which mkfs.f2fs >/dev/null 2>&1; then
>   exit 77
>  fi
>  
> +PAGE_SIZE=$(getconf PAGE_SIZE)
> +F2FS_BLKSIZE=4096
> +if [ "$PAGE_SIZE" -gt "$F2FS_BLKSIZE" ]; then
> + printf "f2fs is not supported on PAGE_SIZE(%d) != %d\n" $PAGE_SIZE
> $F2FS_BLKSIZE
> + exit 77
> +fi

This confusing to me. You are skipping the test when

    PAGE_SIZE > F2FS_BLKSIZE

but the corresponding message says

    PAGE_SIZE != F2FS_BLKSIZE

Now... which condition is it supposed to be? ">" or "!="?

I know from the Linux kernel's ext2 driver that it is very well
possible that PAGE_SIZE != EXT2_BLOCK_SIZE can work
unless EXT2_BLOCK_SIZE > PAGE_SIZE.

So ">" and "!=" are not necessarily the same thing, and IMHO the check
and the message use the same condition.

Uli

Re: [PATCH v2] tests: Don't run f2fs test on systems with PAGE_SIZE more than 4096 bytes

[Hans Ulrich Niedermann]

On Wed, 29 Apr 2020 19:25:57 +0200
Hans Ulrich Niedermann <hun@n-dimensional.de> wrote:

> On Wed, 29 Apr 2020 17:06:36 +0300
> Anatoly Pugachev <matorola@gmail.com> wrote:
> 
> > Don't run f2fs test on systems with PAGE_SIZE > 4096 bytes, since
> > f2fs is not supported on these systems and trying to mount a f2fs
> > filesystem would fail.  
> 
> "Skip the f2fs test on ..." might be better wording, both in this
> paragraph and the Subject.
> 
> Exit code 77 is certainly documented with the word "skip", and "exit
> 77" will show up in the "make check" output as "SKIP" as well.
> 
> > v2 changes:
> > 
> > - fix compare
> > - quotes around variable expansion
> > 
> > Signed-off-by: Anatoly Pugachev <matorola@gmail.com>
> > Reviewed-by: Mike Gilbert <floppym@gentoo.org>
> > 
> > ---
> >  tests/f2fs_test.in | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/f2fs_test.in b/tests/f2fs_test.in
> > index 1ea77c826..3da1dad57 100644
> > --- a/tests/f2fs_test.in
> > +++ b/tests/f2fs_test.in
> > @@ -15,5 +15,11 @@ if ! which mkfs.f2fs >/dev/null 2>&1; then
> >   exit 77
> >  fi
> >  
> > +PAGE_SIZE=$(getconf PAGE_SIZE)
> > +F2FS_BLKSIZE=4096
> > +if [ "$PAGE_SIZE" -gt "$F2FS_BLKSIZE" ]; then
> > + printf "f2fs is not supported on PAGE_SIZE(%d) != %d\n" $PAGE_SIZE
> > $F2FS_BLKSIZE
> > + exit 77
> > +fi  
> 
> This confusing to me. You are skipping the test when
> 
>     PAGE_SIZE > F2FS_BLKSIZE
> 
> but the corresponding message says
> 
>     PAGE_SIZE != F2FS_BLKSIZE
> 
> Now... which condition is it supposed to be? ">" or "!="?
> 
> I know from the Linux kernel's ext2 driver that it is very well
> possible that PAGE_SIZE != EXT2_BLOCK_SIZE can work
> unless EXT2_BLOCK_SIZE > PAGE_SIZE.
> 
> So ">" and "!=" are not necessarily the same thing, and IMHO the check
> and the message use the same condition.

The commit title and the commit message are also talking about
"PAGE_SIZE > 4096" instead of "PAGE_SIZE != 4096".

This adds more confusion over whether F2FS_BLKSIZE just happens to be
4096 somewhere where PAGE_SIZE is 4096, whether F2FS_BLKSIZE is defined
to be 4096 everywhere, and whether the whole thing is about the number
4096 or about the value of F2FS_BLKSIZE.

(Again, in the ext2 example, filesystems can have many block sizes, but
the default is the Linux PAGE_SIZE, so all x86 PCs will create
filesystems with block size 4096 but a MIPS system like the Western
Digital SOHO external HDD NAS will create a filesystem with block
size 65536. The kernel driver just needs PAGE_SIZE >= block size to
work, so a MIPS created ext2 filesystem will not mount on a x86 PC.)

Mentioning the same condition in commit title, commit message, the
actual "if" branch, and then the "printf" message would certainly reduce
my confusion.

Uli