Skip to content

Linux 6.17: fix inode creation/invalidation race - #245

Merged
hbirth merged 7 commits into
DDNStorage:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1from
hbirth:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
Oct 10, 2026
Merged

hbirth merged 7 commits into
DDNStorage:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1from
hbirth:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1

Conversation

@hbirth

@hbirth hbirth commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

note that the most probable cause for the reported problem was the readdir cache on the dentry

hbirth added 6 commits October 9, 2026 11:40
Convert space indentation to tabs, put block comment delimiters on
their own lines, align continuation lines to the open parenthesis and
drop the double dash as comment punctuation.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_reverse_inval_inode() drops the invalidation when the inode is not
in the icache: fuse_ilookup() misses and we return -ENOENT without
recording anything.

This loses invalidations that race with inode creation:

 1. fuse_lookup_name() samples attr_version/evict_ctr and sends LOOKUP
 2. the server builds the reply, the request completes, but the
    requesting task has not yet run fuse_iget()
 3. the inode changes on the server, which sends
    FUSE_NOTIFY_INVAL_INODE
 4. fuse_reverse_inval_inode() misses in the icache and the
    invalidation is dropped
 5. fuse_iget() hashes the inode and installs the pre-invalidation
    attributes: the new inode has attr_version == 0 so the staleness
    check in fuse_change_attributes_i() cannot reject them, and
    evict_ctr is unchanged so fuse_change_attributes_common() clears
    inval_mask

The stale attributes are then served for the whole attribute timeout.

Commit 03eacfd ("fuse: fix inode initialization race") closed the
window where the inode is already hashed but not yet initialized; this
closes the remaining window before the inode is hashed (including the
stale-inode retry in fuse_iget() between remove_inode_hash() and
re-insertion).

Fix it by reusing the evict_ctr mechanism: a missed invalidation bumps
the counter, which makes an in-flight LOOKUP/READDIRPLUS reply leave
the new inode's attributes invalid (fresh inodes start with
inval_mask == ~0), forcing a GETATTR on first use.  Retry the lookup
after bumping: ilookup5() and iget5_locked() serialize on the inode
hash lock, so if the retry still misses, the creator inserted after our
lookup and is guaranteed to observe the bumped counter; if it finds the
inode, the normal found-path serializes via attr_version.

Note the scope and cost of this:

 - Only the attribute side is protected.  The page cache needs nothing
   here (a not-yet-created inode has no pages), but the dentry
   invalidation done on the found path (fc->inval_inode_entries /
   fc->expire_inode_entries) cannot be applied to a dentry that does
   not exist yet, so a racing lookup still instantiates its dentry
   with the reply's full entry timeout.  Closing that needs the
   fc->epoch scheme from the upstream fuse tree (see 2396356
   "fuse: add more control over cache invalidation behaviour" and
   64becd2 "fuse: new work queue to invalidate dentries from old
   epochs") and is left for a separate change.

 - The counter is connection-global, so an invalidation for an
   uncached nodeid costs every concurrently created inode one extra
   GETATTR on first use.  Invalidations for cached inodes (the common
   notify-storm case) take the found path and are unaffected.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_create_open() and create_new_entry() pass evict_ctr = 0 to
fuse_iget(), which makes fuse_change_attributes_common() unconditionally
clear inval_mask.  A reverse invalidation for the new nodeid that
arrives between the server generating the CREATE/MKNOD/LINK reply and
fuse_iget() hashing the inode misses the icache and is dropped, and the
reply then installs the pre-invalidation attributes as valid - the same
lost-invalidation race just fixed for LOOKUP/READDIRPLUS, which these
paths are currently exempt from.

Sample evict_ctr before sending the request and pass it through, like
fuse_lookup_name() does.  A missed invalidation then leaves the new
inode's attributes invalid, forcing a GETATTR on first use.

attr_version deliberately stays 0.  For a new inode the staleness check
in fuse_change_attributes_i() cannot reject anything anyway
(fi->attr_version == 0), and for an existing inode (a FUSE_LINK target,
or FUSE_CREATE resolving to an already-cached nodeid) the unconditional
install is self-correcting: it bumps fi->attr_version, so any
later-processed reply carrying older attributes is rejected.  Passing a
pre-request snapshot instead would invert that - a concurrent reply
processed after the snapshot but carrying older server state would
cause the fresh CREATE/LINK reply (e.g. the incremented nlink) to be
silently dropped while the stale attributes stay valid.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
Commit 03eacfd ("fuse: fix inode initialization race") closed the
window where fuse_reverse_inval_inode() finds a hashed but not yet
initialized inode by making it wait on a waitqueue until
fi->attr_version becomes non-zero.

The backport of upstream commit 95bb492f26e7 ("fuse: fix inode
initialization race") closes the same window structurally: fuse_iget()
now keeps the inode I_NEW until the first attribute install completes,
and ilookup5() waits for I_NEW to clear, so fuse_ilookup() can no
longer return an inode with attr_version == 0.  The wait loop is dead
code; remove it together with the waitqueue.

This also removes an uninterruptible sleep from the /dev/fuse notify
path, where a server thread blocked on client task progress, and a
wake_up_all() from every attribute install.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_change_attributes_common() and fuse_change_attributes_common_sx()
carry a verbatim copy of the inval_mask-clearing predicate and its
explanatory comment, which have to be edited in lockstep whenever the
race protection changes.

Move both into a helper, fuse_may_validate_attrs(), and while at it
bring the rationale up to date:

 - evict_ctr is now also bumped by fuse_reverse_inval_inode() when an
   invalidation misses the icache, so the guard protects against
   missed reverse invalidations as well as racing evicts

 - the '!evict_ctr -> this is create' clause was stale: since the
   create paths sample evict_ctr, a zero means the update does not
   come from fuse_iget() at all (matching the upstream wording)

No functional change.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_dentry_revalidate() sends its LOOKUP holding only dget_parent(),
not the parent's i_rwsem.  fuse_reverse_inval_entry() can therefore
clear the entry timeout while the request is in flight, and the reply
re-arms it, undoing the invalidation.  For FUSE_EXPIRE_ONLY and for
fuse_prune_aliases() the dentry also stays hashed, so the expiry is lost
outright and the stale entry survives a full entry_valid period.

The other lookup paths - ->lookup(), ->atomic_open(), create_new_entry()
and readdirplus - all run under the parent's i_rwsem, which
fuse_reverse_inval_entry() takes exclusively, so they are already
ordered against the invalidation and need no guard.

Sample the parent's i_version - the counter fuse_dir_changed() already
bumps and the readdir cache already samples - before sending the
request, and skip fuse_change_entry_timeout() if it moved.  Add
fuse_entry_invalidated() to bump it from the invalidation sites that
expire entries without going through fuse_dir_changed(), i.e.
fuse_prune_aliases() and fuse_invalidate_inode_entry().

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
@hbirth hbirth changed the title Linux 6.17: fix the inode creation/invalidation race Linux 6.17: fix inode creation/invalidation race Oct 9, 2026
@hbirth
hbirth requested review from hazhou-ddn and yongzech October 9, 2026 11:47
The statx path sets inode->i_blkbits from the server's blksize; the
getattr path records it in fi->cached_i_blkbits and leaves i_blkbits
at the page size iomap was set up with.  With a block larger than the
page, iomap's write begin zero-fills the head of an unaligned write's
first page after reading it, and writeback sends the zeros.

Record it the same way on the statx path.

Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>

@achhenderson achhenderson left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks! It intersects a little with pr-247, so we might need a rebase, but otherwise i think its good. thank you

@achhenderson achhenderson left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(duplicate submission of the review above, please ignore)

@hbirth
hbirth merged commit 94d0223 into DDNStorage:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1 Oct 10, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants