[PATCH] NFS: fix nfs_fetch_iversion()

[PATCH] NFS: fix nfs_fetch_iversion()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

The change attribute is always set by all NFS client versions so get rid
of the open-coded version.

Fixes: 3cc55f4434b4 ("nfs: use change attribute for NFS re-exports")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/export.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/export.c b/fs/nfs/export.c
index f2b34cfe286c..b347e3ce0cc8 100644
--- a/fs/nfs/export.c
+++ b/fs/nfs/export.c
@@ -171,17 +171,10 @@ static u64 nfs_fetch_iversion(struct inode *inode)
 {
 	struct nfs_server *server = NFS_SERVER(inode);
 
-	/* Is this the right call?: */
-	nfs_revalidate_inode(server, inode);
-	/*
-	 * Also, note we're ignoring any returned error.  That seems to be
-	 * the practice for cache consistency information elsewhere in
-	 * the server, but I'm not sure why.
-	 */
-	if (server->nfs_client->rpc_ops->version >= 4)
-		return inode_peek_iversion_raw(inode);
-	else
-		return time_to_chattr(&inode->i_ctime);
+	if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
+						   NFS_INO_REVAL_PAGECACHE))
+		__nfs_revalidate_inode(server, inode);
+	return inode_peek_iversion_raw(inode);
 }
 
 const struct export_operations nfs_export_ops = {
-- 
2.30.2

Re: [PATCH] NFS: fix nfs_fetch_iversion()

[J. Bruce Fields]

On Wed, Mar 24, 2021 at 03:53:53PM -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> The change attribute is always set by all NFS client versions so get rid
> of the open-coded version.

Thanks!

I'm unclear whether there's a user-visible bug here or whether it's
mainly cleanup?

--b.

> Fixes: 3cc55f4434b4 ("nfs: use change attribute for NFS re-exports")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/export.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> index f2b34cfe286c..b347e3ce0cc8 100644
> --- a/fs/nfs/export.c
> +++ b/fs/nfs/export.c
> @@ -171,17 +171,10 @@ static u64 nfs_fetch_iversion(struct inode *inode)
>  {
>  	struct nfs_server *server = NFS_SERVER(inode);
>  
> -	/* Is this the right call?: */
> -	nfs_revalidate_inode(server, inode);
> -	/*
> -	 * Also, note we're ignoring any returned error.  That seems to be
> -	 * the practice for cache consistency information elsewhere in
> -	 * the server, but I'm not sure why.
> -	 */
> -	if (server->nfs_client->rpc_ops->version >= 4)
> -		return inode_peek_iversion_raw(inode);
> -	else
> -		return time_to_chattr(&inode->i_ctime);
> +	if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
> +						   NFS_INO_REVAL_PAGECACHE))
> +		__nfs_revalidate_inode(server, inode);
> +	return inode_peek_iversion_raw(inode);
>  }
>  
>  const struct export_operations nfs_export_ops = {
> -- 
> 2.30.2
> 

Re: [PATCH] NFS: fix nfs_fetch_iversion()

[Trond Myklebust]

On Wed, 2021-03-24 at 17:23 -0400, J. Bruce Fields wrote:
> On Wed, Mar 24, 2021 at 03:53:53PM -0400, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > The change attribute is always set by all NFS client versions so
> > get rid
> > of the open-coded version.
> 
> Thanks!
> 
> I'm unclear whether there's a user-visible bug here or whether it's
> mainly cleanup?
> 

It is mainly about ensuring that revalidation is done correctly. It is
a performance issue, but is also about ensuring that we all feed into
the same validity checking code.

> --b.
> 
> > Fixes: 3cc55f4434b4 ("nfs: use change attribute for NFS re-
> > exports")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/export.c | 15 ++++-----------
> >  1 file changed, 4 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> > index f2b34cfe286c..b347e3ce0cc8 100644
> > --- a/fs/nfs/export.c
> > +++ b/fs/nfs/export.c
> > @@ -171,17 +171,10 @@ static u64 nfs_fetch_iversion(struct inode
> > *inode)
> >  {
> >         struct nfs_server *server = NFS_SERVER(inode);
> >  
> > -       /* Is this the right call?: */
> > -       nfs_revalidate_inode(server, inode);
> > -       /*
> > -        * Also, note we're ignoring any returned error.  That
> > seems to be
> > -        * the practice for cache consistency information elsewhere
> > in
> > -        * the server, but I'm not sure why.
> > -        */
> > -       if (server->nfs_client->rpc_ops->version >= 4)
> > -               return inode_peek_iversion_raw(inode);
> > -       else
> > -               return time_to_chattr(&inode->i_ctime);
> > +       if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
> > +                                                 
> > NFS_INO_REVAL_PAGECACHE))
> > +               __nfs_revalidate_inode(server, inode);
> > +       return inode_peek_iversion_raw(inode);
> >  }
> >  
> >  const struct export_operations nfs_export_ops = {
> > -- 
> > 2.30.2
> > 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com