Update atime on read in the simple example - #739
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: f0e852e361
ℹ️ 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".
| if let Ok(mut attrs) = self.get_inode(ino) { | ||
| self.touch_atime(&mut attrs); | ||
| } |
There was a problem hiding this comment.
When the kernel dispatches FUSE_COPY_FILE_RANGE (for example via copy_file_range(2)), the implementation below reads from src_inode with read_exact_at but never calls this atime updater. Linux advances the source file's atime for successful copies, so a freshly written source can still report atime < mtime after being copied even though normal read now updates it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in c6b1124. Confirmed against tmpfs before changing anything:
copy_file_range, source atime moved? tmpfs YES fuser (before) no
tmpfs YES fuser (after) YES
relatime still applies on both: a second copy of the same source leaves atime alone.
Worth naming how I missed it, because the gap was in my method rather than a detail I overlooked. I went through read, readdir and readlink - the handlers named after reading - and never asked the other question, which is which handlers read data regardless of their name. copy_file_range reads the source directly with read_exact_at and never goes through read(), so it was invisible to the way I framed the check.
Applying the other question to the rest of the surface: readdirplus is not implemented here, so the kernel falls back to readdir, which is covered. getattr, lookup and getxattr read metadata, not data, and Linux does not move atime for those. write and fallocate do not read. That leaves nothing else, but the enumeration is the point - the previous list happened to be complete for the paths I had changed, not for the paths that access data.
The PR body still says three access paths and there are four; I will correct it.
Generated by Claude Code
f0e852e to
c6b1124
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6b1124f38
ℹ️ 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 mut buffer = vec![0; read_size as usize]; | ||
| file.read_exact_at(&mut buffer, offset as u64).unwrap(); | ||
| if let Ok(mut attrs) = self.get_inode(ino) { | ||
| self.touch_atime(&mut attrs); |
There was a problem hiding this comment.
When a caller opens a file with O_NOATIME, Linux leaves atime unchanged for reads through that handle, but this new unconditional call advances it anyway. The FUSE read request already carries the file flags as _flags here, and for paths like readdir/copy_file_range the bit would need to be preserved from open in the file handle before calling touch_atime; otherwise backup/indexing tools that use O_NOATIME will dirty access times on this filesystem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right about the divergence, and read() now honors it in 8aeefad. Two things I checked before changing anything, since the answer was not obvious to me.
Does the read request really carry the flag? Yes - I instrumented a build to print it rather than assume:
normal read flags=0x8000 noatime=false
O_NOATIME read flags=0x48000 noatime=true (O_LARGEFILE | O_NOATIME)
So it is one condition in read(), no plumbing. Against tmpfs, before and after:
O_NOATIME read normal read
tmpfs suppressed moves
fuser (before) MOVED moves
fuser (after) suppressed moves
readdir() and copy_file_range() I have deliberately left alone, and I want to be straight that this is a judgement call rather than an oversight - it is the same shape of gap you caught in the copy_file_range thread. Neither is given open flags, so honoring O_NOATIME there means carrying a third bit in the file handle alongside FILE_HANDLE_READ_BIT and FILE_HANDLE_WRITE_BIT, set at open()/opendir(). That is roughly twenty lines and follows an existing pattern, so it is not hard - it is a question of whether it belongs in this example.
I came down against it on three grounds: no xfstest exercises O_NOATIME at all (I grepped tests/generic; the only hit anywhere is a src/ helper), opening a directory or a copy source with O_NOATIME is genuinely obscure next to a backup tool reading file data, and this example has been getting trimmed rather than grown.
Rather than leave that silently inconsistent, the limitation is now written into the touch_atime doc comment, naming which paths honor the flag and what carrying it further would take. Happy to add the plumbing if you would rather have it complete.
Worth flagging for review either way: O_NOATIME is the one part of this PR with no coverage in either suite, so a green run says nothing about it. The tmpfs comparison above is the only real check on those lines.
Generated by Claude Code
c6b1124 to
8aeefad
Compare
The example set last_accessed when an inode was created and when utimens asked for it, and never again, so atime never moved for a reader. That is what generic/192 was catching. Reads now follow relatime, the kernel's default: atime moves only when it is no newer than mtime or ctime - that is, when nothing has read the inode since it last changed - or when it has gone stale by more than a day. The alternative, moving it on every read, would mean an inode write per read for no gain: what a reader can draw from atime is that an atime older than mtime means nobody has looked since the last write, and relatime preserves exactly that. read(), readdir(), readlink() and the source side of copy_file_range() all count as access, as they do on a local filesystem. read() also honors O_NOATIME, which a read request carries: a caller that asked to leave no trace does not move atime. readdir() and copy_file_range() are not given the flag and would have to carry it over from the open() that set it, which the helper's comment records. A mount can ask for noatime or strictatime instead, but no FUSE request reports which the mount used and this example takes no option for it, so the kernel default is the only policy on offer. generic/003 stays excluded, with its reason corrected. It exercises the noatime, relatime and strictatime mount options and a read-only mount, and fuse-xfstests cannot ask for any of them: _fuser_mount reads $4 for suid and drops the rest, so "_scratch_mount -o relatime" and "_scratch_cycle_mount noatime" both arrive as a plain mount. Its failures have moved from "atime never updates" to four complaints that are purely about options the filesystem never sees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSmtYYZ3resRdsCD1PwqUK
8aeefad to
978d974
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 978d974e0a
ℹ️ 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".
| || attrs.last_accessed.0 < a_day_ago | ||
| { | ||
| attrs.last_accessed = now; | ||
| self.write_inode(attrs); |
There was a problem hiding this comment.
When the simple example is mounted with --n-threads > 1, this helper can run in parallel with write(), truncate(), or setattr() on the same inode. It receives a snapshot read before the atime update and write_inode rewrites the entire serialized inode here, so if another request updates size, mtime, mode, or xattrs between the caller's get_inode and this write, a read/readdir/readlink can roll those fields back; a read should not be able to undo a completed metadata or data change. Serialize inode updates or apply the atime change against the latest inode state instead of writing the stale snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The race is real, but it is the example's existing concurrency model rather than something this change introduces, so I am not fixing it here.
Checked rather than assumed:
- there is no locking anywhere in
examples/simple.rs- noMutex, noRwLock, nothing - there are 38
write_inodecall sites, and every one is the same unguarded read-modify-write of the whole serialized record - the file already says of its file-handle scheme that it "isn't safe... this implementation is just a toy"
So two concurrent write()s to one inode already roll each other's size and mtime back, and setattr racing write already does the same. Serializing this properly means per-inode locking across all 38 sites, which is an architectural change to the example and not something to bolt onto an atime PR.
One part of your point I do want to separate out, because it is fair and is new: before this change reads were pure, so read-vs-write was not a racing pair at all. Making reads into writers adds a hazard category that did not exist, even though the mechanism is the same one already used everywhere else. relatime narrows it - the write only happens on the first read after a change, not on every read - but narrow is not none.
I considered re-reading the inode inside the helper just before writing. That only shrinks the window while leaving the same time-of-check-to-time-of-use hole, and it would read as if the problem had been handled, so I would rather leave the honest version.
Worth noting --n-threads defaults to 1 and the test suites do not pass it, so nothing in CI exercises this - which is also why I would not want to "fix" it half way on the strength of a green run.
Happy to do the locking as its own change if you want it; it would touch every handler, so it seems like your call rather than mine.
Generated by Claude Code
2 participants
The example set
last_accessedwhen an inode was created and whenutimensasked for it, and never again, so atime never moved for a reader. generic/192 was catching that.721 passing, up from 720.
The policy
Reads follow
relatime, the kernel's default: atime moves only when it is no newer than mtime or ctime - that is, when nothing has read the inode since it last changed - or when it has gone stale by more than a day.Moving it on every read would mean an inode write per read for no gain. What a reader can actually draw from atime is that an atime older than mtime means nobody has looked since the last write, and relatime preserves exactly that. In the steady state a read costs nothing extra: suite runtime is unchanged.
Four paths count as access, as they do on a local filesystem:
read(),readdir(),readlink(), and the source side ofcopy_file_range(). That last one was missed in the first version and caught in review - it reads the source withread_exact_atwithout going throughread(), so listing the handlers named after reading was not enough; the question is which handlers read data. Applying that:readdirplusis not implemented so the kernel falls back toreaddir;getattr,lookupandgetxattrread metadata rather than data, which Linux does not count;writeandfallocatedo not read.read()also honorsO_NOATIMEon Linux. A read request carries the flag - verified by instrumenting a build rather than assuming:readdir()andcopy_file_range()are not given open flags, so honoring it there would mean carrying a third bit in the file handle alongsideFILE_HANDLE_READ_BITandFILE_HANDLE_WRITE_BIT. I left that out - no test anywhere exercisesO_NOATIME, and a directory or copy source opened with it is obscure next to a backup tool reading file data - and recorded the limitation in the helper's doc comment so it is not silently inconsistent. Say the word if you would rather have it complete.A mount can ask for
noatimeorstrictatimeinstead, but no FUSE request reports which the mount used and this example takes no option for it, so the kernel default is the only policy on offer.Checked against tmpfs
tmpfs also mounts relatime, so it is like-for-like:
readlinkmoves atimecopy_file_rangemoves source atimeO_NOATIMEread suppressesgeneric/192 additionally requires atime to survive a mount cycle, which it does - the inode is on disk.
Worth flagging for review:
O_NOATIMEis the one part of this change with no coverage in either suite, so a green run says nothing about it. The tmpfs comparison is the only real check on those lines.generic/003 stays excluded, with its reason corrected
It was filed as "requires atime support", which is not the whole story. It exercises the
noatime,relatimeandstrictatimemount options plus a read-only mount, and fuse-xfstests cannot ask for any of them:Its failures moved rather than disappeared, which is the evidence. Before: four errors saying atime never updates. After:
Every one is an option the filesystem never sees. Same category as generic/306 and 452: it needs a change in fuse-xfstests, not in fuser.
Testing
All against the final commit:
cargo clippy --all-targetsclean under--deny warningscargo check --target aarch64-apple-darwin --features=macos-no-mount --examplescleanThat last one is new, and I added it because I broke
mac-ciandfreebsd-cion an earlier revision of this PR:libc::O_NOATIMEis Linux-only and does not exist on either. The flag test is now behind#[cfg(target_os = "linux")], matching how the rename flags are already handled. The darwin cross-check reproduces mac-ci's exact invocation locally, so that class of breakage is catchable before pushing next time.Known limitation, not addressed here
Review also raised that the atime write is an unguarded read-modify-write that can roll back a concurrent
write/setattrunder--n-threads > 1. That is real, but it is the example's existing model: there is no locking anywhere in the file, and all 38write_inodesites have the same shape - two concurrentwrite()s already race identically. Fixing it means per-inode locking across every handler, which is an architectural change rather than part of an atime PR. The one genuinely new element is that reads were previously pure, so read-vs-write was not a racing pair at all; relatime narrows the window to the first read after a change but does not remove it. Happy to do the locking separately if wanted.Remaining in the exclusion list
Nothing left that fuser alone can fix. generic/394's assertion already passes and only its cleanup fails, needing
CAP_SYS_RESOURCEon the test container. generic/003, 294, 306 and 452 need mount options to reach the filesystem, which is a fuse-xfstests change. The rest are Docker restrictions, missing tooling, slow tests, or genuine design mismatches.