Expire a directory entry instead of invalidating it - #718
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e7a83d0ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let notif = Notification::new_inval_entry(parent, name, FUSE_EXPIRE_ONLY) | ||
| .map_err(Self::too_big_err)?; | ||
| self.send_inval(notify_code::FUSE_NOTIFY_INVAL_ENTRY, ¬if) |
There was a problem hiding this comment.
When this is called on a kernel that does not advertise FUSE_HAS_EXPIRE_ONLY, the last word is still treated as the old padding field, so older kernels accept the notification as a normal FUSE_NOTIFY_INVAL_ENTRY instead of rejecting it. In that unsupported-kernel scenario, callers who rely on the documented “too old” error can silently detach submounts—the behavior this API is meant to avoid—so the notifier needs to know/guard the negotiated capability or the precondition/error contract should be made explicit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right, and the wrong part was mine: the doc said a kernel too old to support expiring would return an error. Nothing produces that error. An old kernel reads the word as the padding it used to be, invalidates, and reports success.
The evidence was already sitting in the PR description without my noticing what it meant. I had recorded that sending an undefined bit (flags=0x2) returns Ok, and filed it as "this test does not discriminate". It does discriminate, just for a different question: the kernel does no validation of that word at all, so there is no mechanism by which an older kernel could reject bit 0 either.
I then tested the negotiation half directly, which turned up a wrinkle worth recording. Re-running the submount probe without calling add_capabilities(FUSE_HAS_EXPIRE_ONLY):
== WITH negotiation == submount before: true expire_entry -> Ok(()) after: true
== WITHOUT negotiation == submount before: true expire_entry -> Ok(()) after: true
So FUSE_HAS_EXPIRE_ONLY is an advertisement from the kernel, not a gate: a kernel that supports expiring honours the flag whether or not it was requested. That matters for the fix you suggest -- a guard keyed on the negotiated set would reject the common case where a filesystem never requested the capability but the kernel supports it. A correct guard has to key on what the kernel advertised, and Session currently keeps only the negotiated intersection, so that means new session state threaded through to Notifier.
d8fdcce takes the "make the contract explicit" half of your suggestion, since that part is unambiguous and small:
A kernel that predates expiring treats the flag as the padding it used to be: it invalidates the entry, detaching any submounts, and still reports success. There is no error to detect that by, and the kernel honours the flag whether or not it was requested during init, so a filesystem that depends on the entry surviving must check
KernelConfig::capabilities()forInitFlags::FUSE_HAS_EXPIRE_ONLYduring init and not call this when it is absent.
I have deliberately not added the guard. Failing closed would match what this repo already does for FUSE_SETXATTR_ACL_KILL_SGID in #716, and I think it is the better end state, but it needs Notifier to carry kernel-advertised capabilities and it is a policy call on @cberner's API rather than a defect fix. Happy to add it here or as a follow-up -- your call.
Generated by Claude Code
There was a problem hiding this comment.
Implemented in edbdb46 -- @cberner asked for the fail-closed version, so expire_entry() now returns ENOTSUP instead of sending a request the kernel would answer by invalidating.
The capability it keys on is the advertised set, not the negotiated one. That distinction is load-bearing: the kernel honours FUSE_EXPIRE_ONLY whether or not it was requested during init, so guarding on negotiated would reject every filesystem that never called add_capabilities. Session kept only the negotiated intersection, so this adds kernel_capabilities alongside it, threaded through SessionEventLoop and BackgroundSession into Notifier.
Verified both directions:
- Three unit tests over a real
Notifier: refused withENOTSUPwhen the capability is absent, sent when present, andinval_entry()unaffected since it depends on nothing. - End to end against a running kernel, with a filesystem that never calls
add_capabilities-- the guard passes on advertised support alone and the submount still survives:
submount before: true
expire_entry -> Ok(())
submount after: true
That last case is the one that would have broken under a negotiated-set guard, which is why it is worth having a test for.
Generated by Claude Code
2e7a83d to
d8fdcce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8fdcce6e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// Same notification, but with the flags word the kernel reads set to | ||
| /// FUSE_EXPIRE_ONLY instead of the zero that used to be hardcoded there. |
There was a problem hiding this comment.
This added doc comment explains the test in terms of what “used to be hardcoded” in this commit, which is a transient implementation detail rather than a long-term invariant. Please either remove the comment or keep it focused on the durable behavior being asserted, such as the flags word carrying FUSE_EXPIRE_ONLY.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and it is exactly what AGENTS.md rules out -- comments should carry long-term invariants, not minor implementation details of the current commit. "the zero that used to be hardcoded there" is commit-history prose that the code would have to keep maintaining.
Trimmed in f1f76e8 to the durable contrast the test actually asserts:
/// The same notification as `inval_entry`, but with FUSE_EXPIRE_ONLY in the flags word.
Generated by Claude Code
d8fdcce to
f1f76e8
Compare
fuse_notify_inval_entry_out carries a flags word that fuser sent as a hardcoded zero, named padding. Its one defined bit, FUSE_EXPIRE_ONLY, asks the kernel to mark the entry for revalidation rather than forcibly detaching it, which also detaches anything mounted beneath it. A filesystem that merely suspects an entry is stale had no way to say so and had to use the destructive form. Name the field and add Notifier::expire_entry() to set the bit. A kernel predating the flag reads the word as padding, invalidates, and reports success, so nothing in the reply reveals that the request was not honoured. Rather than leave that to a documented precondition, the notifier now carries what the kernel advertised during init and refuses to send with ENOTSUP when expiring is unsupported. The capability has to be the advertised set rather than the negotiated one: the kernel honours the flag whether or not it was requested, so keying on what was requested would reject filesystems that never asked for it. Confirmed against a running kernel that the two forms differ: with a bind mount over an entry of a mounted filesystem, inval_entry() detaches it and expire_entry() leaves it in place. Fixes #367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NjYWzn3mu8Sc2y2eACtJM9
f1f76e8 to
edbdb46
Compare
2 participants
Fixes #367 (ABI 7.38 milestone).
The problem
fuse_notify_inval_entry_outcarries a flags word that fuser sent as a hardcoded zero, namedpadding. Its one defined bit,FUSE_EXPIRE_ONLY, asks the kernel to mark the entry for revalidation rather than forcibly detaching it -- and detaching also detaches anything mounted beneath it. A filesystem that merely suspects an entry is stale had no way to say so, and had to reach for the destructive form.The change
Name the field and add
Notifier::expire_entry(). This follows libfuse, which exposes the same thing as a separatefuse_lowlevel_notify_expire_entry()rather than a flags parameter -- with a single defined bit, a public bitflags type would be more surface than the feature needs.expire_entry()fails closed: it returnsENOTSUPwhen the kernel did not advertiseFUSE_HAS_EXPIRE_ONLY, rather than sending a request such a kernel would answer by invalidating the entry, detaching submounts, and reporting success. Nothing in the reply distinguishes that case, so leaving it to a documented precondition would mean the one call whose purpose is to be non-destructive could silently be the destructive one.The guard keys on what the kernel advertised, not on the negotiated set. That distinction is load-bearing: the kernel honours the flag whether or not it was requested during init, so guarding on
negotiatedwould reject every filesystem that never calledadd_capabilities.Sessionkept only the negotiated intersection, so this addskernel_capabilitiesalongside it, threaded throughSessionEventLoopandBackgroundSessionintoNotifier.Verification
Two obvious checks turned out not to discriminate, which is worth recording since a reviewer would likely reach for them:
flags=0x2) returnsOk. The kernel does not validate that word at all, so acceptance proves nothing about whether it is read -- and by the same token, nothing could rejectFUSE_EXPIRE_ONLYon an older kernel either.lookup, so counting lookups proves nothing.The difference that does show up is submounts. With a bind mount over an entry of a mounted filesystem:
inval_entry(flags=0)expire_entry(FUSE_EXPIRE_ONLY)Separately, running that probe with and without
add_capabilities(FUSE_HAS_EXPIRE_ONLY)gave identical results, which is what establishes that the capability is an advertisement rather than a gate.Tests: a byte-level assertion that the flags word is
0x01whereinval_entry's is0x00, plus three over a realNotifiercovering both sides of the guard -- refused withENOTSUPwhen the capability is absent, sent when present, andinval_entry()unaffected since it depends on nothing. The guard was also re-checked end to end against a running kernel with a filesystem that never callsadd_capabilities, the case a negotiated-set guard would have broken.cargo test --all(82 lib tests),fmt,clippy --all-targetsandclippy --no-default-featuresclean underRUSTFLAGS=--deny warnings; all-features build andx86_64-apple-darwincheck clean.Noticed but not fixed
The
deletetest insrc/ll/notify.rscallsnew_inval_entryand sends the result under theFUSE_NOTIFY_DELETEcode, sonew_delete()and its 24-bytefuse_notify_delete_outlayout have no coverage. This PR only adds the newflagsargument to that call so it keeps compiling; correcting the test changes what it asserts, which is unrelated to this change. Happy to do it separately.