Re: [PATCH] nfs: set NFS_FSDATA_BLOCKED before invalidating d_seq

[NeilBrown]

On Sat, 24 Oct 2026, Jonathan Flynn wrote:
> block_revalidate() calls write_seqcount_invalidate(&dentry->d_seq) and
> only then sets dentry->d_fsdata to NFS_FSDATA_BLOCKED.  Because ->d_seq
> is invalidated before NFS_FSDATA_BLOCKED is set, an RCU-walk open can
> get past an unlink that is in progress.
> 
> The barrier in write_seqcount_invalidate() is issued before the
> increment, so it orders only the stores made before the call.  An
> RCU-walk lookup that samples ->d_seq after the increment can still read
> the old ->d_fsdata, and so ->d_revalidate() accepts the dentry.  The
> walker then waits for ->d_lock in lockref_get_not_dead(), takes its
> reference, and passes the ->d_seq recheck in __legitimize_path(),
> because ->d_seq still holds the value the walker sampled.  By then
> nfs_unlink() has seen d_count() == 1 and sends the REMOVE instead of
> silly-renaming, so the open proceeds on a file the server is removing.
> 
> Set NFS_FSDATA_BLOCKED first, then call write_seqcount_invalidate().  A
> walker that samples the new ->d_seq then also sees NFS_FSDATA_BLOCKED
> and returns -ECHILD.  A walker that sampled the old ->d_seq fails the
> recheck and retries in ref-walk, which waits for the unlink to finish.
> __d_drop() follows the same order: unhash the dentry, then invalidate
> ->d_seq.
> 
> Also correct the comment in block_revalidate(): the ->d_seq recheck is
> in __legitimize_path() and try_to_unlazy_next(), not in
> legitimize_mnt().
> 
> Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
> ---
> Neil, this answers Sashiko's second point on 1/9: v3 still calls
> write_seqcount_invalidate() before it sets NFS_FSDATA_BLOCKED.  This
> patch applies on top of v3; fold it into 1/9 if you prefer.
> 
> The window with the v3 order:
> 
>   CPU0: nfs_unlink(), ->d_lock held     CPU1: RCU-walk open, same name
>   d_count() == 1, no silly-rename
>   write_seqcount_invalidate(&d_seq)
>                                         __d_lookup_rcu() samples the
>                                           new ->d_seq
>                                         ->d_revalidate() reads ->d_fsdata,
>                                           not yet NFS_FSDATA_BLOCKED
>   d_fsdata = NFS_FSDATA_BLOCKED
>   spin_unlock(&dentry->d_lock)
>   REMOVE sent                           lockref_get_not_dead() takes a ref
>                                         read_seqcount_retry() passes
>                                         the open proceeds

Thanks. This diagram is really helpful.

I think I will move the d_fsdata = NFS_FSDATA_BLOCKED assignment before
the comment and maybe add a bit of explanation.

Thanks for helping me understand this problem.

NeilBrown


> 
> Your afs 1/5 also sets DCACHE_BLOCKED after write_seqcount_invalidate().
> 
>  fs/nfs/dir.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 53cd3adf3ffe..79331e0fa044 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1879,15 +1879,20 @@ static void block_revalidate(struct dentry *dentry)
>  	/* Any new reference that could lead to an open
>  	 * will either:
>  	 *  - take ->d_lock in lookup_open() -> d_lookup() or
> -	 *  - will check d_seq in legitimize_mnt()
> +	 *  - will check d_seq in __legitimize_path() or try_to_unlazy_next()
>  	 *
>  	 * Holding this lock and invalidating ->d_seq ensures we cannot
>  	 * race with __nfs_lookup_revalidate().
>  	 */
>  	lockdep_assert_held(&dentry->d_lock);
> -	write_seqcount_invalidate(&dentry->d_seq);
> -
>  	dentry->d_fsdata = NFS_FSDATA_BLOCKED;
> +
> +	/*
> +	 * Set NFS_FSDATA_BLOCKED before invalidating ->d_seq: the barrier in
> +	 * write_seqcount_invalidate() orders only earlier stores, so an
> +	 * RCU-walk that samples the new ->d_seq also sees NFS_FSDATA_BLOCKED.
> +	 */
> +	write_seqcount_invalidate(&dentry->d_seq);
>  }
>  
>  static void unblock_revalidate(struct dentry *dentry)
> -- 
> 2.43.0
> 
>