2021-03-24 20:00:40

by Trond Myklebust

[permalink] [raw]
Subject: [PATCH] NFS: fix nfs_fetch_iversion()

From: Trond Myklebust <[email protected]>

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 <[email protected]>
---
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


2021-03-25 03:26:29

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] NFS: fix nfs_fetch_iversion()

On Wed, Mar 24, 2021 at 03:53:53PM -0400, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>

2021-03-25 03:41:33

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] NFS: fix nfs_fetch_iversion()

On Wed, 2021-03-24 at 17:23 -0400, J. Bruce Fields wrote:
> On Wed, Mar 24, 2021 at 03:53:53PM -0400, [email protected] wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >  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
[email protected]