On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:

On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust
[off-list ref] wrote:

quoted

On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote:

quoted

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 <redacted>
Fixes: 9ccee940bd5b ("Support statx() mask and query flags
parameters")
Cc: Trond Myklebust <redacted>
---
 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

quoted

                                      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@hammerspace.com