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

[Scott Mayhew <smayhew@redhat.com>]

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
>