From: Trond Myklebust <[email protected]>
Valid return values for decode_dirent() callback functions are:
0: Success
-EBADCOOKIE: End of directory
-EAGAIN: End of xdr_stream
All errors need to map into one of those three values.
Fixes: 573c4e1ef53a ("NFS: Simplify ->decode_dirent() calling sequence")
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs2xdr.c | 2 +-
fs/nfs/nfs3xdr.c | 21 ++++++---------------
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
index 7fba7711e6b3..3d5ba43f44bb 100644
--- a/fs/nfs/nfs2xdr.c
+++ b/fs/nfs/nfs2xdr.c
@@ -949,7 +949,7 @@ int nfs2_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
error = decode_filename_inline(xdr, &entry->name, &entry->len);
if (unlikely(error))
- return error;
+ return -EAGAIN;
/*
* The type (size and byte order) of nfscookie isn't defined in
diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
index 54a1d21cbcc6..7ab60ad98776 100644
--- a/fs/nfs/nfs3xdr.c
+++ b/fs/nfs/nfs3xdr.c
@@ -1967,7 +1967,6 @@ int nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
bool plus)
{
struct user_namespace *userns = rpc_userns(entry->server->client);
- struct nfs_entry old = *entry;
__be32 *p;
int error;
u64 new_cookie;
@@ -1987,15 +1986,15 @@ int nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
error = decode_fileid3(xdr, &entry->ino);
if (unlikely(error))
- return error;
+ return -EAGAIN;
error = decode_inline_filename3(xdr, &entry->name, &entry->len);
if (unlikely(error))
- return error;
+ return -EAGAIN;
error = decode_cookie3(xdr, &new_cookie);
if (unlikely(error))
- return error;
+ return -EAGAIN;
entry->d_type = DT_UNKNOWN;
@@ -2003,7 +2002,7 @@ int nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
entry->fattr->valid = 0;
error = decode_post_op_attr(xdr, entry->fattr, userns);
if (unlikely(error))
- return error;
+ return -EAGAIN;
if (entry->fattr->valid & NFS_ATTR_FATTR_V3)
entry->d_type = nfs_umode_to_dtype(entry->fattr->mode);
@@ -2018,11 +2017,8 @@ int nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
return -EAGAIN;
if (*p != xdr_zero) {
error = decode_nfs_fh3(xdr, entry->fh);
- if (unlikely(error)) {
- if (error == -E2BIG)
- goto out_truncated;
- return error;
- }
+ if (unlikely(error))
+ return -EAGAIN;
} else
zero_nfs_fh3(entry->fh);
}
@@ -2031,11 +2027,6 @@ int nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
entry->cookie = new_cookie;
return 0;
-
-out_truncated:
- dprintk("NFS: directory entry contains invalid file handle\n");
- *entry = old;
- return -EAGAIN;
}
/*
--
2.35.1
From: Trond Myklebust <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
include/linux/nfs_fs.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 72a732a5103c..6e10725887d1 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -363,7 +363,7 @@ static inline void nfs_mark_for_revalidate(struct inode *inode)
spin_unlock(&inode->i_lock);
}
-static inline int nfs_server_capable(struct inode *inode, int cap)
+static inline int nfs_server_capable(const struct inode *inode, int cap)
{
return NFS_SERVER(inode)->caps & cap;
}
@@ -587,12 +587,11 @@ extern struct nfs_commit_data *nfs_commitdata_alloc(bool never_fail);
extern void nfs_commit_free(struct nfs_commit_data *data);
bool nfs_commit_end(struct nfs_mds_commit_info *cinfo);
-static inline int
-nfs_have_writebacks(struct inode *inode)
+static inline bool nfs_have_writebacks(const struct inode *inode)
{
if (S_ISREG(inode->i_mode))
return atomic_long_read(&NFS_I(inode)->nrequests) != 0;
- return 0;
+ return false;
}
/*
--
2.35.1
From: Trond Myklebust <[email protected]>
Enable tracing of lookup revalidation failures.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
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:
--
2.35.1
From: Trond Myklebust <[email protected]>
For the purpose of ensuring that opendir() followed by seekdir() work as
correctly as possible, try to initialise the readdir verifier in
nfs_opendir().
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 1aa55cac9d9a..1dfbd05081ad 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -89,6 +89,7 @@ static struct nfs_open_dir_context *alloc_nfs_open_dir_context(struct inode *dir
NFS_INO_REVAL_FORCED);
list_add(&ctx->list, &nfsi->open_files);
clear_bit(NFS_INO_FORCE_READDIR, &nfsi->flags);
+ memcpy(ctx->verf, nfsi->cookieverf, sizeof(ctx->verf));
spin_unlock(&dir->i_lock);
return ctx;
}
--
2.35.1
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 <[email protected]> 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 <[email protected]>
>> 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