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@xxxxxxxxxx>
> > ---
> > 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
> >
> >
--
Jeff Layton <jlayton@xxxxxxxxxx>
