| Message ID | 20220227231227.9038-4-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Readdir improvements | expand |
On 24 Feb 2022, at 21:09, Trond Myklebust wrote: > On Thu, 2022-02-24 at 09:14 -0500, Benjamin Coddington wrote: >> There's a path through nfs4_lookup_revalidate that will now only produce >> this exit tracepoint. Does it need the _enter tracepoint added? > > You're thinking about the nfs_lookup_revalidate_delegated() path? The > _enter() tracepoint doesn't provide any useful information that isn't > already provided by the _exit(), AFAICS. No, the path through nfs4_do_lookup_revalidate(), reval_dentry: jump. But I agree there's not much value in the _enter() tracepoint. Maybe we can remove it, and _exit more like _done. I am thinking about hearing back from folks about mis-matched _enter() and _exit() results, but also realize this is nit-picking. Ben
> On Mar 9, 2022, at 8:42 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > On 24 Feb 2022, at 21:09, Trond Myklebust wrote: >> On Thu, 2022-02-24 at 09:14 -0500, Benjamin Coddington wrote: >>> There's a path through nfs4_lookup_revalidate that will now only produce >>> this exit tracepoint. Does it need the _enter tracepoint added? >> >> You're thinking about the nfs_lookup_revalidate_delegated() path? The >> _enter() tracepoint doesn't provide any useful information that isn't >> already provided by the _exit(), AFAICS. > > No, the path through nfs4_do_lookup_revalidate(), reval_dentry: jump. But I > agree there's not much value in the _enter() tracepoint. Maybe we can > remove it, and _exit more like _done. > > I am thinking about hearing back from folks about mis-matched _enter() and > _exit() results, but also realize this is nit-picking. I think the _enter / _exit trace points simply replaced dprintk call sites which did much the same reporting. Maybe we should consider replacing some of these because we can rely on function call tracing instead. But generally we like to see trace points that report exceptional events rather than "I made it to this point". The latter category of trace points are interesting while code is under development but often loses its value once the code is in the field. -- Chuck Lever
On 9 Mar 2022, at 10:28, Chuck Lever III wrote: >> On Mar 9, 2022, at 8:42 AM, Benjamin Coddington <bcodding@redhat.com> >> wrote: >> >> On 24 Feb 2022, at 21:09, Trond Myklebust wrote: >>> On Thu, 2022-02-24 at 09:14 -0500, Benjamin Coddington wrote: >>>> There's a path through nfs4_lookup_revalidate that will now only >>>> produce >>>> this exit tracepoint. Does it need the _enter tracepoint added? >>> >>> You're thinking about the nfs_lookup_revalidate_delegated() path? >>> The >>> _enter() tracepoint doesn't provide any useful information that >>> isn't >>> already provided by the _exit(), AFAICS. >> >> No, the path through nfs4_do_lookup_revalidate(), reval_dentry: jump. >> But I >> agree there's not much value in the _enter() tracepoint. Maybe we >> can >> remove it, and _exit more like _done. >> >> I am thinking about hearing back from folks about mis-matched >> _enter() and >> _exit() results, but also realize this is nit-picking. > > I think the _enter / _exit trace points simply replaced > dprintk call sites which did much the same reporting. > Maybe we should consider replacing some of these because > we can rely on function call tracing instead. > > But generally we like to see trace points that report > exceptional events rather than "I made it to this point". > The latter category of trace points are interesting > while code is under development but often loses its > value once the code is in the field. Instead of "hearing back from folks" I should have said "I worry our QE team is going to discover and possibly report as a bug". :P Thanks for filling in Chuck! Ben
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index ebddc736eac2..1aa55cac9d9a 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1474,9 +1474,7 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, { switch (error) { case 1: - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is valid\n", - __func__, dentry); - return 1; + break; case 0: /* * We can't d_drop the root of a disconnected tree: @@ -1485,13 +1483,10 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry, * inodes on unmount and further oopses. */ if (inode && IS_ROOT(dentry)) - return 1; - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is invalid\n", - __func__, dentry); - return 0; + error = 1; + break; } - dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) lookup returned error %d\n", - __func__, dentry, error); + trace_nfs_lookup_revalidate_exit(dir, dentry, 0, error); return error; } @@ -1623,9 +1618,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, goto out_bad; trace_nfs_lookup_revalidate_enter(dir, dentry, flags); - error = nfs_lookup_revalidate_dentry(dir, dentry, inode); - trace_nfs_lookup_revalidate_exit(dir, dentry, flags, error); - return error; + return nfs_lookup_revalidate_dentry(dir, dentry, inode); out_valid: return nfs_lookup_revalidate_done(dir, dentry, inode, 1); out_bad: