[LTP] [PATCH] statx04: Re-add BTRFS version check

[LTP] [PATCH] statx04: Re-add BTRFS version check

[Richard Palethorpe via ltp]

Removing this was a step too far. This causes regressions on products
where there is now no chance of a backport. This is different from the
other version checks which are for much newer kernels. Also there
could be differences in the difficulty of a backport.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Cc: Yang Xu <xuyang2018.jy@fujitsu.com>
---

Note that I am still very much against new version checks if there is
a high chance of backports. We should leave long established checks
alone however.

 testcases/kernel/syscalls/statx/statx04.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c
index 180c61bf9..b5ca0586a 100644
--- a/testcases/kernel/syscalls/statx/statx04.c
+++ b/testcases/kernel/syscalls/statx/statx04.c
@@ -182,6 +182,9 @@ static void caid_flags_setup(void)
 
 static void setup(void)
 {
+	if (!strcmp(tst_device->fs_type, "btrfs") && tst_kvercmp(4, 13, 0) < 0)
+		tst_brk(TCONF, "Btrfs statx() supported since 4.13");
+
 	SAFE_MKDIR(TESTDIR_FLAGGED, 0777);
 	SAFE_MKDIR(TESTDIR_UNFLAGGED, 0777);
 
-- 
2.33.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] statx04: Re-add BTRFS version check

[xuyang2018.jy]

Hi Richard
> Removing this was a step too far. This causes regressions on products
> where there is now no chance of a backport.

Do you mean that your distribution based on older kernel ie 4.11
supports statx syscall but btrfs missed the btrfs patch? Also this
distribution doesn't update and so have no choice to backport.
> This is different from the
> other version checks which are for much newer kernels. 
IMO, distribution based on older kernel 4.11 still can make ext2 ext4
xfs supports statx because the backport looks not diffcult. So, I don't
think this is a difference. It depends on kernel users worked on this
distirbution whether have this requirement.
Also there could be differences in the difficulty of a backport.
I see xfs/btrfs code, it only fills the attributes field of stat
struture by parsing inode flags.

If you must add this check on suse distribution, I guess you just add
this version check for suse distribution. For centos7,8, neither of them
supports btrfs, but I don't know other distribution situation ie unbuntu.

Maybe you can just add suse detection in lib/tst_kvercmp.c.

Just my personal thought.

Best Regards
Yang Xu

> 
> Signed-off-by: Richard Palethorpe<rpalethorpe@suse.com>
> Cc: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
> 
> Note that I am still very much against new version checks if there is
> a high chance of backports. We should leave long established checks
> alone however.
> 
>   testcases/kernel/syscalls/statx/statx04.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/testcases/kernel/syscalls/statx/statx04.c b/testcases/kernel/syscalls/statx/statx04.c
> index 180c61bf9..b5ca0586a 100644
> --- a/testcases/kernel/syscalls/statx/statx04.c
> +++ b/testcases/kernel/syscalls/statx/statx04.c
> @@ -182,6 +182,9 @@ static void caid_flags_setup(void)
> 
>   static void setup(void)
>   {
> +	if (!strcmp(tst_device->fs_type, "btrfs")&&  tst_kvercmp(4, 13, 0)<  0)
> +		tst_brk(TCONF, "Btrfs statx() supported since 4.13");
> +
>   	SAFE_MKDIR(TESTDIR_FLAGGED, 0777);
>   	SAFE_MKDIR(TESTDIR_UNFLAGGED, 0777);
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] statx04: Re-add BTRFS version check

[Cyril Hrubis]

Hi!
> Do you mean that your distribution based on older kernel ie 4.11
> supports statx syscall but btrfs missed the btrfs patch? Also this
> distribution doesn't update and so have no choice to backport.
> > This is different from the
> > other version checks which are for much newer kernels. 
> IMO, distribution based on older kernel 4.11 still can make ext2 ext4
> xfs supports statx because the backport looks not diffcult. So, I don't
> think this is a difference. It depends on kernel users worked on this
> distirbution whether have this requirement.
> Also there could be differences in the difficulty of a backport.
> I see xfs/btrfs code, it only fills the attributes field of stat
> struture by parsing inode flags.
> 
> If you must add this check on suse distribution, I guess you just add
> this version check for suse distribution. For centos7,8, neither of them
> supports btrfs, but I don't know other distribution situation ie unbuntu.

I just checked debian, both oldstable (4.16) and stable (5.10) have new
enough kernels for this not to matter.

> Maybe you can just add suse detection in lib/tst_kvercmp.c.

I guess that this would be the cleanest solution.

Actually SUSE should be detected just fine, since we parse
/etc/os-release for ID='foo' in the test library.

So this could be solved just by defining:

static struct tst_kern_exv kvers[] = {
	{"sles", "4.13.0"}
	{}
};

and then doing:

	if (tst_kvercmp2(0, 0, 0, kvers) < 0)
		tst_brk(TCONF, "Btrfs statx() supported since 4.13");


Also it would be a bit cleaner to add this to the tst_test structure as
.min_kver_ex as well, but that's a different story...

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] statx04: Re-add BTRFS version check

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> Do you mean that your distribution based on older kernel ie 4.11
>> supports statx syscall but btrfs missed the btrfs patch? Also this
>> distribution doesn't update and so have no choice to backport.
>> > This is different from the
>> > other version checks which are for much newer kernels. 
>> IMO, distribution based on older kernel 4.11 still can make ext2 ext4
>> xfs supports statx because the backport looks not diffcult. So, I don't
>> think this is a difference. It depends on kernel users worked on this
>> distirbution whether have this requirement.
>> Also there could be differences in the difficulty of a backport.
>> I see xfs/btrfs code, it only fills the attributes field of stat
>> struture by parsing inode flags.
>> 
>> If you must add this check on suse distribution, I guess you just add
>> this version check for suse distribution. For centos7,8, neither of them
>> supports btrfs, but I don't know other distribution situation ie unbuntu.
>
> I just checked debian, both oldstable (4.16) and stable (5.10) have new
> enough kernels for this not to matter.
>
>> Maybe you can just add suse detection in lib/tst_kvercmp.c.
>
> I guess that this would be the cleanest solution.
>
> Actually SUSE should be detected just fine, since we parse
> /etc/os-release for ID='foo' in the test library.
>
> So this could be solved just by defining:
>
> static struct tst_kern_exv kvers[] = {
> 	{"sles", "4.13.0"}
> 	{}
> };
>
> and then doing:
>
> 	if (tst_kvercmp2(0, 0, 0, kvers) < 0)
> 		tst_brk(TCONF, "Btrfs statx() supported since 4.13");
>
>
> Also it would be a bit cleaner to add this to the tst_test structure as
> .min_kver_ex as well, but that's a different story...

After some more internal discussions. We can just filter statx04 and use
the new test which performs the feature checks. I think it would have
been better to add the checks to statx04 and add a new test without any
checks. However it is done now.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] statx04: Re-add BTRFS version check

[Cyril Hrubis]

Hi!
> After some more internal discussions. We can just filter statx04 and use
> the new test which performs the feature checks. I think it would have
> been better to add the checks to statx04 and add a new test without any
> checks. However it is done now.

We never promised stable test names in LTP to begin with.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp