[PATCH v2] nfsd: fix file change detection in CB_GETATTR

[PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

RFC 8881, section 10.4.3 doesn't say anything about caching the file
size in the delegation record, nor does it say anything about comparing
a cached file size with the size reported by the client in the
CB_GETATTR reply for the purpose of determining if the client holds
modified data for the file.

What section 10.4.3 of RFC 8881 does say is that the server should
compare the *current* file size with size reported by the client holding
the delegation in the CB_GETATTR reply, and if they differ to treat it
as a modification regardless of the change attribute retrieved via the
CB_GETATTR.

Doing otherwise would cause the server to believe the client holding the
delegation has a modified version of the file, even if the client
flushed the modifications to the server prior to the CB_GETATTR.  This
would have the added side effect of subsequent CB_GETATTRs causing
updates to the mtime, ctime, and change attribute even if the client
holding the delegation makes no further updates to the file.

Modify nfsd4_deleg_getattr_conflict() to obtain the current file size
via vfs_getattr().  Retain the ncf_cur_fsize field, since it's a
convenient way to return the file size back to nfsd4_encode_fattr4(),
but don't use it for the purpose of detecting file changes.

Also, if we recall the delegation (because the client didn't respond to
the CB_GETATTR), then skip the logic that checks the nfs4_cb_fattr
fields.

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
This patch is against Chuck's nfsd-testing branch.

A pynfs test that illustrates the issue is available here (delegated
timestamps must be disabled to make the test fail):
https://lore.kernel.org/linux-nfs/20260404003050.1560149-6-smayhew@redhat.com/T/#u

v2 changes:
- update kerneldoc comment for nfsd4_deleg_getattr_conflict()
- relocated declarations in nfsd4_deleg_getattr_conflict() to maintain
  reverse-xmas tree ordering
- pass the struct kstat from the nfsd4_fattr_args to
  nfsd4_deleg_getattr_conflict() instead of creating another one on the
  stack
- only call vfs_getattr() in nfsd4_deleg_getattr_conflict() for the
  !ncf_file_modified case (once the file has been flagged as modified,
  it remains so until the delegation is returned or revoked, so further
  calls to vfs_getattr() are unnecessary)
- if we recall the delegation (because the client didn't respond to the
  CB_GETATTR), then skip the CB_GETATTR comparison logic

Chuck - to test the last part I hacked a pynfs test to force a CB_RECALL
by not responding to the CB_GETATTR in a timely manner.  But nfsd has a
pretty aggressive timeout for the DELEGRETURN (30ms) before it responds
to the client that issued the GETATTR with NFS4ERR_DELAY.  On the system
I'm testing on, it looks like it's taking just over 30ms on average:

            nfsd-2971    [059] ...1.   451.007305: nfsd_cb_recall: addr=127.0.0.1:0 client 69d047c9:03b2de67 stateid 00000002:00000001
            nfsd-2971    [059] .....   451.037306: nfsd_delegret_wakeup: xid=0x16c768b1 inode=000000000a5c53d9 (timed out)
            nfsd-2971    [059] .....   451.037461: nfsd_deleg_return: client 69d047c9:03b2de67 stateid 00000002:00000001

So I bumped the timeout to 50ms (just in my test kernel) and it appears
to do the right thing... I'm just not sure how often it'll actually come
in play in normal usage.

 fs/nfsd/nfs4state.c | 35 ++++++++++++++++++++++++-----------
 fs/nfsd/nfs4xdr.c   |  3 ++-
 fs/nfsd/state.h     |  3 ++-
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fa657badf5f8..53d8e7e7d60b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9444,7 +9444,9 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
 /**
  * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict
  * @rqstp: RPC transaction context
- * @dentry: dentry of inode to be checked for a conflict
+ * @path: used to get the inode and size of the file to be checked for a
+ * 	  conflict
+ * @stat: used to get the size of the file to be checked for a conflict
  * @pdp: returned WRITE delegation, if one was found
  *
  * This function is called when there is a conflict between a write
@@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct dentry *dentry, struct nfs4_delegation
  * caller must put the reference.
  */
 __be32
-nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
-			     struct nfs4_delegation **pdp)
+nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
+			     struct kstat *stat, struct nfs4_delegation **pdp)
 {
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct nfsd_thread_local_info *ntli = rqstp->rq_private;
+	struct inode *inode = d_inode(path->dentry);
 	struct file_lock_context *ctx;
 	struct nfs4_delegation *dp = NULL;
 	struct file_lease *fl;
 	struct nfs4_cb_fattr *ncf;
-	struct inode *inode = d_inode(dentry);
 	__be32 status;
+	int err;
 
 	ctx = locks_inode_context(inode);
 	if (!ctx)
@@ -9516,20 +9519,30 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
 		if (status != nfserr_jukebox ||
 		    !nfsd_wait_for_delegreturn(rqstp, inode))
 			goto out_status;
+		status = nfs_ok;
+		goto out_status;
+	}
+	if (!ncf->ncf_file_modified) {
+		if (ncf->ncf_initial_cinfo != ncf->ncf_cb_change) {
+			ncf->ncf_file_modified = true;
+		} else {
+			err = vfs_getattr(path, stat, STATX_SIZE,
+					  AT_STATX_SYNC_AS_STAT);
+			if (err) {
+				status = nfserrno(err);
+				goto out_status;
+			}
+			if (stat->size != ncf->ncf_cb_fsize)
+				ncf->ncf_file_modified = true;
+		}
 	}
-	if (!ncf->ncf_file_modified &&
-	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
-	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
-		ncf->ncf_file_modified = true;
 	if (ncf->ncf_file_modified) {
-		int err;
-
 		/*
 		 * Per section 10.4.3 of RFC 8881, the server would
 		 * not update the file's metadata with the client's
 		 * modified size
 		 */
-		err = cb_getattr_update_times(dentry, dp);
+		err = cb_getattr_update_times(path->dentry, dp);
 		if (err) {
 			status = nfserrno(err);
 			goto out_status;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2a0946c630e1..5e09682c8135 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3914,7 +3914,8 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 	    (attrmask[1] & (FATTR4_WORD1_TIME_ACCESS |
 			    FATTR4_WORD1_TIME_MODIFY |
 			    FATTR4_WORD1_TIME_METADATA))) {
-		status = nfsd4_deleg_getattr_conflict(rqstp, dentry, &dp);
+		status = nfsd4_deleg_getattr_conflict(rqstp, &path, &args.stat,
+						      &dp);
 		if (status)
 			goto out;
 	}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 811c148f36fc..4fa6329c75b4 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -904,7 +904,8 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
 }
 
 extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
-		struct dentry *dentry, struct nfs4_delegation **pdp);
+		struct path *path, struct kstat *stat,
+		struct nfs4_delegation **pdp);
 
 struct nfsd4_get_dir_delegation;
 struct nfs4_delegation *nfsd_get_dir_deleg(struct nfsd4_compound_state *cstate,
-- 
2.53.0

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Chuck Lever]

On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> RFC 8881, section 10.4.3 doesn't say anything about caching the file
> size in the delegation record, nor does it say anything about comparing
> a cached file size with the size reported by the client in the
> CB_GETATTR reply for the purpose of determining if the client holds
> modified data for the file.
>
> What section 10.4.3 of RFC 8881 does say is that the server should
> compare the *current* file size with size reported by the client holding
> the delegation in the CB_GETATTR reply, and if they differ to treat it
> as a modification regardless of the change attribute retrieved via the
> CB_GETATTR.
>
> Doing otherwise would cause the server to believe the client holding the
> delegation has a modified version of the file, even if the client
> flushed the modifications to the server prior to the CB_GETATTR.  This
> would have the added side effect of subsequent CB_GETATTRs causing
> updates to the mtime, ctime, and change attribute even if the client
> holding the delegation makes no further updates to the file.
>
> Modify nfsd4_deleg_getattr_conflict() to obtain the current file size
> via vfs_getattr().  Retain the ncf_cur_fsize field, since it's a
> convenient way to return the file size back to nfsd4_encode_fattr4(),
> but don't use it for the purpose of detecting file changes.
>
> Also, if we recall the delegation (because the client didn't respond to
> the CB_GETATTR), then skip the logic that checks the nfs4_cb_fattr
> fields.
>
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---

> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fa657badf5f8..53d8e7e7d60b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c

> @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> dentry *dentry, struct nfs4_delegation
>   * caller must put the reference.
>   */
>  __be32
> -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> *dentry,
> -			     struct nfs4_delegation **pdp)
> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> +			     struct kstat *stat, struct nfs4_delegation **pdp)

Passing the kstat struct in saves some stack just as I suggested,
but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
though -- did you happen to measure how deep it gets after this patch
is applied?


>  {
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	struct nfsd_thread_local_info *ntli = rqstp->rq_private;
> +	struct inode *inode = d_inode(path->dentry);
>  	struct file_lock_context *ctx;
>  	struct nfs4_delegation *dp = NULL;
>  	struct file_lease *fl;
>  	struct nfs4_cb_fattr *ncf;
> -	struct inode *inode = d_inode(dentry);
>  	__be32 status;
> +	int err;
> 
>  	ctx = locks_inode_context(inode);
>  	if (!ctx)

-- 
Chuck Lever

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

On Sat, 04 Apr 2026, Chuck Lever wrote:

> 
> 
> On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> > RFC 8881, section 10.4.3 doesn't say anything about caching the file
> > size in the delegation record, nor does it say anything about comparing
> > a cached file size with the size reported by the client in the
> > CB_GETATTR reply for the purpose of determining if the client holds
> > modified data for the file.
> >
> > What section 10.4.3 of RFC 8881 does say is that the server should
> > compare the *current* file size with size reported by the client holding
> > the delegation in the CB_GETATTR reply, and if they differ to treat it
> > as a modification regardless of the change attribute retrieved via the
> > CB_GETATTR.
> >
> > Doing otherwise would cause the server to believe the client holding the
> > delegation has a modified version of the file, even if the client
> > flushed the modifications to the server prior to the CB_GETATTR.  This
> > would have the added side effect of subsequent CB_GETATTRs causing
> > updates to the mtime, ctime, and change attribute even if the client
> > holding the delegation makes no further updates to the file.
> >
> > Modify nfsd4_deleg_getattr_conflict() to obtain the current file size
> > via vfs_getattr().  Retain the ncf_cur_fsize field, since it's a
> > convenient way to return the file size back to nfsd4_encode_fattr4(),
> > but don't use it for the purpose of detecting file changes.
> >
> > Also, if we recall the delegation (because the client didn't respond to
> > the CB_GETATTR), then skip the logic that checks the nfs4_cb_fattr
> > fields.
> >
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fa657badf5f8..53d8e7e7d60b 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> 
> > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > dentry *dentry, struct nfs4_delegation
> >   * caller must put the reference.
> >   */
> >  __be32
> > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > *dentry,
> > -			     struct nfs4_delegation **pdp)
> > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> 
> Passing the kstat struct in saves some stack just as I suggested,
> but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> though -- did you happen to measure how deep it gets after this patch
> is applied?
> 

I tried using the stack tracer:

# echo 1 >/proc/sys/kernel/stack_tracer_enabled
# echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
# cat /sys/kernel/debug/tracing/stack_trace
        Depth    Size   Location    (18 entries)
        -----    ----   --------
  0)     1936      48   vfs_getattr+0x9/0x50
  1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
  2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
  3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
  4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
  5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
  6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
  7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
  8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
  9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
 10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
 11)      536      40   svc_process+0x150/0x240 [sunrpc]
 12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
 13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
 14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
 15)      288      56   kthread+0xe3/0x120
 16)      232      40   ret_from_fork+0x1a1/0x270
 17)      192     192   ret_from_fork_asm+0x1a/0x30

But that's capturing a vfs_getattr() call from encoding a readdir reply,
rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().

Here's the stack depth for nfsd4_deleg_getattr_conflict():

# echo nfsd4_deleg_getattr_conflict >/sys/kernel/debug/tracing/stack_trace_filter
# echo 0 >/sys/kernel/debug/tracing/stack_max_size 
# cat /sys/kernel/debug/tracing/stack_trace
        Depth    Size   Location    (14 entries)
        -----    ----   --------
  0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
  1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
  2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
  3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
  4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
  5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
  6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
  7)      536      40   svc_process+0x150/0x240 [sunrpc]
  8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
  9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
 10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
 11)      288      56   kthread+0xe3/0x120
 12)      232      40   ret_from_fork+0x1a1/0x270
 13)      192     192   ret_from_fork_asm+0x1a/0x30

Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
function (that we can trace) is avc_lookup(), so here's a bpftrace script that
prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
(I mostly punted to Gemini for this):

# cat peak-usage.bt 
kprobe:nfsd4_deleg_getattr_conflict {
    @in_deleg_conflict[tid] = 1;
}

kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
    $stack_size = (uint64)16384; 
    $sp = reg("sp");
    $stack_base = $sp & ~($stack_size - 1);
    $total_used = $stack_base + $stack_size - $sp;

    if ($total_used > @max_depth_bytes) {
        @max_depth_bytes = $total_used;
        @deepest_stack = kstack;
    }
}

kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }

And finally the result:

# bpftrace peak-usage.bt 
Attached 3 probes
^C

@deepest_stack: 
        avc_lookup+1
        avc_has_perm_noaudit+60
        avc_has_perm+89
        selinux_inode_getattr+203
        security_inode_getattr+70
        vfs_getattr+35
        nfsd4_deleg_getattr_conflict+958
        nfsd4_encode_fattr4+401
        nfsd4_encode_getattr+44
        nfsd4_encode_operation+207
        nfsd4_proc_compound+470
        nfsd_dispatch+217
        svc_process_common+1227
        svc_process+336
        svc_handle_xprt+1200
        svc_recv+434
        nfsd+284
        kthread+227
        ret_from_fork+417
        ret_from_fork_asm+26

@max_depth_bytes: 1792

> 
> >  {
> >  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >  	struct nfsd_thread_local_info *ntli = rqstp->rq_private;
> > +	struct inode *inode = d_inode(path->dentry);
> >  	struct file_lock_context *ctx;
> >  	struct nfs4_delegation *dp = NULL;
> >  	struct file_lease *fl;
> >  	struct nfs4_cb_fattr *ncf;
> > -	struct inode *inode = d_inode(dentry);
> >  	__be32 status;
> > +	int err;
> > 
> >  	ctx = locks_inode_context(inode);
> >  	if (!ctx)
> 
> -- 
> Chuck Lever
> 

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Chuck Lever]

On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> On Sat, 04 Apr 2026, Chuck Lever wrote:
>> On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:

>> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> > index fa657badf5f8..53d8e7e7d60b 100644
>> > --- a/fs/nfsd/nfs4state.c
>> > +++ b/fs/nfsd/nfs4state.c
>> 
>> > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
>> > dentry *dentry, struct nfs4_delegation
>> >   * caller must put the reference.
>> >   */
>> >  __be32
>> > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
>> > *dentry,
>> > -			     struct nfs4_delegation **pdp)
>> > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
>> > +			     struct kstat *stat, struct nfs4_delegation **pdp)
>> 
>> Passing the kstat struct in saves some stack just as I suggested,
>> but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
>> though -- did you happen to measure how deep it gets after this patch
>> is applied?
>> 
>
> I tried using the stack tracer:
>
> # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> # cat /sys/kernel/debug/tracing/stack_trace
>         Depth    Size   Location    (18 entries)
>         -----    ----   --------
>   0)     1936      48   vfs_getattr+0x9/0x50
>   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
>   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
>   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
>   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
>   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
>   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
>   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
>   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
>   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
>  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
>  11)      536      40   svc_process+0x150/0x240 [sunrpc]
>  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
>  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
>  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
>  15)      288      56   kthread+0xe3/0x120
>  16)      232      40   ret_from_fork+0x1a1/0x270
>  17)      192     192   ret_from_fork_asm+0x1a/0x30
>
> But that's capturing a vfs_getattr() call from encoding a readdir reply,
> rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
>
> Here's the stack depth for nfsd4_deleg_getattr_conflict():
>
> # echo nfsd4_deleg_getattr_conflict 
> >/sys/kernel/debug/tracing/stack_trace_filter
> # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> # cat /sys/kernel/debug/tracing/stack_trace
>         Depth    Size   Location    (14 entries)
>         -----    ----   --------
>   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
>   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
>   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
>   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
>   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
>   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
>   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
>   7)      536      40   svc_process+0x150/0x240 [sunrpc]
>   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
>   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
>  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
>  11)      288      56   kthread+0xe3/0x120
>  12)      232      40   ret_from_fork+0x1a1/0x270
>  13)      192     192   ret_from_fork_asm+0x1a/0x30
>
> Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> (I mostly punted to Gemini for this):
>
> # cat peak-usage.bt 
> kprobe:nfsd4_deleg_getattr_conflict {
>     @in_deleg_conflict[tid] = 1;
> }
>
> kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
>     $stack_size = (uint64)16384; 
>     $sp = reg("sp");
>     $stack_base = $sp & ~($stack_size - 1);
>     $total_used = $stack_base + $stack_size - $sp;
>
>     if ($total_used > @max_depth_bytes) {
>         @max_depth_bytes = $total_used;
>         @deepest_stack = kstack;
>     }
> }
>
> kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
>
> And finally the result:
>
> # bpftrace peak-usage.bt 
> Attached 3 probes
> ^C
>
> @deepest_stack: 
>         avc_lookup+1
>         avc_has_perm_noaudit+60
>         avc_has_perm+89
>         selinux_inode_getattr+203
>         security_inode_getattr+70
>         vfs_getattr+35
>         nfsd4_deleg_getattr_conflict+958
>         nfsd4_encode_fattr4+401
>         nfsd4_encode_getattr+44
>         nfsd4_encode_operation+207
>         nfsd4_proc_compound+470
>         nfsd_dispatch+217
>         svc_process_common+1227
>         svc_process+336
>         svc_handle_xprt+1200
>         svc_recv+434
>         nfsd+284
>         kthread+227
>         ret_from_fork+417
>         ret_from_fork_asm+26
>
> @max_depth_bytes: 1792

Since the new code only needs the file's size, perhaps you can get
away with

        if (i_size_read(inode) != ncf->ncf_cb_fsize)

rather than

        err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
        if (err) {
                status = nfserrno(err);
                goto out_status;
        }
        if (stat->size != ncf->ncf_cb_fsize)

Then there's no longer a need for a struct kstat at all. The client is
holding a delegation, so I would expect the file size to be stable.


-- 
Chuck Lever

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Jeff Layton]

On Mon, 2026-04-06 at 10:12 -0400, Chuck Lever wrote:
> 
> On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > On Sat, 04 Apr 2026, Chuck Lever wrote:
> > > On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> 
> > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > index fa657badf5f8..53d8e7e7d60b 100644
> > > > --- a/fs/nfsd/nfs4state.c
> > > > +++ b/fs/nfsd/nfs4state.c
> > > 
> > > > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > > > dentry *dentry, struct nfs4_delegation
> > > >   * caller must put the reference.
> > > >   */
> > > >  __be32
> > > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > > > *dentry,
> > > > -			     struct nfs4_delegation **pdp)
> > > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > > > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> > > 
> > > Passing the kstat struct in saves some stack just as I suggested,
> > > but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> > > though -- did you happen to measure how deep it gets after this patch
> > > is applied?
> > > 
> > 
> > I tried using the stack tracer:
> > 
> > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > # cat /sys/kernel/debug/tracing/stack_trace
> >         Depth    Size   Location    (18 entries)
> >         -----    ----   --------
> >   0)     1936      48   vfs_getattr+0x9/0x50
> >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> >  15)      288      56   kthread+0xe3/0x120
> >  16)      232      40   ret_from_fork+0x1a1/0x270
> >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> > 
> > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> > 
> > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> > 
> > # echo nfsd4_deleg_getattr_conflict 
> > > /sys/kernel/debug/tracing/stack_trace_filter
> > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > # cat /sys/kernel/debug/tracing/stack_trace
> >         Depth    Size   Location    (14 entries)
> >         -----    ----   --------
> >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> >  11)      288      56   kthread+0xe3/0x120
> >  12)      232      40   ret_from_fork+0x1a1/0x270
> >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> > 
> > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > (I mostly punted to Gemini for this):
> > 
> > # cat peak-usage.bt 
> > kprobe:nfsd4_deleg_getattr_conflict {
> >     @in_deleg_conflict[tid] = 1;
> > }
> > 
> > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> >     $stack_size = (uint64)16384; 
> >     $sp = reg("sp");
> >     $stack_base = $sp & ~($stack_size - 1);
> >     $total_used = $stack_base + $stack_size - $sp;
> > 
> >     if ($total_used > @max_depth_bytes) {
> >         @max_depth_bytes = $total_used;
> >         @deepest_stack = kstack;
> >     }
> > }
> > 
> > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> > 
> > And finally the result:
> > 
> > # bpftrace peak-usage.bt 
> > Attached 3 probes
> > ^C
> > 
> > @deepest_stack: 
> >         avc_lookup+1
> >         avc_has_perm_noaudit+60
> >         avc_has_perm+89
> >         selinux_inode_getattr+203
> >         security_inode_getattr+70
> >         vfs_getattr+35
> >         nfsd4_deleg_getattr_conflict+958
> >         nfsd4_encode_fattr4+401
> >         nfsd4_encode_getattr+44
> >         nfsd4_encode_operation+207
> >         nfsd4_proc_compound+470
> >         nfsd_dispatch+217
> >         svc_process_common+1227
> >         svc_process+336
> >         svc_handle_xprt+1200
> >         svc_recv+434
> >         nfsd+284
> >         kthread+227
> >         ret_from_fork+417
> >         ret_from_fork_asm+26
> > 
> > @max_depth_bytes: 1792
> 
> Since the new code only needs the file's size, perhaps you can get
> away with
> 
>         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> 
> rather than
> 
>         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
>         if (err) {
>                 status = nfserrno(err);
>                 goto out_status;
>         }
>         if (stat->size != ncf->ncf_cb_fsize)
> 
> Then there's no longer a need for a struct kstat at all. The client is
> holding a delegation, so I would expect the file size to be stable.
> 

I hate relying on the size for this, since it's not a reliable
indicator of change, especially on something that has a fixed size (VM
image, for instance).

In fact, I wonder if this ought to be looking at the mtime too. If the
mtime that the client reported is later than the mtime the server has,
then we can probably infer that there are buffered writes out there.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

On Mon, 06 Apr 2026, Chuck Lever wrote:

> 
> 
> On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > On Sat, 04 Apr 2026, Chuck Lever wrote:
> >> On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> 
> >> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> > index fa657badf5f8..53d8e7e7d60b 100644
> >> > --- a/fs/nfsd/nfs4state.c
> >> > +++ b/fs/nfsd/nfs4state.c
> >> 
> >> > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> >> > dentry *dentry, struct nfs4_delegation
> >> >   * caller must put the reference.
> >> >   */
> >> >  __be32
> >> > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> >> > *dentry,
> >> > -			     struct nfs4_delegation **pdp)
> >> > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> >> > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> >> 
> >> Passing the kstat struct in saves some stack just as I suggested,
> >> but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> >> though -- did you happen to measure how deep it gets after this patch
> >> is applied?
> >> 
> >
> > I tried using the stack tracer:
> >
> > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > # cat /sys/kernel/debug/tracing/stack_trace
> >         Depth    Size   Location    (18 entries)
> >         -----    ----   --------
> >   0)     1936      48   vfs_getattr+0x9/0x50
> >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> >  15)      288      56   kthread+0xe3/0x120
> >  16)      232      40   ret_from_fork+0x1a1/0x270
> >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> >
> > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> >
> > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> >
> > # echo nfsd4_deleg_getattr_conflict 
> > >/sys/kernel/debug/tracing/stack_trace_filter
> > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > # cat /sys/kernel/debug/tracing/stack_trace
> >         Depth    Size   Location    (14 entries)
> >         -----    ----   --------
> >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> >  11)      288      56   kthread+0xe3/0x120
> >  12)      232      40   ret_from_fork+0x1a1/0x270
> >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> >
> > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > (I mostly punted to Gemini for this):
> >
> > # cat peak-usage.bt 
> > kprobe:nfsd4_deleg_getattr_conflict {
> >     @in_deleg_conflict[tid] = 1;
> > }
> >
> > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> >     $stack_size = (uint64)16384; 
> >     $sp = reg("sp");
> >     $stack_base = $sp & ~($stack_size - 1);
> >     $total_used = $stack_base + $stack_size - $sp;
> >
> >     if ($total_used > @max_depth_bytes) {
> >         @max_depth_bytes = $total_used;
> >         @deepest_stack = kstack;
> >     }
> > }
> >
> > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> >
> > And finally the result:
> >
> > # bpftrace peak-usage.bt 
> > Attached 3 probes
> > ^C
> >
> > @deepest_stack: 
> >         avc_lookup+1
> >         avc_has_perm_noaudit+60
> >         avc_has_perm+89
> >         selinux_inode_getattr+203
> >         security_inode_getattr+70
> >         vfs_getattr+35
> >         nfsd4_deleg_getattr_conflict+958
> >         nfsd4_encode_fattr4+401
> >         nfsd4_encode_getattr+44
> >         nfsd4_encode_operation+207
> >         nfsd4_proc_compound+470
> >         nfsd_dispatch+217
> >         svc_process_common+1227
> >         svc_process+336
> >         svc_handle_xprt+1200
> >         svc_recv+434
> >         nfsd+284
> >         kthread+227
> >         ret_from_fork+417
> >         ret_from_fork_asm+26
> >
> > @max_depth_bytes: 1792
> 
> Since the new code only needs the file's size, perhaps you can get
> away with
> 
>         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> 
> rather than
> 
>         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
>         if (err) {
>                 status = nfserrno(err);
>                 goto out_status;
>         }
>         if (stat->size != ncf->ncf_cb_fsize)

Definitely.  I'll send a v3 in a bit.
> 
> Then there's no longer a need for a struct kstat at all. The client is
> holding a delegation, so I would expect the file size to be stable.
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

On Mon, 06 Apr 2026, Jeff Layton wrote:

> On Mon, 2026-04-06 at 10:12 -0400, Chuck Lever wrote:
> > 
> > On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > > On Sat, 04 Apr 2026, Chuck Lever wrote:
> > > > On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> > 
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index fa657badf5f8..53d8e7e7d60b 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > 
> > > > > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > > > > dentry *dentry, struct nfs4_delegation
> > > > >   * caller must put the reference.
> > > > >   */
> > > > >  __be32
> > > > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > > > > *dentry,
> > > > > -			     struct nfs4_delegation **pdp)
> > > > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > > > > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> > > > 
> > > > Passing the kstat struct in saves some stack just as I suggested,
> > > > but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> > > > though -- did you happen to measure how deep it gets after this patch
> > > > is applied?
> > > > 
> > > 
> > > I tried using the stack tracer:
> > > 
> > > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > > # cat /sys/kernel/debug/tracing/stack_trace
> > >         Depth    Size   Location    (18 entries)
> > >         -----    ----   --------
> > >   0)     1936      48   vfs_getattr+0x9/0x50
> > >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> > >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> > >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> > >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> > >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> > >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> > >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> > >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > >  15)      288      56   kthread+0xe3/0x120
> > >  16)      232      40   ret_from_fork+0x1a1/0x270
> > >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> > > 
> > > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> > > 
> > > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> > > 
> > > # echo nfsd4_deleg_getattr_conflict 
> > > > /sys/kernel/debug/tracing/stack_trace_filter
> > > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > > # cat /sys/kernel/debug/tracing/stack_trace
> > >         Depth    Size   Location    (14 entries)
> > >         -----    ----   --------
> > >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> > >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> > >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> > >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> > >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > >  11)      288      56   kthread+0xe3/0x120
> > >  12)      232      40   ret_from_fork+0x1a1/0x270
> > >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> > > 
> > > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > > (I mostly punted to Gemini for this):
> > > 
> > > # cat peak-usage.bt 
> > > kprobe:nfsd4_deleg_getattr_conflict {
> > >     @in_deleg_conflict[tid] = 1;
> > > }
> > > 
> > > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> > >     $stack_size = (uint64)16384; 
> > >     $sp = reg("sp");
> > >     $stack_base = $sp & ~($stack_size - 1);
> > >     $total_used = $stack_base + $stack_size - $sp;
> > > 
> > >     if ($total_used > @max_depth_bytes) {
> > >         @max_depth_bytes = $total_used;
> > >         @deepest_stack = kstack;
> > >     }
> > > }
> > > 
> > > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> > > 
> > > And finally the result:
> > > 
> > > # bpftrace peak-usage.bt 
> > > Attached 3 probes
> > > ^C
> > > 
> > > @deepest_stack: 
> > >         avc_lookup+1
> > >         avc_has_perm_noaudit+60
> > >         avc_has_perm+89
> > >         selinux_inode_getattr+203
> > >         security_inode_getattr+70
> > >         vfs_getattr+35
> > >         nfsd4_deleg_getattr_conflict+958
> > >         nfsd4_encode_fattr4+401
> > >         nfsd4_encode_getattr+44
> > >         nfsd4_encode_operation+207
> > >         nfsd4_proc_compound+470
> > >         nfsd_dispatch+217
> > >         svc_process_common+1227
> > >         svc_process+336
> > >         svc_handle_xprt+1200
> > >         svc_recv+434
> > >         nfsd+284
> > >         kthread+227
> > >         ret_from_fork+417
> > >         ret_from_fork_asm+26
> > > 
> > > @max_depth_bytes: 1792
> > 
> > Since the new code only needs the file's size, perhaps you can get
> > away with
> > 
> >         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> > 
> > rather than
> > 
> >         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
> >         if (err) {
> >                 status = nfserrno(err);
> >                 goto out_status;
> >         }
> >         if (stat->size != ncf->ncf_cb_fsize)
> > 
> > Then there's no longer a need for a struct kstat at all. The client is
> > holding a delegation, so I would expect the file size to be stable.
> > 
> 
> I hate relying on the size for this, since it's not a reliable
> indicator of change, especially on something that has a fixed size (VM
> image, for instance).

You're right, it's not.  I dislike how the pseudocode in 10.4.3 is only
checking the change attribute to determine if the file has been modified
and then 2 paragraphs down it talks about checking the file size.

At any rate, the main point of this fix is to make sure we're comparing
the size from the cb_getattr reply to the current size, not a cached
value.  For example:

client 1 does open(O_RDWR|O_CREATE_O_TRUNC)
client 1 writes some data and flushes it to the server
client 2 does some operation that triggers a CB_GETATTR

At this point client 1 doesn't have any modified data, so it sends the
original change attribute and its current file size (which should match
the current file size on the server).  With the current code, the server
compares the size client 1 sent with the value it cached on the
delegation (in this case 0).  The server treats it as if the client is
has modified data, which it does not.  That's all this patch is aiming
to fix.

> 
> In fact, I wonder if this ought to be looking at the mtime too. If the
> mtime that the client reported is later than the mtime the server has,
> then we can probably infer that there are buffered writes out there.

But the spec doesn't say anything about comparing the mtime.  Plus, if
the client is sending the original change attribute but a different
mtime than what the server has, wouldn't that be a client bug?

Plus, wouldn't it sort of be crossing into delegated timestamps
territory if we started checking the mtime?

> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Jeff Layton]

On Mon, 2026-04-06 at 13:47 -0400, Scott Mayhew wrote:
> On Mon, 06 Apr 2026, Jeff Layton wrote:
> 
> > On Mon, 2026-04-06 at 10:12 -0400, Chuck Lever wrote:
> > > 
> > > On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > > > On Sat, 04 Apr 2026, Chuck Lever wrote:
> > > > > On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> > > 
> > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > index fa657badf5f8..53d8e7e7d60b 100644
> > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > 
> > > > > > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > > > > > dentry *dentry, struct nfs4_delegation
> > > > > >   * caller must put the reference.
> > > > > >   */
> > > > > >  __be32
> > > > > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > > > > > *dentry,
> > > > > > -			     struct nfs4_delegation **pdp)
> > > > > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > > > > > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> > > > > 
> > > > > Passing the kstat struct in saves some stack just as I suggested,
> > > > > but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> > > > > though -- did you happen to measure how deep it gets after this patch
> > > > > is applied?
> > > > > 
> > > > 
> > > > I tried using the stack tracer:
> > > > 
> > > > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > > > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > >         Depth    Size   Location    (18 entries)
> > > >         -----    ----   --------
> > > >   0)     1936      48   vfs_getattr+0x9/0x50
> > > >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> > > >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> > > >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> > > >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> > > >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> > > >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> > > >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > >  15)      288      56   kthread+0xe3/0x120
> > > >  16)      232      40   ret_from_fork+0x1a1/0x270
> > > >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > 
> > > > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > > > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> > > > 
> > > > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> > > > 
> > > > # echo nfsd4_deleg_getattr_conflict 
> > > > > /sys/kernel/debug/tracing/stack_trace_filter
> > > > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > >         Depth    Size   Location    (14 entries)
> > > >         -----    ----   --------
> > > >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> > > >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> > > >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> > > >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > >  11)      288      56   kthread+0xe3/0x120
> > > >  12)      232      40   ret_from_fork+0x1a1/0x270
> > > >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > 
> > > > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > > > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > > > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > > > (I mostly punted to Gemini for this):
> > > > 
> > > > # cat peak-usage.bt 
> > > > kprobe:nfsd4_deleg_getattr_conflict {
> > > >     @in_deleg_conflict[tid] = 1;
> > > > }
> > > > 
> > > > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> > > >     $stack_size = (uint64)16384; 
> > > >     $sp = reg("sp");
> > > >     $stack_base = $sp & ~($stack_size - 1);
> > > >     $total_used = $stack_base + $stack_size - $sp;
> > > > 
> > > >     if ($total_used > @max_depth_bytes) {
> > > >         @max_depth_bytes = $total_used;
> > > >         @deepest_stack = kstack;
> > > >     }
> > > > }
> > > > 
> > > > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> > > > 
> > > > And finally the result:
> > > > 
> > > > # bpftrace peak-usage.bt 
> > > > Attached 3 probes
> > > > ^C
> > > > 
> > > > @deepest_stack: 
> > > >         avc_lookup+1
> > > >         avc_has_perm_noaudit+60
> > > >         avc_has_perm+89
> > > >         selinux_inode_getattr+203
> > > >         security_inode_getattr+70
> > > >         vfs_getattr+35
> > > >         nfsd4_deleg_getattr_conflict+958
> > > >         nfsd4_encode_fattr4+401
> > > >         nfsd4_encode_getattr+44
> > > >         nfsd4_encode_operation+207
> > > >         nfsd4_proc_compound+470
> > > >         nfsd_dispatch+217
> > > >         svc_process_common+1227
> > > >         svc_process+336
> > > >         svc_handle_xprt+1200
> > > >         svc_recv+434
> > > >         nfsd+284
> > > >         kthread+227
> > > >         ret_from_fork+417
> > > >         ret_from_fork_asm+26
> > > > 
> > > > @max_depth_bytes: 1792
> > > 
> > > Since the new code only needs the file's size, perhaps you can get
> > > away with
> > > 
> > >         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> > > 
> > > rather than
> > > 
> > >         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
> > >         if (err) {
> > >                 status = nfserrno(err);
> > >                 goto out_status;
> > >         }
> > >         if (stat->size != ncf->ncf_cb_fsize)
> > > 
> > > Then there's no longer a need for a struct kstat at all. The client is
> > > holding a delegation, so I would expect the file size to be stable.
> > > 
> > 
> > I hate relying on the size for this, since it's not a reliable
> > indicator of change, especially on something that has a fixed size (VM
> > image, for instance).
> 
> You're right, it's not.  I dislike how the pseudocode in 10.4.3 is only
> checking the change attribute to determine if the file has been modified
> and then 2 paragraphs down it talks about checking the file size.
> 
> At any rate, the main point of this fix is to make sure we're comparing
> the size from the cb_getattr reply to the current size, not a cached
> value.  For example:
> 
> client 1 does open(O_RDWR|O_CREATE_O_TRUNC)
> client 1 writes some data and flushes it to the server
> client 2 does some operation that triggers a CB_GETATTR
> 
> At this point client 1 doesn't have any modified data, so it sends the
> original change attribute and its current file size (which should match
> the current file size on the server).  With the current code, the server
> compares the size client 1 sent with the value it cached on the
> delegation (in this case 0).  The server treats it as if the client is
> has modified data, which it does not.  That's all this patch is aiming
> to fix.
> 
> > 
> > In fact, I wonder if this ought to be looking at the mtime too. If the
> > mtime that the client reported is later than the mtime the server has,
> > then we can probably infer that there are buffered writes out there.
> 
> But the spec doesn't say anything about comparing the mtime.  Plus, if
> the client is sending the original change attribute but a different
> mtime than what the server has, wouldn't that be a client bug?
> 

Not really. The change attr sent by the client is "special". It's
usually just original change attr at time of delegation +1 iff
something has been modified. I don't see that as a reliable indicator
that there is still buffered write data on the client.

> Plus, wouldn't it sort of be crossing into delegated timestamps
> territory if we started checking the mtime?
> 

Good point.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Jeff Layton]

On Mon, 2026-04-06 at 13:47 -0400, Scott Mayhew wrote:
> On Mon, 06 Apr 2026, Jeff Layton wrote:
> 
> > On Mon, 2026-04-06 at 10:12 -0400, Chuck Lever wrote:
> > > 
> > > On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > > > On Sat, 04 Apr 2026, Chuck Lever wrote:
> > > > > On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> > > 
> > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > index fa657badf5f8..53d8e7e7d60b 100644
> > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > 
> > > > > > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > > > > > dentry *dentry, struct nfs4_delegation
> > > > > >   * caller must put the reference.
> > > > > >   */
> > > > > >  __be32
> > > > > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > > > > > *dentry,
> > > > > > -			     struct nfs4_delegation **pdp)
> > > > > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > > > > > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> > > > > 
> > > > > Passing the kstat struct in saves some stack just as I suggested,
> > > > > but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> > > > > though -- did you happen to measure how deep it gets after this patch
> > > > > is applied?
> > > > > 
> > > > 
> > > > I tried using the stack tracer:
> > > > 
> > > > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > > > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > >         Depth    Size   Location    (18 entries)
> > > >         -----    ----   --------
> > > >   0)     1936      48   vfs_getattr+0x9/0x50
> > > >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> > > >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> > > >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> > > >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> > > >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> > > >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> > > >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > >  15)      288      56   kthread+0xe3/0x120
> > > >  16)      232      40   ret_from_fork+0x1a1/0x270
> > > >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > 
> > > > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > > > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> > > > 
> > > > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> > > > 
> > > > # echo nfsd4_deleg_getattr_conflict 
> > > > > /sys/kernel/debug/tracing/stack_trace_filter
> > > > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > >         Depth    Size   Location    (14 entries)
> > > >         -----    ----   --------
> > > >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> > > >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> > > >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> > > >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > >  11)      288      56   kthread+0xe3/0x120
> > > >  12)      232      40   ret_from_fork+0x1a1/0x270
> > > >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > 
> > > > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > > > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > > > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > > > (I mostly punted to Gemini for this):
> > > > 
> > > > # cat peak-usage.bt 
> > > > kprobe:nfsd4_deleg_getattr_conflict {
> > > >     @in_deleg_conflict[tid] = 1;
> > > > }
> > > > 
> > > > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> > > >     $stack_size = (uint64)16384; 
> > > >     $sp = reg("sp");
> > > >     $stack_base = $sp & ~($stack_size - 1);
> > > >     $total_used = $stack_base + $stack_size - $sp;
> > > > 
> > > >     if ($total_used > @max_depth_bytes) {
> > > >         @max_depth_bytes = $total_used;
> > > >         @deepest_stack = kstack;
> > > >     }
> > > > }
> > > > 
> > > > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> > > > 
> > > > And finally the result:
> > > > 
> > > > # bpftrace peak-usage.bt 
> > > > Attached 3 probes
> > > > ^C
> > > > 
> > > > @deepest_stack: 
> > > >         avc_lookup+1
> > > >         avc_has_perm_noaudit+60
> > > >         avc_has_perm+89
> > > >         selinux_inode_getattr+203
> > > >         security_inode_getattr+70
> > > >         vfs_getattr+35
> > > >         nfsd4_deleg_getattr_conflict+958
> > > >         nfsd4_encode_fattr4+401
> > > >         nfsd4_encode_getattr+44
> > > >         nfsd4_encode_operation+207
> > > >         nfsd4_proc_compound+470
> > > >         nfsd_dispatch+217
> > > >         svc_process_common+1227
> > > >         svc_process+336
> > > >         svc_handle_xprt+1200
> > > >         svc_recv+434
> > > >         nfsd+284
> > > >         kthread+227
> > > >         ret_from_fork+417
> > > >         ret_from_fork_asm+26
> > > > 
> > > > @max_depth_bytes: 1792
> > > 
> > > Since the new code only needs the file's size, perhaps you can get
> > > away with
> > > 
> > >         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> > > 
> > > rather than
> > > 
> > >         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
> > >         if (err) {
> > >                 status = nfserrno(err);
> > >                 goto out_status;
> > >         }
> > >         if (stat->size != ncf->ncf_cb_fsize)
> > > 
> > > Then there's no longer a need for a struct kstat at all. The client is
> > > holding a delegation, so I would expect the file size to be stable.
> > > 
> > 
> > I hate relying on the size for this, since it's not a reliable
> > indicator of change, especially on something that has a fixed size (VM
> > image, for instance).
> 
> You're right, it's not.  I dislike how the pseudocode in 10.4.3 is only
> checking the change attribute to determine if the file has been modified
> and then 2 paragraphs down it talks about checking the file size.
> 
> At any rate, the main point of this fix is to make sure we're comparing
> the size from the cb_getattr reply to the current size, not a cached
> value.  For example:
> 
> client 1 does open(O_RDWR|O_CREATE_O_TRUNC)
> client 1 writes some data and flushes it to the server
> client 2 does some operation that triggers a CB_GETATTR
> 
> At this point client 1 doesn't have any modified data, so it sends the
> original change attribute and its current file size (which should match
> the current file size on the server).  With the current code, the server
> compares the size client 1 sent with the value it cached on the
> delegation (in this case 0).  The server treats it as if the client is
> has modified data, which it does not.  That's all this patch is aiming
> to fix.
> 
> > 
> > In fact, I wonder if this ought to be looking at the mtime too. If the
> > mtime that the client reported is later than the mtime the server has,
> > then we can probably infer that there are buffered writes out there.
> 
> But the spec doesn't say anything about comparing the mtime.  Plus, if
> the client is sending the original change attribute but a different
> mtime than what the server has, wouldn't that be a client bug?
> 
> Plus, wouldn't it sort of be crossing into delegated timestamps
> territory if we started checking the mtime?
> 
> 

Should we be looking at extending the spec? Maybe we should add a "I
have outstanding buffered data" attribute that the client could send to
the server in a CB_GETATTR?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] nfsd: fix file change detection in CB_GETATTR

[Scott Mayhew]

On Mon, 06 Apr 2026, Jeff Layton wrote:

> On Mon, 2026-04-06 at 13:47 -0400, Scott Mayhew wrote:
> > On Mon, 06 Apr 2026, Jeff Layton wrote:
> > 
> > > On Mon, 2026-04-06 at 10:12 -0400, Chuck Lever wrote:
> > > > 
> > > > On Sun, Apr 5, 2026, at 6:13 PM, Scott Mayhew wrote:
> > > > > On Sat, 04 Apr 2026, Chuck Lever wrote:
> > > > > > On Fri, Apr 3, 2026, at 8:54 PM, Scott Mayhew wrote:
> > > > 
> > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > > index fa657badf5f8..53d8e7e7d60b 100644
> > > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > > 
> > > > > > > @@ -9459,17 +9461,18 @@ static int cb_getattr_update_times(struct 
> > > > > > > dentry *dentry, struct nfs4_delegation
> > > > > > >   * caller must put the reference.
> > > > > > >   */
> > > > > > >  __be32
> > > > > > > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry 
> > > > > > > *dentry,
> > > > > > > -			     struct nfs4_delegation **pdp)
> > > > > > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct path *path,
> > > > > > > +			     struct kstat *stat, struct nfs4_delegation **pdp)
> > > > > > 
> > > > > > Passing the kstat struct in saves some stack just as I suggested,
> > > > > > but it is an ugly API. The nfsd4_encode_fattr4() call stack is tall,
> > > > > > though -- did you happen to measure how deep it gets after this patch
> > > > > > is applied?
> > > > > > 
> > > > > 
> > > > > I tried using the stack tracer:
> > > > > 
> > > > > # echo 1 >/proc/sys/kernel/stack_tracer_enabled
> > > > > # echo vfs_getattr >/sys/kernel/debug/tracing/stack_trace_filter
> > > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > > >         Depth    Size   Location    (18 entries)
> > > > >         -----    ----   --------
> > > > >   0)     1936      48   vfs_getattr+0x9/0x50
> > > > >   1)     1888     552   nfsd4_encode_fattr4+0x1b2/0x7a0 [nfsd]
> > > > >   2)     1336      80   nfsd4_encode_entry4_fattr+0xf8/0x210 [nfsd]
> > > > >   3)     1256      96   nfsd4_encode_entry4+0x10b/0x2a0 [nfsd]
> > > > >   4)     1160     144   nfsd_buffered_readdir+0x139/0x310 [nfsd]
> > > > >   5)     1016      80   nfsd_readdir+0x9f/0x180 [nfsd]
> > > > >   6)      936      80   nfsd4_encode_readdir+0xdf/0x1e0 [nfsd]
> > > > >   7)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > > >   8)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > > >   9)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > > >  10)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > > >  11)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > > >  12)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > > >  13)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > > >  14)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > > >  15)      288      56   kthread+0xe3/0x120
> > > > >  16)      232      40   ret_from_fork+0x1a1/0x270
> > > > >  17)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > > 
> > > > > But that's capturing a vfs_getattr() call from encoding a readdir reply,
> > > > > rather than the vfs_getattr() call I added to nfsd4_deleg_getattr_conflict().
> > > > > 
> > > > > Here's the stack depth for nfsd4_deleg_getattr_conflict():
> > > > > 
> > > > > # echo nfsd4_deleg_getattr_conflict 
> > > > > > /sys/kernel/debug/tracing/stack_trace_filter
> > > > > # echo 0 >/sys/kernel/debug/tracing/stack_max_size 
> > > > > # cat /sys/kernel/debug/tracing/stack_trace
> > > > >         Depth    Size   Location    (14 entries)
> > > > >         -----    ----   --------
> > > > >   0)     1472      48   nfsd4_deleg_getattr_conflict+0x9/0x410 [nfsd]
> > > > >   1)     1424     552   nfsd4_encode_fattr4+0x191/0x7a0 [nfsd]
> > > > >   2)      872      16   nfsd4_encode_getattr+0x2c/0x40 [nfsd]
> > > > >   3)      856      80   nfsd4_encode_operation+0xcf/0x3d0 [nfsd]
> > > > >   4)      776      80   nfsd4_proc_compound+0x1d6/0x7a0 [nfsd]
> > > > >   5)      696      80   nfsd_dispatch+0xd9/0x240 [nfsd]
> > > > >   6)      616      80   svc_process_common+0x4cb/0x6b0 [sunrpc]
> > > > >   7)      536      40   svc_process+0x150/0x240 [sunrpc]
> > > > >   8)      496      72   svc_handle_xprt+0x4b0/0x5f0 [sunrpc]
> > > > >   9)      424      56   svc_recv+0x1b2/0x3a0 [sunrpc]
> > > > >  10)      368      80   nfsd+0x11c/0x3d0 [nfsd]
> > > > >  11)      288      56   kthread+0xe3/0x120
> > > > >  12)      232      40   ret_from_fork+0x1a1/0x270
> > > > >  13)      192     192   ret_from_fork_asm+0x1a/0x30
> > > > > 
> > > > > Manually inspecting function graphs of vfs_getattr(), it looks like the deepest
> > > > > function (that we can trace) is avc_lookup(), so here's a bpftrace script that
> > > > > prints the stack depth from avc_lookup() via nfsd4_deleg_getattr_conflict()
> > > > > (I mostly punted to Gemini for this):
> > > > > 
> > > > > # cat peak-usage.bt 
> > > > > kprobe:nfsd4_deleg_getattr_conflict {
> > > > >     @in_deleg_conflict[tid] = 1;
> > > > > }
> > > > > 
> > > > > kprobe:avc_lookup /@in_deleg_conflict[tid]/ {
> > > > >     $stack_size = (uint64)16384; 
> > > > >     $sp = reg("sp");
> > > > >     $stack_base = $sp & ~($stack_size - 1);
> > > > >     $total_used = $stack_base + $stack_size - $sp;
> > > > > 
> > > > >     if ($total_used > @max_depth_bytes) {
> > > > >         @max_depth_bytes = $total_used;
> > > > >         @deepest_stack = kstack;
> > > > >     }
> > > > > }
> > > > > 
> > > > > kretprobe:nfsd4_deleg_getattr_conflict { delete(@in_deleg_conflict[tid]); }
> > > > > 
> > > > > And finally the result:
> > > > > 
> > > > > # bpftrace peak-usage.bt 
> > > > > Attached 3 probes
> > > > > ^C
> > > > > 
> > > > > @deepest_stack: 
> > > > >         avc_lookup+1
> > > > >         avc_has_perm_noaudit+60
> > > > >         avc_has_perm+89
> > > > >         selinux_inode_getattr+203
> > > > >         security_inode_getattr+70
> > > > >         vfs_getattr+35
> > > > >         nfsd4_deleg_getattr_conflict+958
> > > > >         nfsd4_encode_fattr4+401
> > > > >         nfsd4_encode_getattr+44
> > > > >         nfsd4_encode_operation+207
> > > > >         nfsd4_proc_compound+470
> > > > >         nfsd_dispatch+217
> > > > >         svc_process_common+1227
> > > > >         svc_process+336
> > > > >         svc_handle_xprt+1200
> > > > >         svc_recv+434
> > > > >         nfsd+284
> > > > >         kthread+227
> > > > >         ret_from_fork+417
> > > > >         ret_from_fork_asm+26
> > > > > 
> > > > > @max_depth_bytes: 1792
> > > > 
> > > > Since the new code only needs the file's size, perhaps you can get
> > > > away with
> > > > 
> > > >         if (i_size_read(inode) != ncf->ncf_cb_fsize)
> > > > 
> > > > rather than
> > > > 
> > > >         err = vfs_getattr(path, stat, STATX_SIZE, AT_STATX_SYNC_AS_STAT);
> > > >         if (err) {
> > > >                 status = nfserrno(err);
> > > >                 goto out_status;
> > > >         }
> > > >         if (stat->size != ncf->ncf_cb_fsize)
> > > > 
> > > > Then there's no longer a need for a struct kstat at all. The client is
> > > > holding a delegation, so I would expect the file size to be stable.
> > > > 
> > > 
> > > I hate relying on the size for this, since it's not a reliable
> > > indicator of change, especially on something that has a fixed size (VM
> > > image, for instance).
> > 
> > You're right, it's not.  I dislike how the pseudocode in 10.4.3 is only
> > checking the change attribute to determine if the file has been modified
> > and then 2 paragraphs down it talks about checking the file size.
> > 
> > At any rate, the main point of this fix is to make sure we're comparing
> > the size from the cb_getattr reply to the current size, not a cached
> > value.  For example:
> > 
> > client 1 does open(O_RDWR|O_CREATE_O_TRUNC)
> > client 1 writes some data and flushes it to the server
> > client 2 does some operation that triggers a CB_GETATTR
> > 
> > At this point client 1 doesn't have any modified data, so it sends the
> > original change attribute and its current file size (which should match
> > the current file size on the server).  With the current code, the server
> > compares the size client 1 sent with the value it cached on the
> > delegation (in this case 0).  The server treats it as if the client is
> > has modified data, which it does not.  That's all this patch is aiming
> > to fix.
> > 
> > > 
> > > In fact, I wonder if this ought to be looking at the mtime too. If the
> > > mtime that the client reported is later than the mtime the server has,
> > > then we can probably infer that there are buffered writes out there.
> > 
> > But the spec doesn't say anything about comparing the mtime.  Plus, if
> > the client is sending the original change attribute but a different
> > mtime than what the server has, wouldn't that be a client bug?
> > 
> > Plus, wouldn't it sort of be crossing into delegated timestamps
> > territory if we started checking the mtime?
> > 
> > 
> 
> Should we be looking at extending the spec? Maybe we should add a "I
> have outstanding buffered data" attribute that the client could send to
> the server in a CB_GETATTR?

I guess I don't understand why the client's usage of the change
attribute doesn't already cover that.

If an application calls write() but not fsync(), close(), etc, and the
background flushers haven't kicked in yet, then the dirty pages/folios
will be tracked on the nfs_inode and the client will send the elevated
change attribute in the CB_GETATTR reply.

What am I missing?

> -- 
> Jeff Layton <jlayton@kernel.org>
>