Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask

[Trond Myklebust]

On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
> On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust
> <trondmy@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote:
> > > As per statx(2) man page only clear out flags that are
> > > unsupported by
> > > the
> > > fs or have an unrepresentable value.  Atime is supported by NFS
> > > as
> > > long as
> > > it's supported on the server.
> > > 
> > > So the STATX_ATIME flag should not be cleared in the result_mask
> > > if
> > > the
> > > operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount.
> > > 
> > > This patch doesn't change the revalidation algorithm in any way,
> > > just
> > > the
> > > clearing of flags in stat->result_mask.
> > > 
> > > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > > Fixes: 9ccee940bd5b ("Support statx() mask and query flags
> > > parameters")
> > > Cc: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > > ---
> > >  fs/nfs/inode.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index b65aee481d13..34bb3e591709 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path,
> > > struct
> > > kstat *stat,
> > >       if (!(request_mask &
> > > (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> > >                                       STATX_MTIME|STATX_UID|STATX
> > > _GID
> > > > 
> > > 
> > >                                       STATX_SIZE|STATX_BLOCKS)))
> > > -             goto out_no_revalidate;
> > > +             goto out_no_update;
> > > 
> > >       /* Check whether the cached attributes are stale */
> > >       do_update |= force_sync ||
> > > nfs_attribute_cache_expired(inode);
> > > @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path,
> > > struct
> > > kstat *stat,
> > >                       goto out;
> > >       } else
> > >               nfs_readdirplus_parent_cache_hit(path->dentry);
> > > -out_no_revalidate:
> > > -     /* Only return attributes that were revalidated. */
> > > -     stat->result_mask &= request_mask;
> > >  out_no_update:
> > >       generic_fillattr(inode, stat);
> > >       stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> > 
> > NACK.
> > 
> > When we don't revalidate the attribute, then the content of the
> > field
> > contains junk values. The above code is very intentional.
> 
> How is it then that only STATX_ATIME is cleared and not the other
> fields?

It isn't just the atime. We can also fail to revalidate the ctime and
mtime if they are not being requested by the user.

> 
> Note: junk != stale.  The statx definition doesn't talk about the
> fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
> attributes are okay, and do not warrant clearing the result_mask.
> 

I disagree. stale == junk here, because the default of
AT_STATX_SYNC_AS_STAT is described by the manpage as "Do  whatever
stat(2) does." which this is not.

The default behaviour for "stat(2)" is to revalidate attributes that we
know or suspect are stale. We never knowingly return stale attributes.

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