Fix ioctl_ficlone on XFS without reflink support

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(-)


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

Best regards,

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",
		...
	}

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.

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...

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

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.

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.

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.

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.

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