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

On Oct 20, 2018, at 11:46 AM, Trond Myklebust [off-list ref] wrote:

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

quoted

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).

I guess the assumption in the VFS code is that statx is mostly called
by local filesystems, for which STATX_BASIC_STATS is usually right,
so the basic VFS helper is OK to set those stats.  It should also be
possible for the filesystem to clear flags out of result_mask for
attributes that it doesn't want to return.

For filesystems that know what they are doing, it might just be best
to always clear stat->result_mask and fill in what they want, based
on the available attributes and request_mask rather than assuming
something is set by the caller.

Cheers, Andreas

quoted

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.

Cheers, Andreas