[LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

reflink has been introduced in XFS by kernel 4.9 and mkfs.xfs enabled
by default in 5.1.0. Check the mkfs.xfs version in order to make sure
that mkfs.xfs supports reflink and verify kernel is at least 4.9 when
we run the ioctl_ficlone tests on XFS.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
ioctl_ficlone tests are failing in certain circumstances. In particular,
when kernel version is lower than 4.9 and mkfs.xfs is not 5.1.0
---
 testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c      | 9 ++++++++-
 testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c      | 5 ++++-
 testcases/kernel/syscalls/ioctl/ioctl_ficlonerange01.c | 5 ++++-
 testcases/kernel/syscalls/ioctl/ioctl_ficlonerange02.c | 5 ++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c
index f5407f789..d0faf3327 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlone01.c
@@ -95,6 +95,12 @@ static void run(void)
 	SAFE_UNLINK(DSTPATH);
 }
 
+static void setup(void)
+{
+	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
+		tst_brk(TCONF, "XFS doesn't support reflink");
+}
+
 static void cleanup(void)
 {
 	if (src_fd != -1)
@@ -106,6 +112,7 @@ static void cleanup(void)
 
 static struct tst_test test = {
 	.test_all = run,
+	.setup = setup,
 	.cleanup = cleanup,
 	.min_kver = "4.5",
 	.needs_root = 1,
@@ -115,7 +122,7 @@ static struct tst_test test = {
 		{.type = "bcachefs"},
 		{.type = "btrfs"},
 		{
-			.type = "xfs",
+			.type = "xfs >= 5.1.0",
 			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
 		},
 		{}
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
index 3cc386c59..8e32ba039 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
@@ -62,6 +62,9 @@ static void setup(void)
 	int attr;
 	struct stat sb;
 
+	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
+		tst_brk(TCONF, "XFS doesn't support reflink");
+
 	rw_file = SAFE_OPEN("ok_only", O_CREAT | O_RDWR, 0640);
 	ro_file = SAFE_OPEN("rd_only", O_CREAT | O_RDONLY, 0640);
 	wo_file = SAFE_OPEN("rw_only", O_CREAT | O_WRONLY, 0640);
@@ -113,7 +116,7 @@ static struct tst_test test = {
 		{.type = "bcachefs"},
 		{.type = "btrfs"},
 		{
-			.type = "xfs",
+			.type = "xfs >= 5.1.0",
 			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
 		},
 		{}
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange01.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange01.c
index e352c513b..4128285b1 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange01.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange01.c
@@ -108,6 +108,9 @@ static void setup(void)
 {
 	struct stat sb;
 
+	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
+		tst_brk(TCONF, "XFS doesn't support reflink");
+
 	SAFE_STAT(MNTPOINT, &sb);
 
 	filesize = sb.st_blksize * CHUNKS;
@@ -148,7 +151,7 @@ static struct tst_test test = {
 		{.type = "bcachefs"},
 		{.type = "btrfs"},
 		{
-			.type = "xfs",
+			.type = "xfs >= 5.1.0",
 			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
 		},
 		{}
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange02.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange02.c
index ad36df162..d1f8d60c0 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange02.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlonerange02.c
@@ -60,6 +60,9 @@ static void setup(void)
 {
 	struct stat sb;
 
+	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
+		tst_brk(TCONF, "XFS doesn't support reflink");
+
 	SAFE_STAT(MNTPOINT, &sb);
 
 	alignment = sb.st_blksize;
@@ -85,7 +88,7 @@ static struct tst_test test = {
 		{.type = "bcachefs"},
 		{.type = "btrfs"},
 		{
-			.type = "xfs",
+			.type = "xfs >= 5.1.0",
 			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
 		},
 		{}

---
base-commit: 968e6245d93bc91723e72086a71e6bc50f495d0b
change-id: 20240924-ioctl_ficlone01_fix-88a17ef58543

Best regards,
-- 
Andrea Cervesato <andrea.cervesato@suse.com>


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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> +static void setup(void)
> +{
> +	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
> +		tst_brk(TCONF, "XFS doesn't support reflink");
> +}
> +
>  static void cleanup(void)
>  {
>  	if (src_fd != -1)
> @@ -106,6 +112,7 @@ static void cleanup(void)
>  
>  static struct tst_test test = {
>  	.test_all = run,
> +	.setup = setup,
>  	.cleanup = cleanup,
>  	.min_kver = "4.5",
>  	.needs_root = 1,
> @@ -115,7 +122,7 @@ static struct tst_test test = {
>  		{.type = "bcachefs"},
>  		{.type = "btrfs"},
>  		{
> -			.type = "xfs",
> +			.type = "xfs >= 5.1.0",
>  			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
>  		},
>  		{}
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
> index 3cc386c59..8e32ba039 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
> @@ -62,6 +62,9 @@ static void setup(void)
>  	int attr;
>  	struct stat sb;
>  
> +	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
> +		tst_brk(TCONF, "XFS doesn't support reflink");
> +
>  	rw_file = SAFE_OPEN("ok_only", O_CREAT | O_RDWR, 0640);
>  	ro_file = SAFE_OPEN("rd_only", O_CREAT | O_RDONLY, 0640);
>  	wo_file = SAFE_OPEN("rw_only", O_CREAT | O_WRONLY, 0640);
> @@ -113,7 +116,7 @@ static struct tst_test test = {
>  		{.type = "bcachefs"},
>  		{.type = "btrfs"},
>  		{
> -			.type = "xfs",
> +			.type = "xfs >= 5.1.0",

Does this even work? I suppose that we do have a minimal version syntax
for commands but not for mkfs.foo. And even for commands the version
parser needs to be implemented for each command separately. We have one
for mkfs.ext4 at the moment.

I suppose that we need to add .mkfs_ver string to the struct tst_fs and
possibly .kernel_ver as well so that we can add both checks to the
structures as:

	{
		.type = "xfs",
		.mkfs_ver = ">= 5.1.0",
		.kernel_ver = ">= 4.9.0",
		...
	}

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Andrea Cervesato via ltp]

Hi!

On 9/26/24 13:57, Cyril Hrubis wrote:
> Hi!
>> +static void setup(void)
>> +{
>> +	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
>> +		tst_brk(TCONF, "XFS doesn't support reflink");
>> +}
>> +
>>   static void cleanup(void)
>>   {
>>   	if (src_fd != -1)
>> @@ -106,6 +112,7 @@ static void cleanup(void)
>>   
>>   static struct tst_test test = {
>>   	.test_all = run,
>> +	.setup = setup,
>>   	.cleanup = cleanup,
>>   	.min_kver = "4.5",
>>   	.needs_root = 1,
>> @@ -115,7 +122,7 @@ static struct tst_test test = {
>>   		{.type = "bcachefs"},
>>   		{.type = "btrfs"},
>>   		{
>> -			.type = "xfs",
>> +			.type = "xfs >= 5.1.0",
>>   			.mkfs_opts = (const char *const []) {"-m", "reflink=1", NULL},
>>   		},
>>   		{}
>> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
>> index 3cc386c59..8e32ba039 100644
>> --- a/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
>> +++ b/testcases/kernel/syscalls/ioctl/ioctl_ficlone03.c
>> @@ -62,6 +62,9 @@ static void setup(void)
>>   	int attr;
>>   	struct stat sb;
>>   
>> +	if (!strcmp(tst_device->fs_type, "xfs") && tst_kvercmp(4, 9, 0) < 0)
>> +		tst_brk(TCONF, "XFS doesn't support reflink");
>> +
>>   	rw_file = SAFE_OPEN("ok_only", O_CREAT | O_RDWR, 0640);
>>   	ro_file = SAFE_OPEN("rd_only", O_CREAT | O_RDONLY, 0640);
>>   	wo_file = SAFE_OPEN("rw_only", O_CREAT | O_WRONLY, 0640);
>> @@ -113,7 +116,7 @@ static struct tst_test test = {
>>   		{.type = "bcachefs"},
>>   		{.type = "btrfs"},
>>   		{
>> -			.type = "xfs",
>> +			.type = "xfs >= 5.1.0",
> Does this even work? I suppose that we do have a minimal version syntax
> for commands but not for mkfs.foo. And even for commands the version
> parser needs to be implemented for each command separately. We have one
> for mkfs.ext4 at the moment.
This is clearly an error I'm going to fix.
>
> I suppose that we need to add .mkfs_ver string to the struct tst_fs and
> possibly .kernel_ver as well so that we can add both checks to the
> structures as:
>
> 	{
> 		.type = "xfs",
> 		.mkfs_ver = ">= 5.1.0",
> 		.kernel_ver = ">= 4.9.0",
> 		...
> 	}
>
I'm not sure if it makes sense to add this feature if we already have 
.needs_cmd and .min_kver. It's better to keep things simple in this case.

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> I'm not sure if it makes sense to add this feature if we already have 
> .needs_cmd and .min_kver. It's better to keep things simple in this case.

That will obvioiusly not work because we need to skip just a single
filesystem not the whole test...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Andrea Cervesato via ltp]

On 9/26/24 17:40, Cyril Hrubis wrote:
> Hi!
>> I'm not sure if it makes sense to add this feature if we already have
>> .needs_cmd and .min_kver. It's better to keep things simple in this case.
> That will obvioiusly not work because we need to skip just a single
> filesystem not the whole test...
>
Right, so the discussion turns into what mkfs.* commands we want to 
support, because we need to create a parser for each one of them and at 
the moment I see mkfs.ext4 only.

Andrea


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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> >> I'm not sure if it makes sense to add this feature if we already have
> >> .needs_cmd and .min_kver. It's better to keep things simple in this case.
> > That will obvioiusly not work because we need to skip just a single
> > filesystem not the whole test...
> >
> Right, so the discussion turns into what mkfs.* commands we want to 
> support, because we need to create a parser for each one of them and at 
> the moment I see mkfs.ext4 only.

I would write the code "on demand" so I would implement xfs parser
beacause it's needed now.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Martin Doucha]

On 26. 09. 24 13:57, Cyril Hrubis wrote:
> I suppose that we need to add .mkfs_ver string to the struct tst_fs and
> possibly .kernel_ver as well so that we can add both checks to the
> structures as:
> 
> 	{
> 		.type = "xfs",
> 		.mkfs_ver = ">= 5.1.0",
> 		.kernel_ver = ">= 4.9.0",
> 		...
> 	}

It'd be simpler to add .skip_if_unsupported flag to struct tst_fs and 
then simply TCONF the single filesystem if mount() returns EOPNOTSUPP. 
That way we don't need hardcoded version checks for the kernel. MKFS 
versions checks would still be needed, though.

But since this would turn into a fairly large change right before 
release, I'd recommend reverting the ioctl_ficlone tests before the 
release and merging them back after.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> But since this would turn into a fairly large change right before 
> release, I'd recommend reverting the ioctl_ficlone tests before the 
> release and merging them back after.

That's what I plan to do, revert the tests, tag the release and apply
them again, so that we can fix them properly in the next development
cycle.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> > But since this would turn into a fairly large change right before 
> > release, I'd recommend reverting the ioctl_ficlone tests before the 
> > release and merging them back after.
> 
> That's what I plan to do, revert the tests, tag the release and apply
> them again, so that we can fix them properly in the next development
> cycle.

And the revert is not easy, because there were patches on the top of the
original additions. So I will just apply a change that removes the tests
from runtest files for the release instead.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] Fix ioctl_ficlone on XFS without reflink support

[Cyril Hrubis]

Hi!
> > I suppose that we need to add .mkfs_ver string to the struct tst_fs and
> > possibly .kernel_ver as well so that we can add both checks to the
> > structures as:
> > 
> > 	{
> > 		.type = "xfs",
> > 		.mkfs_ver = ">= 5.1.0",
> > 		.kernel_ver = ">= 4.9.0",
> > 		...
> > 	}
> 
> It'd be simpler to add .skip_if_unsupported flag to struct tst_fs and 
> then simply TCONF the single filesystem if mount() returns EOPNOTSUPP. 
> That way we don't need hardcoded version checks for the kernel. MKFS 
> versions checks would still be needed, though.

That's not a 100% correct either, with that we will not catch if the
functionality was disabled in the kernel by a mistake.

So I think that we need:

- minimal kernel version where the functionality was added in upstream
  in the tst_fs structure
- try to mount the fs
- if we get EOPNOTSUPP and current kernel is older than the minimal
  version report TCONF
- otherwise report TFAIL

-- 
Cyril Hrubis
chrubis@suse.cz

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