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

Andrea Cervesato via ltp
2024-09-26T14:58:46Z

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.