Skip to content

ZTS: add mount_loopback to test zfs behind loop dev #17329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tonyhutter
Copy link
Contributor

Motivation and Context

Add test for #17277

Description

Add a test case to reproduce issue #17277. This has actually been fixed by #17298, but add a test case for good measure. The test case verifies that we can make an xfs filesystem on a ZFS-backed loopback device.

How Has This Been Tested?

Test case added

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:

@tonyhutter tonyhutter force-pushed the mountloop branch 2 times, most recently from 8415e39 to c967763 Compare May 14, 2025 23:48
@tonyhutter
Copy link
Contributor Author

@behlendorf I included your fixes in my latest push.

@robn
Copy link
Member

robn commented May 16, 2025

Two thoughts:

  • maybe a skip if you don't have mkfs.xfs and/or XFS support in your kernel (I routinely don't have those in my tiny VMs)
  • it's more work, but would we be better to identify the IO pattern that tickled the bug, and write a program to test that? If XFS or the kernel ever changes in the future, the test could end up quietly not actually testing anything useful, and not alerting us to a future break. On the other hand, there's no harm in having a straight up functional "can we even mount a loopback filesystem" test, so as long as we know what this test is, probably nbd.

@tonyhutter
Copy link
Contributor Author

@robn -

maybe a skip if you don't have mkfs.xfs and/or XFS support in your kernel (I routinely don't have those in my tiny VMs)

Thanks - I just added a check in my latest push

On the other hand, there's no harm in having a straight up functional "can we even mount a loopback filesystem" test, so as long as we know what this test is, probably nbd.

Yes, its just a basic sanity test to verify "filesystem over loopback over ZFS".

Add a test case to reproduce issue openzfs#17277:

1. Make a pool
2. Write a file to the pool
3. Mount the file as a loopback device
4. Make an XFS filesystem on the loopback device
5. Mount the XFS filesystem... <hangs>

Signed-off-by: Tony Hutter <[email protected]>
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@amotin
Copy link
Member

amotin commented May 19, 2025

It should probably be specified somewhere that skipped test is OK:

Tests with results other than PASS that are unexpected:
    SKIP mount/mount_loopback (expected PASS)

@amotin amotin added the Status: Revision Needed Changes are required for the PR to be accepted label May 19, 2025
if [ ! -d /lib/modules/$(uname -r)/kernel/fs/xfs ] && \
! grep -qE '\sxfs$' /proc/filesystems ; then
log_unsupported "No XFS kernel support"
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since xfs may not be built as a kmod it's probably more reliable to check that either CONFIG_XFS_FS=m or CONFIG_XFS_FS=y is set in /boot/config-$(uname -r).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the /proc/filesystems check though, right? If it's not a module, then it must be builtin and so in the filesystem list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robn that's right - the two checks are to look for modules & built-in filesystems. I actually think the /boot/config-* check is a little more elegant, but I wasn't 100% sure all distros put their configs in /boot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work for at least the distributions covered by the CI. We already do a similar check for uring support in functional/io/io_uring.ksh. If you wanted to be fancy you could first check for /proc/config.gz. Providing the kernel config in /proc/ has been supported for years now, but I'm not sure how commonly it's enabled in distribution kernels.

At least on RHEL8 it's disabled:

/boot/config-4.18.0-553.50.1.el8_10.x86_64:# CONFIG_IKCONFIG is not set
/boot/config-4.18.0-553.51.1.el8_10.x86_64:# CONFIG_IKCONFIG is not set

# 1. Make a pool
# 2. Make a file on the pool or create zvol
# 3. Mount the file/zvol behind a loopback device
# 4. Create & mount an xfs filesystem on the loopback device
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to be greedy, but it'd be nice to have an identical test which uses ext4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Revision Needed Changes are required for the PR to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants