Fix casenorm ZTS tests - #19028
Fix casenorm ZTS tests#19028ryao wants to merge 1 commit into
Conversation
behlendorf
left a comment
behlendorf
left a comment
There was a problem hiding this comment.
These has been disabled forever, I'm glad to see them get sorted out!
behlendorf
added
Status: Code Review Needed
github-actions
Bot
removed
the
Status: Revision Needed
|
I was inspired by #19074 to tackle this another way. Instead of bypassing the dentry cache like FreeBSD and illumos-gate do for namecache and DNLC, respectively, I implemented ->d_hash and ->d_compare in dentry_operations so the dentry cache works properly on case-insensitive filesystems on Linux. Doing this in a Unicode-compatible way while operating without dynamic memory allocations in Linux's RCU context and keeping stack space usage down was challenging. |
69d1806 to
c5f2a5a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
7d2eaa9 to
29748bf
Compare
|
Long story short, I caught some edge cases and fixed them. The long story is: Without ->d_revalidate(), a cached negative dentry on a folded (normalized or case-insensitive) filesystem could change a file's name to something that the user did not intend in create/mkdir/link/symlink/mknod/rename. Implementing ->d_revalidate() to invalidate the dentry when it would be a problem works around that, but some in-tree stacking filesystems, such as NFSv2/3, do not pass the proper flags to us, so we need another workaround to invalidate when flags == 0 that unfortunately catches some cases where we do not want to invalidate. Additionally, overlayfs checks whether a filesystem implements ->d_hash() and ->d_compare() and returns -EREMOTE, since stacking does not work properly when it has a different idea of what a dentry name is than the filesystem beneath it. To work around the overlayfs issue on non-folding filesystems, I have implemented a version of dentry_operations for them that omits the new operations. This will still cause a regression for anyone using overlayfs on top of a folding filesystem, but that was subtly broken in the first place. That said, this change is useful since it prevents performance regressions on non-folded filesystems, and it let us remove some branching from these hooks on folded filesystems. |
159d2ce to
f3fc3e4
Compare
mixed_formd_lookup_ci and mixed_none_lookup_ci rely on the VFS supporting FIGNORECASE, which is only supported on illumos-gate, so we move them to sunos.run. As for the others, we only fail on Linux. To address that, we implement the .d_hash and .d_compare dentry_operations functions so that the dentry cache properly does case folding and normalization. This should give case insensitive filesystems better lookup performance on Linux than on FreeBSD, where the namecache is bypassed on case insensitive filesystems due to a lack of support for case insensitivity. Closing the performance gap on FreeBSD will either require changes to FreeBSD's VFS or require us to bypass FreeBSD's namecache in favor of implementing our own dentry cache. To make this work, we extend u8_textprep_str() to properly update the input size when encountering E2BIG, rather than leaving it undefined. Overlayfs is not compatible with filesystems that implement folding, so we maintain a version of dentry_operations for filesystems that do not require it. This will cause a regression for those who have been using overlayfs on top of folded filesystems. Those instances suffered from edge cases that would likely break it. The only way to avoid this is to add a dataset property that supports turning off the dentry_cache such that we could use the regular zpl_dentry_operations struct. That would allow overlayfs to work on folded filesystems again, although it would still be broken on edge cases, so we do not do that. Finally, we implement ->d_revalidate so that negative dentries do not cause us to override the name given by a user when creating a file. For example, we do not want stat FOO; touch foo to create a file named FOO. This does not apply to renames replacing a file, but typically, a user would want the file name to remain whatever was already there, so this is considered okay. Assisted-by: Grok 4.6 Build Beta Assisted-by: Claude 5 Opus Max Closes openzfs#7633 Signed-off-by: Richard Yao <richard@ryao.dev>
|
|
||
| error = -zfs_lookup(ITOZ(dir), dname(dentry), &zp, | ||
| zfs_flags, cr, NULL, ppn); | ||
| zfs_flags, cr, NULL, NULL); |
There was a problem hiding this comment.
This was the only location where a pathname_t is passed. It'd be good to review the code and see if we can further simplify things by dropping this argument from zfs_lookup.
There was a problem hiding this comment.
We can, but the current zfs_lookup() prototype is from illumos-gate. I am inclined to leave changing that to a future refactor of the platform-dependent / platform-independent line. I think @robn has some work in this area planned. Would you prefer we change that prototype as part of this, or leave it for a future refactor?
There was a problem hiding this comment.
Both the Linux and FreeBSD prototypes have diverged from illumos-gate so it's already been changed. That said, I'm fine with leaving it for a future refactor. Let's just update the comment for the Linux version of zfs_lookup() to mention realpnp is unused and may be removed in a future refactoring.
| /* | ||
| * Overlayfs is incompatible with filesystems that fold filenames, so we | ||
| * maintain a non-folding version for non-folding filesystems, so that it might | ||
| * work on top of them. |
There was a problem hiding this comment.
Can you update the casesensitivity / normalization descriptions in the zfsprops.7 man page to describe from a high-level user perspective how these settings interact with overlayfs on Linux.
There was a problem hiding this comment.
Sure. I will do that in the next push.
| } | ||
|
|
||
| if (error == -ENOENT) | ||
| return (d_splice_alias(NULL, dentry)); |
There was a problem hiding this comment.
I think this can shadow .zfs on case insensitive datasets. zfs_dirlook() matches .zfs with exact strcmp, so a lookup of .ZFS goes to the ZAP, gets ENOENT and now we cache a negative dentry for it. d_hash/d_compare treat .ZFS and .zfs as the same name, so the next lookup of .zfs hits that negative dentry and fails too, and d_revalidate keeps it since it is a plain lookup. Reproducer:
zfs create -o casesensitivity=insensitive tank/ci
stat /tank/ci/.ZFS # ENOENT, fine
stat /tank/ci/.zfs # ENOENT, wrong, works without this PR
Same for .zfs/SNAPSHOT then .zfs/snapshot via zpl_root_lookup(). Stays broken until the dentry is evicted. On TrueNAS this breaks SMB previous versions served from .zfs/snapshot.
Maybe skip caching the negative dentry here when the dir has ctldir and the name folds to .zfs, and return NULL on ENOENT in zpl_root_lookup().
There was a problem hiding this comment.
Nice catch. I am inclined to agree with your proposed solution. I will think about it a bit more and publish a revision to address it.
behlendorf
added
Status: Revision Needed
4 participants
Motivation and Context
It is about time we closed #7633.
Description
mixed_formd_lookup_ci and mixed_none_lookup_ci rely on the VFS supporting FIGNORECASE, which is only supported on illumos-gate. As for the others, we ->d_hash and ->d_compare in dentry_operations so the dentry cache works properly on case-insensitive filesystems on Linux.
How Has This Been Tested?
The ZTS casenorm group has been run against it on Linux and FreeBSD.
Types of Changes
Checklist
Signed-off-by.