Repository navigation
RED-51747: Linux 6.17: fuse: raise i_size from a read reply under DLM with the writeback cache - #265
Closed
achhenderson wants to merge 1 commit into
Conversation
Under DLM with the writeback cache, i_size is this node's view of the file: each cached write claims its own extension under fi->lock, and the server's size is not consulted while the node holds a grant. Bytes another node wrote past this node's i_size are therefore "past EOF" to every piece of generic code that looks at i_size, even once a read has brought them into the page cache. The read-modify-write read of a cached write does exactly that. In RED-51747 three nodes write interleaved 47008-byte blocks of one file. The lower writer's write ends inside a page whose tail already holds the upper writer's bytes on the server. The lower writer claims i_size up to the end of its own block, reads the page under its write grant, copies its bytes in, and the page is correct. When the page is written back, iomap_writeback_handle_eof() zeroes it from the local i_size to the page end. The previous commit keeps those zeros off the wire, but they stay in the folio, which is clean and uptodate: once a later local write raises i_size past the page, a buffered read on this node returns the zeros from the cache, and if this node dirties the page again the whole block is written and the zeros reach the server after all. A read reply is evidence of the file's size. The server answers short past its EOF, so num_read bytes at pos mean the file reaches at least pos + num_read. The grant the read was made under keeps it there: a remote truncate revokes every grant above its new size before it proceeds, which fuse_attr_cache_mask() already relies on, and a local truncate holds i_rwsem exclusive and sets FUSE_I_SIZE_UNSTABLE until the page cache has been cut. Raise i_size to pos + num_read when a read reply arrives, in fuse_do_readfolio() and fuse_readpages_end(), under fi->lock. The folio that holds another node's bytes is then inside i_size, and neither iomap_writeback_handle_eof() nor iomap_block_needs_zeroing() treats its contents as post-EOF. The clamp in fuse_iomap_writeback_range() remains as the second guard. The update follows fuse_read_update_size(): it bumps attr_version, so an attribute reply older than this evidence does not shrink the size back, and it is skipped when the attributes have moved on since the read was sent or while FUSE_I_SIZE_UNSTABLE is set, so a reply issued before a truncate cannot undo it. The O_TRUNC open path bumps attr_version before it empties the cache, so that check covers it as well. The cost is that an unrelated attribute reply landing between a read's request and its reply skips the raise; in that case the clamp still keeps the zeros off the wire, and the next read of the folio raises the size. The retract of a speculative extension after a short cached write only fires when i_size still equals the claimed end, so a size raised past it by a read is left alone. Only the DLM + writeback-cache path is changed. Without DLM the writeback cache is single-client and i_size is authoritative; without the writeback cache the server's size is applied on every attribute reply. Tested on a 6.17.13 lockdep build with two passthrough_hp mounts, --dlm --nopassthrough, over one backing directory, replaying the sequence above: mount A writes [0, 4196) and syncs, drops its cache in place of the revoke, mount B writes [4196, 12288) and syncs, A rewrites [4096, 4196) and syncs, extends the file by one byte, reads [4096, 8192) buffered, and rewrites [4096, 4196) once more. With the previous commit alone the buffered read returns 3996 zero bytes from the cache and the final writeback puts the same zeros on the server; with this commit both return B's bytes. Lockdep is silent. Signed-off-by: Allison Henderson <allison.henderson@ddn.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
|
@achhenderson yes ... we had THREE PRs with almost the same fix ... I find that hillarious ;-) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Follow-up to #264 for RED-51747. Stacked on
red-51747-clamp-writeback-eof-6.17; it carries one commit of its own and should retarget toredfs-ubuntu-hwe-6.17.0-16.16-24.04.1once #264 merges.Problem. Under DLM with the writeback cache,
i_sizeis this node's view of the file. Bytes another node wrote past it are "past EOF" to all generic code, even after a read has brought them into the page cache. The read-modify-write read of a cached write does exactly that: the lower writer in the RED-51747 pattern reads the shared page (correct bytes), copies its own bytes in, and on writebackiomap_writeback_handle_eof()zeroes the folio from the locali_sizeto the page end. #264 keeps those zeros off the wire, but they stay in the clean, uptodate folio. Once a later local write raisesi_sizepast the page, a buffered read on this node returns the zeros from the cache, and if the node dirties the page again the whole block is sent and the zeros reach the server after all. This is the open question at the end of the #264 description.Approach. A read reply is evidence of the file's size: the server answers short past its EOF, so
num_readbytes atposmean the file reaches at leastpos + num_read, and the grant the read was made under keeps it there. Raisei_sizetopos + num_readwhen a read reply arrives, infuse_do_readfolio()andfuse_readpages_end(), underfi->lock. The folio holding the other node's bytes is then insidei_sizeand neitheriomap_writeback_handle_eof()noriomap_block_needs_zeroing()touches it. The clamp from #264 stays as the second guard.Safety / trade-offs. The update follows
fuse_read_update_size(): it bumpsattr_version, is skipped when the attributes moved on since the read was sent (attr_ver < fi->attr_version), and is skipped whileFUSE_I_SIZE_UNSTABLEis set, so a reply issued before a truncate cannot undo it. The O_TRUNC open path bumpsattr_versionbefore emptying the cache, so it is covered by the same check. The cost is that an unrelated attribute reply landing between a read's request and its reply skips the raise; the clamp still keeps the zeros off the wire in that case and the next read of the folio raises the size. Only thefc->dlm && fc->writeback_cachepath changes.One assumption to confirm on the server side: a remote shrink must revoke grants above the new size before the new size is visible (which
fuse_attr_cache_mask()already relies on). If that does not hold, a read reply could raisei_sizeover a truncated-away region.JIRA Ticket:
RED-51747
Complexity:
Testing:
Deterministic two-mount replay of the residual case, on a 6.17.13 lockdep build (
CONFIG_PROVE_LOCKING=y) from this branch, running on an Ubuntu 24.04 VM. Twopassthrough_hp --dlm --nopassthroughmounts (A, B) over one backing directory, so the kernel runs the DLM writeback-cache path against a single-node mock server:[0, 4196)and fsyncs. Page P =[4096, 8192)holds 100 bytes of A's data; A'si_sizeis 4196.posix_fadvise(DONTNEED)+drop_caches), standing in for the DLM revoke.[4196, 12288)and fsyncs. The backing file is 12288 bytes.i_sizepast P), then reads[4096, 8192)buffered.Only
fuse.kowas swapped between runs, on the same kernel:fuse.kobuilt fromC x100, \0 x3996C x100, B x3996Stable across repeated runs and flipped with the module each time. No lockdep reports. Steps 1 to 4 pass with both modules, i.e. the #264 clamp holds and this change does not alter it.
Not yet done: the brlock reproducer on a cluster (needs #264's
clamp1run as the baseline), and a truncate-vs-read check on a real server for the shrink assumption above.Other Notes:
Review of #264 and this proposal: RED-51747 comment 10490229.