diff mbox series

[v6,4/9] nfs: report the inode version in getattr if requested

Message ID 20220930111840.10695-5-jlayton@kernel.org (mailing list archive)
State Superseded
Headers show
Series vfs/nfsd: clean up handling of i_version counter | expand

Commit Message

Jeff Layton Sept. 30, 2022, 11:18 a.m. UTC 
Allow NFS to report the i_version in getattr requests. Since the cost to
fetch it is relatively cheap, do it unconditionally and just set the
flag if it looks like it's valid. Also, conditionally enable the
MONOTONIC flag when the server reports its change attr type as such.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/inode.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

NeilBrown Oct. 3, 2022, 11:29 p.m. UTC | #1 
On Fri, 30 Sep 2022, Jeff Layton wrote:
> Allow NFS to report the i_version in getattr requests. Since the cost to
> fetch it is relatively cheap, do it unconditionally and just set the
> flag if it looks like it's valid. Also, conditionally enable the
> MONOTONIC flag when the server reports its change attr type as such.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfs/inode.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index bea7c005119c..5cb7017e5089 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -830,6 +830,8 @@ static u32 nfs_get_valid_attrmask(struct inode *inode)
>  		reply_mask |= STATX_UID | STATX_GID;
>  	if (!(cache_validity & NFS_INO_INVALID_BLOCKS))
>  		reply_mask |= STATX_BLOCKS;
> +	if (!(cache_validity & NFS_INO_INVALID_CHANGE))
> +		reply_mask |= STATX_VERSION;
>  	return reply_mask;
>  }
>  
> @@ -848,7 +850,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
>  
>  	request_mask &= STATX_TYPE | STATX_MODE | STATX_NLINK | STATX_UID |
>  			STATX_GID | STATX_ATIME | STATX_MTIME | STATX_CTIME |
> -			STATX_INO | STATX_SIZE | STATX_BLOCKS;
> +			STATX_INO | STATX_SIZE | STATX_BLOCKS | STATX_VERSION;
>  
>  	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
>  		if (readdirplus_enabled)
> @@ -877,7 +879,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
>  	/* Is the user requesting attributes that might need revalidation? */
>  	if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
>  					STATX_MTIME|STATX_UID|STATX_GID|
> -					STATX_SIZE|STATX_BLOCKS)))
> +					STATX_SIZE|STATX_BLOCKS|STATX_VERSION)))
>  		goto out_no_revalidate;
>  
>  	/* Check whether the cached attributes are stale */
> @@ -915,6 +917,10 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
>  
>  	generic_fillattr(&init_user_ns, inode, stat);
>  	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> +	stat->version = inode_peek_iversion_raw(inode);

This looks wrong.
1/ it includes the I_VERSION_QUERIED bit, which should be hidden.
2/ it doesn't set that bit.

I understand that the bit was already set when the generic code called
inode_query_iversion(), but it might have changed if we needed to
refresh the attrs.

I'm beginning to think I shouldn't have approved the 3/9 patch.  The
stat->version shouldn't be set in vfs_getattr_nosec() - maybe in
generic_fillattr(), but not a lot of point.

> +	stat->attributes_mask |= STATX_ATTR_VERSION_MONOTONIC;
> +	if (server->change_attr_type != NFS4_CHANGE_TYPE_IS_UNDEFINED)
> +		stat->attributes |= STATX_ATTR_VERSION_MONOTONIC;

So if the server tells us that the change attrs is based on time
metadata, we accept that it will be monotonic (and RFC7862 encourages
this), even though we seem to worry about timestamps going backwards
(which we know that can)...  Interesting.

Thanks,
NeilBrown


>  	if (S_ISDIR(inode->i_mode))
>  		stat->blksize = NFS_SERVER(inode)->dtsize;
>  out:
> -- 
> 2.37.3
> 
>
Jeff Layton Oct. 4, 2022, 9:43 a.m. UTC | #2 
On Tue, 2022-10-04 at 10:29 +1100, NeilBrown wrote:
> On Fri, 30 Sep 2022, Jeff Layton wrote:
> > Allow NFS to report the i_version in getattr requests. Since the cost to
> > fetch it is relatively cheap, do it unconditionally and just set the
> > flag if it looks like it's valid. Also, conditionally enable the
> > MONOTONIC flag when the server reports its change attr type as such.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfs/inode.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index bea7c005119c..5cb7017e5089 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -830,6 +830,8 @@ static u32 nfs_get_valid_attrmask(struct inode *inode)
> >  		reply_mask |= STATX_UID | STATX_GID;
> >  	if (!(cache_validity & NFS_INO_INVALID_BLOCKS))
> >  		reply_mask |= STATX_BLOCKS;
> > +	if (!(cache_validity & NFS_INO_INVALID_CHANGE))
> > +		reply_mask |= STATX_VERSION;
> >  	return reply_mask;
> >  }
> >  
> > @@ -848,7 +850,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >  
> >  	request_mask &= STATX_TYPE | STATX_MODE | STATX_NLINK | STATX_UID |
> >  			STATX_GID | STATX_ATIME | STATX_MTIME | STATX_CTIME |
> > -			STATX_INO | STATX_SIZE | STATX_BLOCKS;
> > +			STATX_INO | STATX_SIZE | STATX_BLOCKS | STATX_VERSION;
> >  
> >  	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
> >  		if (readdirplus_enabled)
> > @@ -877,7 +879,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >  	/* Is the user requesting attributes that might need revalidation? */
> >  	if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> >  					STATX_MTIME|STATX_UID|STATX_GID|
> > -					STATX_SIZE|STATX_BLOCKS)))
> > +					STATX_SIZE|STATX_BLOCKS|STATX_VERSION)))
> >  		goto out_no_revalidate;
> >  
> >  	/* Check whether the cached attributes are stale */
> > @@ -915,6 +917,10 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >  
> >  	generic_fillattr(&init_user_ns, inode, stat);
> >  	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> > +	stat->version = inode_peek_iversion_raw(inode);
> 
> This looks wrong.
> 1/ it includes the I_VERSION_QUERIED bit, which should be hidden.
> 2/ it doesn't set that bit.
> 
> I understand that the bit was already set when the generic code called
> inode_query_iversion(), but it might have changed if we needed to
> refresh the attrs.
> 
> I'm beginning to think I shouldn't have approved the 3/9 patch.  The
> stat->version shouldn't be set in vfs_getattr_nosec() - maybe in
> generic_fillattr(), but not a lot of point.
> 

NFS (and Ceph), do not set the SB_I_VERSION flag and they don't use the
QUERIED bit. These are "server managed" implementations of i_version.
The server is responsible for incrementing the value, and we just store
the result in the i_version field and present it when needed. That's why
the patch for NFS is using the "raw" API.

> > +	stat->attributes_mask |= STATX_ATTR_VERSION_MONOTONIC;
> > +	if (server->change_attr_type != NFS4_CHANGE_TYPE_IS_UNDEFINED)
> > +		stat->attributes |= STATX_ATTR_VERSION_MONOTONIC;
> 
> So if the server tells us that the change attrs is based on time
> metadata, we accept that it will be monotonic (and RFC7862 encourages
> this), even though we seem to worry about timestamps going backwards
> (which we know that can)...  Interesting.
> 
> 

I followed suit from nfs_inode_attrs_cmp(). It seems to treat any value
that isn't UNDEFINED as MONOTONIC, though it does use a less strict
comparator for NFS4_CHANGE_TYPE_IS_TIME_METADATA. It may make sense to
carve that out as an exception.

This is probably an indicator that we need a more strict definition for
STATX_ATTR_VERSION_MONOTONIC.


> 
> >  	if (S_ISDIR(inode->i_mode))
> >  		stat->blksize = NFS_SERVER(inode)->dtsize;
> >  out:
> > -- 
> > 2.37.3
> > 
> >
NeilBrown Oct. 4, 2022, 10:27 p.m. UTC | #3 
On Tue, 04 Oct 2022, Jeff Layton wrote:
> On Tue, 2022-10-04 at 10:29 +1100, NeilBrown wrote:
> > On Fri, 30 Sep 2022, Jeff Layton wrote:
> > > Allow NFS to report the i_version in getattr requests. Since the cost to
> > > fetch it is relatively cheap, do it unconditionally and just set the
> > > flag if it looks like it's valid. Also, conditionally enable the
> > > MONOTONIC flag when the server reports its change attr type as such.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/nfs/inode.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index bea7c005119c..5cb7017e5089 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -830,6 +830,8 @@ static u32 nfs_get_valid_attrmask(struct inode *inode)
> > >  		reply_mask |= STATX_UID | STATX_GID;
> > >  	if (!(cache_validity & NFS_INO_INVALID_BLOCKS))
> > >  		reply_mask |= STATX_BLOCKS;
> > > +	if (!(cache_validity & NFS_INO_INVALID_CHANGE))
> > > +		reply_mask |= STATX_VERSION;
> > >  	return reply_mask;
> > >  }
> > >  
> > > @@ -848,7 +850,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> > >  
> > >  	request_mask &= STATX_TYPE | STATX_MODE | STATX_NLINK | STATX_UID |
> > >  			STATX_GID | STATX_ATIME | STATX_MTIME | STATX_CTIME |
> > > -			STATX_INO | STATX_SIZE | STATX_BLOCKS;
> > > +			STATX_INO | STATX_SIZE | STATX_BLOCKS | STATX_VERSION;
> > >  
> > >  	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
> > >  		if (readdirplus_enabled)
> > > @@ -877,7 +879,7 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> > >  	/* Is the user requesting attributes that might need revalidation? */
> > >  	if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> > >  					STATX_MTIME|STATX_UID|STATX_GID|
> > > -					STATX_SIZE|STATX_BLOCKS)))
> > > +					STATX_SIZE|STATX_BLOCKS|STATX_VERSION)))
> > >  		goto out_no_revalidate;
> > >  
> > >  	/* Check whether the cached attributes are stale */
> > > @@ -915,6 +917,10 @@ int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> > >  
> > >  	generic_fillattr(&init_user_ns, inode, stat);
> > >  	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> > > +	stat->version = inode_peek_iversion_raw(inode);
> > 
> > This looks wrong.
> > 1/ it includes the I_VERSION_QUERIED bit, which should be hidden.
> > 2/ it doesn't set that bit.
> > 
> > I understand that the bit was already set when the generic code called
> > inode_query_iversion(), but it might have changed if we needed to
> > refresh the attrs.
> > 
> > I'm beginning to think I shouldn't have approved the 3/9 patch.  The
> > stat->version shouldn't be set in vfs_getattr_nosec() - maybe in
> > generic_fillattr(), but not a lot of point.
> > 
> 
> NFS (and Ceph), do not set the SB_I_VERSION flag and they don't use the
> QUERIED bit. These are "server managed" implementations of i_version.
> The server is responsible for incrementing the value, and we just store
> the result in the i_version field and present it when needed. That's why
> the patch for NFS is using the "raw" API.

Ahh - of course.  I got confused because the "raw" api is used by code
(in iversion.h) that wants to access the QUERIED bit.  Maybe having
different names would help.  Or maybe me re-familiarising myself with
the interfaces would help...

Reviewed-by: NeilBrown <neilb@suse.de>


> 
> > > +	stat->attributes_mask |= STATX_ATTR_VERSION_MONOTONIC;
> > > +	if (server->change_attr_type != NFS4_CHANGE_TYPE_IS_UNDEFINED)
> > > +		stat->attributes |= STATX_ATTR_VERSION_MONOTONIC;
> > 
> > So if the server tells us that the change attrs is based on time
> > metadata, we accept that it will be monotonic (and RFC7862 encourages
> > this), even though we seem to worry about timestamps going backwards
> > (which we know that can)...  Interesting.
> > 
> > 
> 
> I followed suit from nfs_inode_attrs_cmp(). It seems to treat any value
> that isn't UNDEFINED as MONOTONIC, though it does use a less strict
> comparator for NFS4_CHANGE_TYPE_IS_TIME_METADATA. It may make sense to
> carve that out as an exception.
> 
> This is probably an indicator that we need a more strict definition for
> STATX_ATTR_VERSION_MONOTONIC.

Maybe.  Or maybe we decide that if the system time goes backwards and
things break, then you get to keep both halves.
The pedant in me want to handle that properly.  The pragmatist doesn't
think it is worth it.

Thanks,
NeilBrown


> 
> 
> > 
> > >  	if (S_ISDIR(inode->i_mode))
> > >  		stat->blksize = NFS_SERVER(inode)->dtsize;
> > >  out:
> > > -- 
> > > 2.37.3
> > > 
> > > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
>
diff mbox series

Patch

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index bea7c005119c..5cb7017e5089 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -830,6 +830,8 @@  static u32 nfs_get_valid_attrmask(struct inode *inode)
 		reply_mask |= STATX_UID | STATX_GID;
 	if (!(cache_validity & NFS_INO_INVALID_BLOCKS))
 		reply_mask |= STATX_BLOCKS;
+	if (!(cache_validity & NFS_INO_INVALID_CHANGE))
+		reply_mask |= STATX_VERSION;
 	return reply_mask;
 }
 
@@ -848,7 +850,7 @@  int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 
 	request_mask &= STATX_TYPE | STATX_MODE | STATX_NLINK | STATX_UID |
 			STATX_GID | STATX_ATIME | STATX_MTIME | STATX_CTIME |
-			STATX_INO | STATX_SIZE | STATX_BLOCKS;
+			STATX_INO | STATX_SIZE | STATX_BLOCKS | STATX_VERSION;
 
 	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
 		if (readdirplus_enabled)
@@ -877,7 +879,7 @@  int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 	/* Is the user requesting attributes that might need revalidation? */
 	if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
 					STATX_MTIME|STATX_UID|STATX_GID|
-					STATX_SIZE|STATX_BLOCKS)))
+					STATX_SIZE|STATX_BLOCKS|STATX_VERSION)))
 		goto out_no_revalidate;
 
 	/* Check whether the cached attributes are stale */
@@ -915,6 +917,10 @@  int nfs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 
 	generic_fillattr(&init_user_ns, inode, stat);
 	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
+	stat->version = inode_peek_iversion_raw(inode);
+	stat->attributes_mask |= STATX_ATTR_VERSION_MONOTONIC;
+	if (server->change_attr_type != NFS4_CHANGE_TYPE_IS_UNDEFINED)
+		stat->attributes |= STATX_ATTR_VERSION_MONOTONIC;
 	if (S_ISDIR(inode->i_mode))
 		stat->blksize = NFS_SERVER(inode)->dtsize;
 out: