[PATCH 0/3] LTP random fixes for xfs and btrfs

[PATCH 0/3] LTP random fixes for xfs and btrfs

[Zorro Lang]

[PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

  It doesn't skip filesystems as its plan, fix it.

[PATCH 2/3] stat04+lstat03: fix bad blocksize mkfs option for xfs

  mkfs.xfs doesn't support "-b 1024", needs "-b size=1024"

[PATCH 3/3] stat04+lstat03: skip test on btrfs

  The "-b" option of mkfs.btrfs isn't a blocksize option, there's not blocksize
  option in mkfs.btrfs. So I'd like to skip this test for btrfs. But I'm not
  sure if there's better way, so CC *btrfs list* to get more review points for
  that.
  (BTW, better to have a common helper to deal with different filesystems'
   blocksize options in the future)

[PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Zorro Lang]

This test need to skip test known filesystems, but according to below
code logic (in lib/tst_test.c):

  if (!tst_test->all_filesystems && tst_test->skip_filesystems) {
        long fs_type = tst_fs_type(".");
        const char *fs_name = tst_fs_type_name(fs_type);

        if (tst_fs_in_skiplist(fs_name, tst_test->skip_filesystems)) {
            tst_brk(TCONF, "%s is not supported by the test",
            fs_name);
        }

        tst_res(TINFO, "%s is supported by the test", fs_name);
  }

if all_filesystems is 1, the skip_filesystems doesn't work. So set
all_filesystems to 0.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 testcases/kernel/syscalls/ioctl/ioctl_ficlone02.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlone02.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlone02.c
index fab0daaee..b7f676ec7 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_ficlone02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlone02.c
@@ -57,7 +57,7 @@ static struct tst_test test = {
 	.needs_root = 1,
 	.mount_device = 1,
 	.mntpoint = MNTPOINT,
-	.all_filesystems = 1,
+	.all_filesystems = 0,
 	.skip_filesystems = (const char *[]) {
 		"bcachefs",
 		"btrfs",
-- 
2.45.2

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Cyril Hrubis]

Hi!
> This test need to skip test known filesystems, but according to below
> code logic (in lib/tst_test.c):
> 
>   if (!tst_test->all_filesystems && tst_test->skip_filesystems) {
>         long fs_type = tst_fs_type(".");
>         const char *fs_name = tst_fs_type_name(fs_type);
> 
>         if (tst_fs_in_skiplist(fs_name, tst_test->skip_filesystems)) {
>             tst_brk(TCONF, "%s is not supported by the test",
>             fs_name);
>         }
>
>         tst_res(TINFO, "%s is supported by the test", fs_name);
>   }
> 
> if all_filesystems is 1, the skip_filesystems doesn't work. So set
> all_filesystems to 0.

The code to skip filesystems in the case of all filesystems is in the
run_tcase_per_fs() function:

static int run_tcases_per_fs(void)
{
        int ret = 0;
        unsigned int i;
        const char *const *filesystems = tst_get_supported_fs_types(tst_test->skip_filesystems);

The skip_filesystems array is passed to the tst_get_supporte_fs_types()
function which filters out them.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Cyril Hrubis]

Hi!
> The code to skip filesystems in the case of all filesystems is in the
> run_tcase_per_fs() function:
> 
> static int run_tcases_per_fs(void)
> {
>         int ret = 0;
>         unsigned int i;
>         const char *const *filesystems = tst_get_supported_fs_types(tst_test->skip_filesystems);
> 
> The skip_filesystems array is passed to the tst_get_supporte_fs_types()
> function which filters out them.

Perhaps you mean that the skiplist does not work with .all_filesystems
_and_ the LTP_SINGLE_FS_TYPE environment variable?

I guess that we need:

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index bbbb8df19..49b1d7205 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -159,6 +159,10 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)

        if (only_fs) {
                tst_res(TINFO, "WARNING: testing only %s", only_fs);
+
+               if (tst_fs_in_skiplist(only_fs, skiplist))
+                       tst_brk(TCONF, "Requested filesystems is in test skiplist");
+
                if (tst_fs_is_supported(only_fs))
                        fs_types[0] = only_fs;
                return fs_types;


-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Petr Vorel]

> Hi!
> > The code to skip filesystems in the case of all filesystems is in the
> > run_tcase_per_fs() function:

> > static int run_tcases_per_fs(void)
> > {
> >         int ret = 0;
> >         unsigned int i;
> >         const char *const *filesystems = tst_get_supported_fs_types(tst_test->skip_filesystems);

> > The skip_filesystems array is passed to the tst_get_supporte_fs_types()
> > function which filters out them.

> Perhaps you mean that the skiplist does not work with .all_filesystems
> _and_ the LTP_SINGLE_FS_TYPE environment variable?

> I guess that we need:

> diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> index bbbb8df19..49b1d7205 100644
> --- a/lib/tst_supported_fs_types.c
> +++ b/lib/tst_supported_fs_types.c
> @@ -159,6 +159,10 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)

>         if (only_fs) {
>                 tst_res(TINFO, "WARNING: testing only %s", only_fs);
> +
> +               if (tst_fs_in_skiplist(only_fs, skiplist))
> +                       tst_brk(TCONF, "Requested filesystems is in test skiplist");
> +

It's a nice feature to be able to force testing on filesystem even it's set to
be skipped without need to manually enable the filesystem and recompile.
(It helps testing with LTP compiled as a package without need to compile LTP.)
Therefore I would avoid this.

@Zorro Lang or are you testing whole syscalls on particular filesystem via
LTP_SINGLE_FS_TYPE=xfs ?

Kind regards,
Petr

>                 if (tst_fs_is_supported(only_fs))
>                         fs_types[0] = only_fs;
>                 return fs_types;

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Cyril Hrubis]

Hi!
> It's a nice feature to be able to force testing on filesystem even it's set to
> be skipped without need to manually enable the filesystem and recompile.
> (It helps testing with LTP compiled as a package without need to compile LTP.)
> Therefore I would avoid this.

I guess that this should be another env variable e.g.
LTP_FORCE_SINGLE_FS_TYPE and it should print a big fat warning that it's
only for development purposes.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Petr Vorel]

> Hi!
> > It's a nice feature to be able to force testing on filesystem even it's set to
> > be skipped without need to manually enable the filesystem and recompile.
> > (It helps testing with LTP compiled as a package without need to compile LTP.)
> > Therefore I would avoid this.

> I guess that this should be another env variable e.g.
> LTP_FORCE_SINGLE_FS_TYPE and it should print a big fat warning that it's
> only for development purposes.

Well, LTP_SINGLE_FS_TYPE already has "Testing only" and that was the reason it
just forced filesystem regardless "skip" setup. Sure, we can turn it into
"normal" variable and introduce LTP_FORCE_SINGLE_FS_TYPE if it's needed.
But that would be an use case if anybody uses LTP really to test particular
filesystem. And it would affect only .all_filesystem tests (e.g. user's
responsibility would be to point TMPDIR to that particular system on non-
.all_filesystem tests).

@Zorro was it meant like this?

Kind regards,
Petr

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Cyril Hrubis]

Hi!
> > > It's a nice feature to be able to force testing on filesystem even it's set to
> > > be skipped without need to manually enable the filesystem and recompile.
> > > (It helps testing with LTP compiled as a package without need to compile LTP.)
> > > Therefore I would avoid this.
> 
> > I guess that this should be another env variable e.g.
> > LTP_FORCE_SINGLE_FS_TYPE and it should print a big fat warning that it's
> > only for development purposes.
> 
> Well, LTP_SINGLE_FS_TYPE already has "Testing only"

That was maybe how we intended it but we exposed the API and it seems
that there is a usecase for it so people are possibly using it.

> and that was the reason it just forced filesystem regardless "skip"
> setup. Sure, we can turn it into "normal" variable and introduce
> LTP_FORCE_SINGLE_FS_TYPE if it's needed.  But that would be an use
> case if anybody uses LTP really to test particular filesystem. And it
> would affect only .all_filesystem tests (e.g. user's responsibility
> would be to point TMPDIR to that particular system on non-
> .all_filesystem tests).

There is a usecase we haven't figured out, when you want to test a
single filesystem, you do not want to waste time on the rest of the
filesystems and LTP_SINGlE_FS_TYPE nearly works for that usecase.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Petr Vorel]

> Hi!
> > > > It's a nice feature to be able to force testing on filesystem even it's set to
> > > > be skipped without need to manually enable the filesystem and recompile.
> > > > (It helps testing with LTP compiled as a package without need to compile LTP.)
> > > > Therefore I would avoid this.

> > > I guess that this should be another env variable e.g.
> > > LTP_FORCE_SINGLE_FS_TYPE and it should print a big fat warning that it's
> > > only for development purposes.

> > Well, LTP_SINGLE_FS_TYPE already has "Testing only"

> That was maybe how we intended it but we exposed the API and it seems
> that there is a usecase for it so people are possibly using it.

> > and that was the reason it just forced filesystem regardless "skip"
> > setup. Sure, we can turn it into "normal" variable and introduce
> > LTP_FORCE_SINGLE_FS_TYPE if it's needed.  But that would be an use
> > case if anybody uses LTP really to test particular filesystem. And it
> > would affect only .all_filesystem tests (e.g. user's responsibility
> > would be to point TMPDIR to that particular system on non-
> > .all_filesystem tests).

> There is a usecase we haven't figured out, when you want to test a
> single filesystem, you do not want to waste time on the rest of the
> filesystems and LTP_SINGlE_FS_TYPE nearly works for that usecase.

Yes, I understood this use case, this was probably the reason of this PR.
I'd appreciate Zorro's confirmation we understood him properly (e.g. that
somebody really needs this). I suppose you vote for this feature anyway
and it's fairly simple, thus I can prepare it.

Kind regards,
Petr

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Zorro Lang]

On Mon, Dec 02, 2024 at 03:42:08PM +0100, Petr Vorel wrote:
> > Hi!
> > > The code to skip filesystems in the case of all filesystems is in the
> > > run_tcase_per_fs() function:
> 
> > > static int run_tcases_per_fs(void)
> > > {
> > >         int ret = 0;
> > >         unsigned int i;
> > >         const char *const *filesystems = tst_get_supported_fs_types(tst_test->skip_filesystems);
> 
> > > The skip_filesystems array is passed to the tst_get_supporte_fs_types()
> > > function which filters out them.
> 
> > Perhaps you mean that the skiplist does not work with .all_filesystems
> > _and_ the LTP_SINGLE_FS_TYPE environment variable?
> 
> > I guess that we need:
> 
> > diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> > index bbbb8df19..49b1d7205 100644
> > --- a/lib/tst_supported_fs_types.c
> > +++ b/lib/tst_supported_fs_types.c
> > @@ -159,6 +159,10 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)
> 
> >         if (only_fs) {
> >                 tst_res(TINFO, "WARNING: testing only %s", only_fs);
> > +
> > +               if (tst_fs_in_skiplist(only_fs, skiplist))
> > +                       tst_brk(TCONF, "Requested filesystems is in test skiplist");
> > +
> 
> It's a nice feature to be able to force testing on filesystem even it's set to
> be skipped without need to manually enable the filesystem and recompile.
> (It helps testing with LTP compiled as a package without need to compile LTP.)
> Therefore I would avoid this.
> 
> @Zorro Lang or are you testing whole syscalls on particular filesystem via
> LTP_SINGLE_FS_TYPE=xfs ?

Oh, yes, I always use LTP with different LTP_SINGLE_FS_TYPE. So that's might be
the problem?

Thanks,
Zorro

> 
> Kind regards,
> Petr
> 
> >                 if (tst_fs_is_supported(only_fs))
> >                         fs_types[0] = only_fs;
> >                 return fs_types;
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
> 

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Petr Vorel]

> On Mon, Dec 02, 2024 at 03:42:08PM +0100, Petr Vorel wrote:
> > > Hi!
> > > > The code to skip filesystems in the case of all filesystems is in the
> > > > run_tcase_per_fs() function:

> > > > static int run_tcases_per_fs(void)
> > > > {
> > > >         int ret = 0;
> > > >         unsigned int i;
> > > >         const char *const *filesystems = tst_get_supported_fs_types(tst_test->skip_filesystems);

> > > > The skip_filesystems array is passed to the tst_get_supporte_fs_types()
> > > > function which filters out them.

> > > Perhaps you mean that the skiplist does not work with .all_filesystems
> > > _and_ the LTP_SINGLE_FS_TYPE environment variable?

> > > I guess that we need:

> > > diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> > > index bbbb8df19..49b1d7205 100644
> > > --- a/lib/tst_supported_fs_types.c
> > > +++ b/lib/tst_supported_fs_types.c
> > > @@ -159,6 +159,10 @@ const char **tst_get_supported_fs_types(const char *const *skiplist)

> > >         if (only_fs) {
> > >                 tst_res(TINFO, "WARNING: testing only %s", only_fs);
> > > +
> > > +               if (tst_fs_in_skiplist(only_fs, skiplist))
> > > +                       tst_brk(TCONF, "Requested filesystems is in test skiplist");
> > > +

> > It's a nice feature to be able to force testing on filesystem even it's set to
> > be skipped without need to manually enable the filesystem and recompile.
> > (It helps testing with LTP compiled as a package without need to compile LTP.)
> > Therefore I would avoid this.

> > @Zorro Lang or are you testing whole syscalls on particular filesystem via
> > LTP_SINGLE_FS_TYPE=xfs ?

> Oh, yes, I always use LTP with different LTP_SINGLE_FS_TYPE. So that's might be
> the problem?

Thanks for confirming your use case.

Well, "Testing only" in the help (-h) was added there to suggest it's for
testing/debugging LTP, not a production testing. But newer mind, I'll implement
Cyril's suggestion, real usage justify it. + I'll add LTP_FORCE_SINGLE_FS_TYPE.

We could allow more filesystems, e.g.  instead of running LTP few times with
different LTP_SINGLE_FS_TYPE value: e.g.

for fs in ext4 xfs btrfs; do LTP_SINGLE_FS_TYPE=fs ioctl_ficlone02; done

we could introduce support for particular filesystems
LTP_FILESYSTEMS="ext4,xfs,btrfs" ioctl_ficlone02

(Probably define new variable because "SINGLE" is misleading when supporting
more filesystems. Also when we touch it, I would consider renaming variable
FILESYSTEMS is more obvious for newcomers than "FS_TYPE".)

WDYT?

Kind regards,
Petr

> Thanks,
> Zorro

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Cyril Hrubis]

Hi!
> Well, "Testing only" in the help (-h) was added there to suggest it's for
> testing/debugging LTP, not a production testing. But newer mind, I'll implement
> Cyril's suggestion, real usage justify it. + I'll add LTP_FORCE_SINGLE_FS_TYPE.
> 
> We could allow more filesystems, e.g.  instead of running LTP few times with
> different LTP_SINGLE_FS_TYPE value: e.g.
> 
> for fs in ext4 xfs btrfs; do LTP_SINGLE_FS_TYPE=fs ioctl_ficlone02; done
> 
> we could introduce support for particular filesystems
> LTP_FILESYSTEMS="ext4,xfs,btrfs" ioctl_ficlone02

That is stil not equivalent if you run it with a whole LTP. We would
have to run each test that uses device for all LTP_FILESYSTEMS, since
many of the testcases does use device but does not use .all_filesystems.

So all in all I think that LTP_SINGLE_FS_TYPE is good enough solution.

Or we can try to rething the whole thing, but it's getting quite
complicated with all the options we have.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [LTP] [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero

[Petr Vorel]

> Hi!
> > Well, "Testing only" in the help (-h) was added there to suggest it's for
> > testing/debugging LTP, not a production testing. But newer mind, I'll implement
> > Cyril's suggestion, real usage justify it. + I'll add LTP_FORCE_SINGLE_FS_TYPE.

> > We could allow more filesystems, e.g.  instead of running LTP few times with
> > different LTP_SINGLE_FS_TYPE value: e.g.

> > for fs in ext4 xfs btrfs; do LTP_SINGLE_FS_TYPE=fs ioctl_ficlone02; done

> > we could introduce support for particular filesystems
> > LTP_FILESYSTEMS="ext4,xfs,btrfs" ioctl_ficlone02

> That is stil not equivalent if you run it with a whole LTP. We would
> have to run each test that uses device for all LTP_FILESYSTEMS, since
> many of the testcases does use device but does not use .all_filesystems.

> So all in all I think that LTP_SINGLE_FS_TYPE is good enough solution.

> Or we can try to rething the whole thing, but it's getting quite
> complicated with all the options we have.

I guess LTP_SINGLE_FS_TYPE + LTP_FORCE_SINGLE_FS_TYPE LGTM.

What bothers me more how much time we spent with formatting loop device (done
for each test with .all_filesystems several times). Running all tests on single
filesystem, then reformat and run all of them on the other filesystem would be
faster. The only thing which is better on this is theoretical chance that
filesystem gets corrupted by some test, then other tests would be somehow
influenced.

Kind regards,
Petr

[PATCH 2/3] stat04+lstat03: fix bad blocksize mkfs option for xfs

[Zorro Lang]

Not all filesystems use "-b 1024" to set its blocksize. XFS uses
"-b size=1024", so this test fails as "unknown option -b 1024" on
xfs.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 testcases/kernel/syscalls/lstat/lstat03.c | 8 ++++++--
 testcases/kernel/syscalls/stat/stat04.c   | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/lstat/lstat03.c b/testcases/kernel/syscalls/lstat/lstat03.c
index d48af180b..675fb56f4 100644
--- a/testcases/kernel/syscalls/lstat/lstat03.c
+++ b/testcases/kernel/syscalls/lstat/lstat03.c
@@ -44,8 +44,9 @@ static void run(void)
 
 static void setup(void)
 {
+	char *opt_name="-b";
 	char opt_bsize[32];
-	const char *const fs_opts[] = {opt_bsize, NULL};
+	const char *const fs_opts[] = {opt_name, opt_bsize, NULL};
 	struct stat sb;
 	int pagesize;
 	int fd;
@@ -54,7 +55,10 @@ static void setup(void)
 	SAFE_STAT(".", &sb);
 	pagesize = sb.st_blksize == 4096 ? 1024 : 4096;
 
-	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", pagesize);
+	if (strcmp(tst_device->fs_type, "xfs") == 0)
+		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
+	else
+		snprintf(opt_bsize, sizeof(opt_bsize), "%i", pagesize);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
 	SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, 0);
 
diff --git a/testcases/kernel/syscalls/stat/stat04.c b/testcases/kernel/syscalls/stat/stat04.c
index 05ace606a..2a17cc7d7 100644
--- a/testcases/kernel/syscalls/stat/stat04.c
+++ b/testcases/kernel/syscalls/stat/stat04.c
@@ -43,8 +43,9 @@ static void run(void)
 
 static void setup(void)
 {
+	char *opt_name="-b";
 	char opt_bsize[32];
-	const char *const fs_opts[] = {opt_bsize, NULL};
+	const char *const fs_opts[] = {opt_name, opt_bsize, NULL};
 	struct stat sb;
 	int pagesize;
 	int fd;
@@ -56,7 +57,10 @@ static void setup(void)
 	SAFE_STAT(".", &sb);
 	pagesize = sb.st_blksize == 4096 ? 1024 : 4096;
 
-	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", pagesize);
+	if (strcmp(tst_device->fs_type, "xfs") == 0)
+		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
+	else
+		snprintf(opt_bsize, sizeof(opt_bsize), "%i", pagesize);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
 	SAFE_MOUNT(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, 0);
 
-- 
2.45.2

Re: [LTP] [PATCH 2/3] stat04+lstat03: fix bad blocksize mkfs option for xfs

[Cyril Hrubis]

Hi!
> Not all filesystems use "-b 1024" to set its blocksize. XFS uses
> "-b size=1024", so this test fails as "unknown option -b 1024" on
> xfs.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  testcases/kernel/syscalls/lstat/lstat03.c | 8 ++++++--
>  testcases/kernel/syscalls/stat/stat04.c   | 8 ++++++--
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/lstat/lstat03.c b/testcases/kernel/syscalls/lstat/lstat03.c
> index d48af180b..675fb56f4 100644
> --- a/testcases/kernel/syscalls/lstat/lstat03.c
> +++ b/testcases/kernel/syscalls/lstat/lstat03.c
> @@ -44,8 +44,9 @@ static void run(void)
>  
>  static void setup(void)
>  {
> +	char *opt_name="-b";
>  	char opt_bsize[32];
> -	const char *const fs_opts[] = {opt_bsize, NULL};
> +	const char *const fs_opts[] = {opt_name, opt_bsize, NULL};
                                       ^
				       You can just add "-b" here
				       instead of creating a variable.

>  	struct stat sb;
>  	int pagesize;
>  	int fd;
> @@ -54,7 +55,10 @@ static void setup(void)
>  	SAFE_STAT(".", &sb);
>  	pagesize = sb.st_blksize == 4096 ? 1024 : 4096;
>  
> -	snprintf(opt_bsize, sizeof(opt_bsize), "-b %i", pagesize);
> +	if (strcmp(tst_device->fs_type, "xfs") == 0)
          ^
	  The more common style is if (!strcmp(...))
> +		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
> +	else
> +		snprintf(opt_bsize, sizeof(opt_bsize), "%i", pagesize);

Otherwise it looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

[PATCH 3/3] stat04+lstat03: skip test on btrfs

[Zorro Lang]

The "-b" option for mkfs.btrfs isn't a blocksize option, it does
"specify the size of each device as seen by the filesystem" for
btrfs. There's not an blocksize mkfs option for btrfs, so skip this
test.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 testcases/kernel/syscalls/lstat/lstat03.c | 2 ++
 testcases/kernel/syscalls/stat/stat04.c   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/testcases/kernel/syscalls/lstat/lstat03.c b/testcases/kernel/syscalls/lstat/lstat03.c
index 675fb56f4..f7346893d 100644
--- a/testcases/kernel/syscalls/lstat/lstat03.c
+++ b/testcases/kernel/syscalls/lstat/lstat03.c
@@ -57,6 +57,8 @@ static void setup(void)
 
 	if (strcmp(tst_device->fs_type, "xfs") == 0)
 		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
+	else if (strcmp(tst_device->fs_type, "btrfs") == 0)
+		tst_brk(TCONF, "btrfs is not supported");
 	else
 		snprintf(opt_bsize, sizeof(opt_bsize), "%i", pagesize);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
diff --git a/testcases/kernel/syscalls/stat/stat04.c b/testcases/kernel/syscalls/stat/stat04.c
index 2a17cc7d7..3c4f1a6b4 100644
--- a/testcases/kernel/syscalls/stat/stat04.c
+++ b/testcases/kernel/syscalls/stat/stat04.c
@@ -59,6 +59,8 @@ static void setup(void)
 
 	if (strcmp(tst_device->fs_type, "xfs") == 0)
 		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
+	else if (strcmp(tst_device->fs_type, "btrfs") == 0)
+		tst_brk(TCONF, "btrfs is not supported");
 	else
 		snprintf(opt_bsize, sizeof(opt_bsize), "%i", pagesize);
 	SAFE_MKFS(tst_device->dev, tst_device->fs_type, fs_opts, NULL);
-- 
2.45.2

Re: [LTP] [PATCH 3/3] stat04+lstat03: skip test on btrfs

[Cyril Hrubis]

Hi!
> The "-b" option for mkfs.btrfs isn't a blocksize option, it does
> "specify the size of each device as seen by the filesystem" for
> btrfs. There's not an blocksize mkfs option for btrfs, so skip this
> test.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  testcases/kernel/syscalls/lstat/lstat03.c | 2 ++
>  testcases/kernel/syscalls/stat/stat04.c   | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/testcases/kernel/syscalls/lstat/lstat03.c b/testcases/kernel/syscalls/lstat/lstat03.c
> index 675fb56f4..f7346893d 100644
> --- a/testcases/kernel/syscalls/lstat/lstat03.c
> +++ b/testcases/kernel/syscalls/lstat/lstat03.c
> @@ -57,6 +57,8 @@ static void setup(void)
>  
>  	if (strcmp(tst_device->fs_type, "xfs") == 0)
>  		snprintf(opt_bsize, sizeof(opt_bsize), "size=%i", pagesize);
> +	else if (strcmp(tst_device->fs_type, "btrfs") == 0)
> +		tst_brk(TCONF, "btrfs is not supported");

This is overkill, all we need to skip on Btrfs is the st_blksize test.
We need to set a flag in the test setup and then skip the corresponding
TST_EXP_EXPR().

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 0/3] LTP random fixes for xfs and btrfs

[Qu Wenruo]

在 2024/12/1 20:06, Zorro Lang 写道:
> [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero
>
>    It doesn't skip filesystems as its plan, fix it.
>
> [PATCH 2/3] stat04+lstat03: fix bad blocksize mkfs option for xfs
>
>    mkfs.xfs doesn't support "-b 1024", needs "-b size=1024"
>
> [PATCH 3/3] stat04+lstat03: skip test on btrfs
>
>    The "-b" option of mkfs.btrfs isn't a blocksize option, there's not blocksize
>    option in mkfs.btrfs. So I'd like to skip this test for btrfs. But I'm not
>    sure if there's better way, so CC *btrfs list* to get more review points for
>    that.
>    (BTW, better to have a common helper to deal with different filesystems'
>     blocksize options in the future)
>

Well, I'd say Wilcox is kinda correct here.

Btrfs uses the name "sector size" to indicate the minimal unit, aka, the
blocksize of all the other fses.

Not sure if we will even rename the whole sector size to block size in
the future, it looks like a huge work to do.


However there is another problem related to the btrfs block size (aka,
"sector size").
Our block size starts at 4K, ends at 64K, and we do not yet support
block size > page size.

For systems with 64K page size, although we have the support for block
size < page size, I have added an artificial limits, that only 4K block
size and page size is supported.


So even if we added btrfs block size support, it won't really work
except 4K and page size (at least for now).

Thanks,
Qu

Re: [PATCH 0/3] LTP random fixes for xfs and btrfs

[David Sterba]

On Sun, Dec 01, 2024 at 08:25:19PM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/12/1 20:06, Zorro Lang 写道:
> > [PATCH 1/3] ioctl_ficlone02.c: set all_filesystems to zero
> >
> >    It doesn't skip filesystems as its plan, fix it.
> >
> > [PATCH 2/3] stat04+lstat03: fix bad blocksize mkfs option for xfs
> >
> >    mkfs.xfs doesn't support "-b 1024", needs "-b size=1024"
> >
> > [PATCH 3/3] stat04+lstat03: skip test on btrfs
> >
> >    The "-b" option of mkfs.btrfs isn't a blocksize option, there's not blocksize
> >    option in mkfs.btrfs. So I'd like to skip this test for btrfs. But I'm not
> >    sure if there's better way, so CC *btrfs list* to get more review points for
> >    that.
> >    (BTW, better to have a common helper to deal with different filesystems'
> >     blocksize options in the future)
> >
> 
> Well, I'd say Wilcox is kinda correct here.
> 
> Btrfs uses the name "sector size" to indicate the minimal unit, aka, the
> blocksize of all the other fses.
> 
> Not sure if we will even rename the whole sector size to block size in
> the future, it looks like a huge work to do.

Well, I think we can at least add an alias blocksize to sectorsize to
mkfs.  We don't have a time machine to change the initial confusing
naming, but can slightly improve the user convenience.  Internally in
the code we can keep sectorsize or incrementally rename it to blocksize.
What matters more here is the user intrface, i.e. the mkfs options.