tests/cp_files: fallback for C libs w/o SEEK_DATA

[jlsalvador]

Not all C libraries support SEEK_DATA (e.g., uClibc). When SEEK_DATA is not defined, fallback to SEEK_END.

Motivation and Context

Currently, uClibc does not compile OpenZFS because the test cp_files uses SEEK_DATA as the whence parameter for the lseek method.

Description

When SEEK_DATA is not defined, use SEEK_END as the fallback value.

How Has This Been Tested?

• Compiling Buildroot with a preliminary patch version using GLIBC, UCLIBC, and MUSL as toolchains.
• Running the ZFS Test Suite with this change applied, although the affected test was skipped due to a kernel configuration for Debian 12.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[jlsalvador]

Hello all,

Any alternative instead of SEEK_END?
Thoughts?

[rincebrain]

I think the right thing to do, if SEEK_DATA/SEEK_HOLE are not available, is to make all the tests that rely on that constant get marked SKIP, rather than substituting one with different semantics to test.

I'd probably do this with a configure time check that checked for support for those and then defined something for the test suite and that test program to change their behavior around, but that's off the top of my head, I don't know offhand how we do this for other platform-specific checks.

[behlendorf]

Right, instead of changing the behavior of this test we should disable it as @rincebrain suggested.

[robn]

Another option would be to define those two if they're missing:

#ifndef SEEK_DATA
#define SEEK_DATA 3
#endif
#ifndef SEEK_HOLE
#define SEEK_HOLE 4
#endif

That particular value gets passed directly into SYS__llseek in the kernel, so it should always work on any Linux kernel.

Though as I type is, I feel a little uncomfortable about it. Disabling makes more sense. @jlsalvador if you'd like a hand with the detection bits, let me know - I do know my way around that stuff.

[jlsalvador]

Hello again,

I force-pushed a commit with another solution as @rincebrain suggested.

I have made a few assumptions. Could someone please check if the patch follows the ZFS guidelines?

Perhaps someone could improve it by skipping the test case at runtime (printing [SKIP]) instead of ignoring it with a new AutoMake conditional (WANT_UNISTD_SEEK_DATA).

I tested it by building Buildroot using GLIBC, UCLIBC, and MUSL toolchains.
Currently I am running the ZFS Test Suite on a Debian 12 (./configure --disable-pyzfs), it requires a few hours to finish.

---

@robn, thank you for offering to help. Could you please confirm if the patch is correct?

[behlendorf]

Perhaps someone could improve it by skipping the test case at runtime (printing [SKIP]) instead of ignoring it with a new

The way we've handled this before is to only build the cp_files and seekflood binaries when SEEK_DATA / SEEK_HOLE are available. Which is what you've already done. Then at the top of each relevant test add a check for the binary. If it doesn't exist call log_unsupported with a meaningful error message which will cause the script to exist. You'll also need to update zts-report.py.in to understand these tests may be skipped. The Linux specific tmpfile tests provide a good example of this.

[robn]

@jlsalvador sorry for the delay. This is looking pretty close!

Did you intend to remove the entire cp_files tag? Seems like what we could actually do is not build seekflood, and then have cp_stress.ksh skip if seekflood is not available. That would allow the other cp_files tests to run, and then we don't need to worry about excluding cp_stress.ksh specifically from the build and runfile.

[jlsalvador]

@jlsalvador sorry for the delay. This is looking pretty close!

Did you intend to remove the entire cp_files tag? Seems like what we could actually do is not build seekflood, and then have cp_stress.ksh skip if seekflood is not available. That would allow the other cp_files tests to run, and then we don't need to worry about excluding cp_stress.ksh specifically from the build and runfile.

I'll look at it this weekend.

---

I force-pushed another revision incorporating @robn's suggestions to rename ZFS_AC_CONFIG_USER_UNISTD to ZFS_AC_CONFIG_USER_UNISTD_SEEK_DATA and to add AC_MSG_CHECKING and AC_MSG_RESULT. Thank you, @robn, for your suggestions.

[jlsalvador]

Let's work on resolving this issue! 💪

Current issue details: Buildroot Job Log.

What do we need to include a fix for this issue? Can the patch be backported as it was for the original issue?

[mcmilk]

The cp_files/cp_stress should run as expected on Fedora 39, Fedora 40 and FreeBSD stable/13, so this seems not okay:

Tests with result of PASS that are unexpected:
    PASS cp_files/cp_stress (expected SKIP)

I would prefer to make the needed definitions by hand for now and opened another pull request for this: #16413.

[jlsalvador]

The cp_files/cp_stress should run as expected on Fedora 39, Fedora 40 and FreeBSD stable/13, so this seems not okay:

Tests with result of PASS that are unexpected:
    PASS cp_files/cp_stress (expected SKIP)

I would prefer to make the needed definitions by hand for now and opened another pull request for this: #16413.

As I read it, the test passed correctly; it just was expected to be skipped (hard-coded as-is in this patch).

This is because I included the line 'cp_files/cp_stress': ['SKIP', 16169] in tests/test-runner/bin/zts-report.py.in. I could include it only when the test is expected to be skipped.

---

uClibc-NG has just added SEEK_DATA and SEEK_HOLE support for the next versions:
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/commit/?id=05b11809bd11450aff3d20e43f18415ce58601e7

[mcmilk]

This is because I included the line 'cp_files/cp_stress': ['SKIP', 16169] in tests/test-runner/bin/zts-report.py.in. I could include it only when the test is expected to be skipped.

The test shouldn't be expected to be skipped by default. The other way around it would be better.

---

I searched for uClibc and uClibc-ng ... and it looks like your own project should use uClibc-ng :)
I don't think OpenZFS should add work arounds for an old uClibc from around 2012.

How does this pull request deal with the other user of SEEK_DATA: tests/zfs-tests/cmd/mmap_seek.c ?

[jlsalvador]

This is because I included the line 'cp_files/cp_stress': ['SKIP', 16169] in tests/test-runner/bin/zts-report.py.in. I could include it only when the test is expected to be skipped.

The test shouldn't be expected to be skipped by default. The other way around it would be better.

I searched for uClibc and uClibc-ng ... and it looks like your own project should use uClibc-ng :) I don't think OpenZFS should add work arounds for an old uClibc from around 2012.

How does this pull request deal with the other user of SEEK_DATA: tests/zfs-tests/cmd/mmap_seek.c ?

Your patch is cleaner and smaller than mine. Thanks.
I have already tested it for Buildroot (uClibc).
So, I'm going to close my pull request.

Thank you all!