fuse: re-validate the DLM grant after waiting on the coherency gate - #191
Open
hbirth wants to merge 5 commits into
Open
Conversation
Collaborator
Author
|
@achhenderson @hazhou-ddn the key was to create a way to query whether we have a region marked as exclusive (this was not complete before since we never needed it) ... now we can operate in a loop ... if we lost the dlm lock we can reacquire it without holding the semaphore. This still has a small problem, where we probably acquire a lock twice when two writer were blocked by the notification ... but I think our code can handle that |
Collaborator
Author
|
the second commit I have included because my kunit tests have found those problems. I have not included the AI generated tests, since putting them in the fs/fuse/ directory would make them part of redfs.ko and I don't think that's a good idea |
hazhou-ddn
reviewed
Jul 27, 2026
hbirth
force-pushed
the
redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
branch
from
July 27, 2026 17:30
ac86c7d to
e8231d1
Compare
Collaborator
Author
|
@hazhou-ddn OK, done |
hbirth
force-pushed
the
redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
branch
from
July 27, 2026 18:21
e8231d1 to
d4ff3ce
Compare
hazhou-ddn
previously approved these changes
Jul 28, 2026
hazhou-ddn
left a comment
hazhou-ddn
left a comment
There was a problem hiding this comment.
After addressing Alison and my comments, all others look good to me.
fuse_dlm_try_merge() locates the first merge candidate by walking from rb_first_cached() until it reaches the region just granted. The walk runs under the write-held cache rwsem on every fuse_dlm_lock_range() call, and the tree it walks holds every cached grant of the inode. Strided writers (IOR hard-write) accumulate grants that cannot merge with each other, so the tree keeps growing and every new grant pays a scan of all grants below it -- quadratic over the run, with fuse_dlm_range_is_locked() readers blocked behind each scan. Seed the merge with fuse_page_it_iter_first() on the region widened by one unit to each side instead; finding the lowest overlapping range is what the interval tree is there for. This also repairs two edge cases of the linear scan: a region starting at offset 0 made 'start - 1' wrap so the scan degenerated and merging was silently skipped, and a region ending at U64_MAX overflowed 'end + 1' in the loop bound, ending the merge after the first range. Both bounds now saturate. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
Both cached IO paths request their DLM lock first and then go to sleep on things a NOTIFY invalidate can be holding: the read path blocks on the coherency gate (writer priority), the write path additionally sleeps on a contended i_rwsem. A NOTIFY invalidate running in that window revokes exactly the lock just granted (fuse_dlm_unlock_range()), so the task wakes up and populates or dirties the page cache with no DLM coverage. Close the window without ever sending a FUSE_DLM_WB_LOCK request while holding the gate (a grant that had to wait on an invalidate delivered to this same client would deadlock against our own gate hold): - Drop the lock record under the gate write side in fuse_reverse_inval_inode(), so revocation and page drop are one atomic step with respect to the gate. - After entering the gate read side, re-check the grant against the live lock tree; if it was revoked while we waited, drop the gate, re-request, re-enter and check again. With the revoke now gated, passing the check means the lock cannot go away for the whole gate hold: a revoke arriving mid-operation parks until the IO is done. - Keep the write path's lock request ahead of the inode lock: the round trip must not capture the writer-priority i_rwsem for unbounded cluster-grant latency, and the in-gate re-validation already closes the grant-to-use window. Only O_APPEND moves below the lock, because its range is the current EOF -- stable only under the exclusive inode lock. This also fixes the append range itself: generic_write_checks() rewrites ki_pos to i_size for IOCB_APPEND, so the old 'i_size + ki_pos' double-counted (ki_pos is absolute, not relative) and locked a range disjoint from where the data lands. fuse_get_dlm_lock() now reports whether the grant is recorded, and the re-validation never re-requests a grant that failed, so it cannot spin (the read path seeds this from its pre-gate request instead of discarding that result). A grant the server issued but that could not be recorded (small-allocation -ENOMEM) reports FUSE_DLM_GRANT_UNRECORDED: coverage exists cluster-wide, so failing the IO would be wrong -- it proceeds, it just cannot re-validate. Empty ranges are trivially held, so a zero-length IO neither sends a doomed request nor spins in the retry loops. The write path returns a real failure to the caller instead of dirtying the cache without DLM coverage; only -ENOSYS still degrades to a plain cached write, since it means the server has no DLM at all and clears fc->dlm. The read path keeps falling through unlocked and additionally bounds its retry: a reader-only inode has no force-DIO latch to end a revoke storm, so after a few re-requests the read is served unlocked rather than looping in the kernel for the duration of the storm. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
The NOTIFY_INVAL_INODE revoke computed
fuse_dlm_unlock_range(fi, offset, pg_end == -1 ? 0 : offset + len - 1)
which is wrong at both degenerate ends: a to-EOF invalidate (len <= 0,
e.g. a remote truncate) with offset > 0 becomes the inverted range
[offset, 0] and removes nothing, so the revoked grant stays visible to
the re-validating IO paths forever -- cached writes with no
server-side lock, zero-filled RMW reads; and an invalidate of byte 0
(offset 0, len 1) becomes [0, 0], the "destroy everything" sentinel,
wiping every grant of the inode.
Map the range in one helper shared by the gated and the ungated
branch: to-EOF revokes through U64_MAX, and the bounds widen to page
boundaries to match how grants are recorded -- revoking too much only
costs a re-request, too little leaves a stale grant.
Drop the in-band (0, 0) sentinel: whole-file invalidates walk the
normal removal path, release-all is fuse_dlm_cache_release_locks(),
and an inverted range is rejected with -EINVAL instead of silently
ignored.
Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
…y gate Two revocation paths bypassed the revoke-under-gate invariant the IO paths re-validate against: - fuse_reverse_inval_inode() skipped the gate once mapping_mapped() turned true, revoking concurrently with gate holders -- but fuse_cache_read_iter()/fuse_cache_write_iter() enter the gate unconditionally, so a single mmap() reopened the race. Keep the gate for mmapped inodes; only the force-DIO latch stays disabled for them (a mapping needs the page cache, and fuse_file_mmap() reverts any latch it races with). - The local truncates in fuse_do_setattr() -- the atomic-O_TRUNC open shortcut and the after-setattr trim -- revoked and dropped the cache with no gate at all, so an already re-validated reader could repopulate the truncated range. Take the gate write side around revoke + drop. This cannot deadlock: both run under exclusive i_rwsem, which no gate holder waits on (the write path takes i_rwsem before the gate, the read path never takes it). Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
A FUSE_DLM_WB_LOCK reply and a NOTIFY invalidate are serviced on different threads, so a revoke aimed at the grant a reply carries can be processed before fuse_get_dlm_lock() records it: the revoke finds nothing to remove, and the requester then records an already-dead grant that no later NOTIFY will target -- a permanent false positive for the re-validating IO paths. Add a revocation generation to the lock cache, bumped under the cache lock by every revoke path -- unconditionally, because the racing revoke sees an empty overlap precisely when the grant is in flight. fuse_get_dlm_lock() samples it before sending and records through fuse_dlm_lock_range_gen(), which refuses with -EAGAIN once the generation has moved; the grant is then re-requested instead of recorded, bounded so a revoke storm cannot pin the IO here (past the bound the failure reports like any request failure). Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
hbirth
force-pushed
the
redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
branch
from
July 28, 2026 09:29
d4ff3ce to
8983871
Compare
Collaborator
Author
|
had AI do a review of this change and it came up with a couple of improvements ... one a nice race (8983871), that I'm not sure we have mitigated |
Collaborator
Author
|
can we merge this? |
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
3 participants
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.
Both cached IO paths request their DLM lock first and then go to sleep on things a NOTIFY invalidate can be holding: the read path blocks on the coherency gate (writer priority), the write path additionally sleeps on a contended i_rwsem. A NOTIFY invalidate running in that window revokes exactly the lock just granted (fuse_dlm_unlock_range()), so the task wakes up and populates or dirties the page cache with no DLM coverage.
Close the window without ever sending a FUSE_DLM_WB_LOCK request while holding the gate (a grant that had to wait on an invalidate delivered to this same client would deadlock against our own gate hold):
Drop the lock record under the gate write side in fuse_reverse_inval_inode(), so revocation and page drop are one atomic step with respect to the gate.
After entering the gate read side, re-check the grant against the live lock tree; if it was revoked while we waited, drop the gate, re-request, re-enter and check again. With the revoke now gated, passing the check means the lock cannot go away for the whole gate hold: a revoke arriving mid-operation parks until the IO is done.
Move the write path's lock request below the inode lock, so the sleep on i_rwsem no longer sits between grant and use. This also computes the O_APPEND lock range against a stable i_size instead of a racy lockless read.
fuse_get_dlm_lock() now reports whether the grant is recorded so the re-validation loop can fall through unlocked on a persistent request failure, preserving the old degraded behavior instead of spinning.