Repository navigation
Linux 6.17: fuse: do not send the zeroed EOF folio tail in iomap writeback - #264
Conversation
iomap_writeback_handle_eof() zeroes the folio straddling i_size past that size before ->writeback_range() runs, and passes the size on as end_pos. fuse_iomap_writeback_range() ignored end_pos and queued the whole block, relying on the crop in fuse_send_writepage() to drop the bytes past EOF. That crop reads i_size again when the request is sent. Under DLM with the writeback cache, i_size is only this node's view: each writer claims its own extension under fi->lock while the inode lock is held shared. A partial EOF folio can hold another node's bytes, read in by the read-modify-write read under the write grant. If a concurrent local writer extends i_size between the zeroing and the crop, the crop no longer trims the folio and the zeros are written over the other node's acknowledged data. RED-51747 hits this with three nodes writing interleaved 47008-byte blocks of one file: the higher block's head reads back as zeros up to the page boundary. Traces show the lower writer's flush leaving with "own block + zeros" while the page it read from the server, and the i_size it zeroed at, were correct; a higher-offset write had extended i_size 40 us before the request was sent. Send only up to end_pos and still report the whole range as handled, so the bytes iomap zeroed never reach the server. The 6.12 tree avoided this because its writeback crops at i_size without zeroing. Signed-off-by: Sven Oehme <soehme@ddn.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
achhenderson
left a comment
There was a problem hiding this comment.
Approve for the failure this targets, with one follow-up to track (also summarised on RED-51747, comment 10490229). The nits at the end are non-blocking.
What I checked
I read the diff against redfs-ubuntu-hwe-6.17.0-16.16-24.04.1 together with fs/iomap/buffered-io.c on the same branch, and matched it against the traces on RED-51747 (iteration 1820 with crc tracing, iteration 720 with kernel ftrace). Both traces show the same window: iomap_writeback_handle_eof() zeroes the folio at the node-local i_size, a concurrent local write then extends i_size before fuse_flush_writepages() reads it as crop, and fuse_send_writepage() sends the whole page. Sending end_pos - pos closes that window because end_pos is the exact size the zeroing used, so whatever i_size does afterwards cannot widen the send.
Correctness of the diff
send_lencannot be zero or underflow.iomap_writeback_folio()assertsend_pos > posand bounds the dirty-range walk byround_up(end_pos, blocksize), so every range passed to->writeback_rangestarts belowend_pos.- Returning
lenrather thansend_lenis required:iomap_writeback_range()treats 0 or> rlenas an error and advances by the return value. - A short descriptor cannot misplace a following folio.
fuse_writepage_need_send()checks byte contiguity, so the next folio opens a new request. In practice the short descriptor is always last anyway, since the next folio is entirely pastend_posand skipped byhandle_eof. fuse_send_writepage()sumsdescs[i].length, so the send-time crop runs on the already clamped size and the shrinking-i_sizebranch is unchanged.- Writeback completion counts folios (
iomap_start_folio_write(..., 1)/iomap_finish_folio_write(..., 1)), not bytes, so the shorter length does not affectwrite_bytes_pending. nr_bytesand thealignment_pagesarithmetic usesend_len, which is consistent.- The clamp is not gated on
fc->dlm. I think that is right: bytes pasti_sizeat zeroing time are never meaningful to the server, and single-client behaviour is unchanged in effect.
The commit message matches the code and the traces. The statement that 6.12 crops at i_size without zeroing is consistent with the 6.12 runs on smc-a9 not reproducing.
What this does not close (follow-up, not a blocker)
After the clamp, the folio on the lower writer still holds zeros where the other node's bytes were, and it is clean and uptodate. Concretely, with the reproducer's per-block fsync:
- U writes the head of block b+1 into page P; the server stores it. L takes the grant, read-modify-writes P (now holding U's bytes), writes the tail of block b. L's
i_sizeis the end of block b. - L fsyncs while still holding the grant.
handle_eofzeroes U's bytes in L's copy of P. The clamp sends only L's bytes, which is correct on the wire. P stays cached on L with zeros. - L's next write (block b+3) raises
i_sizepast P. - A buffered read on L of P now returns zeros from cache with no server round trip, for as long as L holds the grant. If L ever re-dirties its own part of P, the whole block is dirty,
end_posis past P, and the zeros go to the server.
The revoke path is safe: fuse_reverse_inval_inode() writes back and then drops the range, and a clean folio drops without -EBUSY. So the exposure is between a flusher/fsync writeback and the next revoke. Whether the reproducer's read-back hits step 4 depends on whether the writer mount reads before another mount's grant revokes it; a buffered read on the writer mount immediately after its fsync would test it directly.
The fix for that is the other proposal already on the ticket: under fc->dlm && writeback_cache, raise i_size to max(i_size, pos + nob) under fi->lock when a read reply arrives, in both fuse_do_readfolio() and fuse_readpages_end(), bumping attr_version and honouring FUSE_I_SIZE_UNSTABLE as fuse_read_update_size() does. That stops handle_eof from zeroing other nodes' bytes at all and keeps this clamp as the second guard. It needs a server-side confirmation first: a remote shrink must revoke grants on the range before the new size is visible, otherwise a read reply could raise i_size over a truncated-away region. I'd keep that as a separate PR.
Nits (non-blocking)
- Add a
WARN_ON_ONCE(send_len == 0)(or equivalent) next to the clamp. It is unreachable today, but the invariant lives in iomap, not here. - Add one sentence to the commit message noting that the zeroed tail remains in the cached folio and that the read-reply
i_sizeupdate is the follow-up, so the next reader does not take this as the complete fix. - Please post the result of the 3-hour
20261016.clamp1run on the ticket or here before merge, so the PR carries its test evidence.
|
Test result on tyan-h3-u24 (Ubuntu 6.17.0-42, this change built as DKMS
The open question about the zeroed tail staying in the uptodate folio still stands; no local buffered-read mismatch showed up in this run. 🤖 Generated with Claude Code |
|
Tested on 6 client hosts (tyan-h4-u7..u12, Ubuntu 6.17.0-40-generic) with one write-back redfs mount each, using a multi-host version of the RED-51747 reproducer. Three hosts write interleaved 47008-byte blocks and three only verify. The module is the installed
The clamp fired 34 times during the run. Each time i_size had already moved past The open review point still applies: the zeroed tail stays in the clean, uptodate folio after a writeback that is not a revoke. A follow-up test will add raising i_size on read replies under DLM. |
|
Companion change: #267 raises i_size over the bytes a DLM read reply returns, so iomap_writeback_handle_eof() no longer zeroes another node's bytes in a shared page in the common case. It is independent of this PR, but the raise is skipped for short reads, for a whole-file invalidate that races the read, and for pages that never came from a read reply, and in those cases only this clamp keeps the zeroed tail off the server. With both applied, tyan-h4-u7..u12 ran 1 h of brlock_mh.py (17,782 iterations) and 1 h of brlock_mh.py --midsync (12,036 iterations) with 0 corruption, and the clamp hit 16 and 2 times, against 34 for this PR alone. Please merge both. |
ea6461c
into
redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
|
Post-merge check on tyan-h3-u24: the deployed redfs.ko (DKMS 🤖 Generated with Claude Code |
Draft fix for RED-51747 (write-back data corruption on a 4 KiB page shared by two writers).
Problem
iomap_writeback_handle_eof()zeroes the folio straddling i_size past that size, then calls->writeback_range()withend_pos= i_size.fuse_iomap_writeback_range()ignoresend_posand queues the whole block, relying on the crop infuse_send_writepage(). That crop reads i_size again later, when the request is sent.Under DLM with the writeback cache, i_size is only this node's view. Writers hold the inode lock shared and each claims its own extension under
fi->lock. A partial EOF folio can hold another node's bytes, read in by the read-modify-write read under the write grant. If a concurrent local writer extends i_size between the zeroing and the crop, the zeroed tail is written over the other node's acknowledged data.Evidence
The reproducer is in red/red PR 29256 (
test/client/brlock_repro): three nodes writing interleaved 47008-byte blocks of one file. It fails about once every 2000 iterations. A redfsd build with crc64 tracing on every read reply and writeback payload showed, at iteration 1820:Full analysis and traces are in RED-51747.
Fix
Send only up to
end_pos, but still report the whole range as handled, so the bytes iomap zeroed never leave the node. The 6.12 tree is not affected: its writeback crops at i_size and does not zero.Status
Draft. A 3-hour test with this change (as DKMS 20261016.clamp1 on Ubuntu 6.17.0-42) is running on tyan-h3-u24. The result will be added here and in RED-51747.
redfs-ubuntu-resolute-7.0.0-14.14has the samefuse_iomap_writeback_range()and needs the same change.Open question for review: the zeroed tail stays in the clean, uptodate folio. After a revoke it is invalidated, but after a flusher writeback a later local i_size extension could expose those zeros to local readers. Should the EOF folio be dropped or kept non-uptodate past
end_posunder DLM?🤖 Generated with Claude Code