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

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

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

quoted

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

quoted

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.

quoted

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.

Ah, you are talking about this:

    /* 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)))
        goto out_no_update;

Well, if this is triggered for statx(...,  STATX_ATIME,
AT_STATX_SYNC_AS_STAT) and MNT_NOATIME, then yes, result will be
junk.
Which means that the code is wrong, it shouldn't do that.

The problem is that vfs_getattr_nosec() populates stat->result_mask
with a default of STATX_BASIC_STATS, which makes no sense unless you
assume that the user will always ask for a superset of
STATX_BASIC_STATS (or you assume that those attributes never need
revalidation, which is obviously braindead).

Otherwise (if something other than STATX_ATIME or STATX_INO or
STATX_TYPE is given as well) it *will* do the same thing as what
stat(2) does, so in that case STATX_ATIME should not  be cleared (yet
it is cleared).

As far as I'm concerned, we can definitely get rid of the

        /*
         * We may force a getattr if the user cares about atime.
         *
         * Note that we only have to check the vfsmount flags here:
         *  - NFS always sets S_NOATIME by so checking it would give a
         *    bogus result
         *  - NFS never sets SB_NOATIME or SB_NODIRATIME so there is
         *    no point in checking those.
         */
        if ((path->mnt->mnt_flags & MNT_NOATIME) ||
            ((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
                request_mask &= ~STATX_ATIME;


however the rest needs to stay, or there is no way we can use statx()
to allow optimised retrieval of only those attributes that your
application cares about.
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com