Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Trond Myklebust
Date: Sat Oct 20 2018 - 13:47:02 EST
On Fri, 2018-10-19 at 22:48 +0200, Miklos Szeredi wrote:
> On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust
> <trondmy@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
> > > 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.
> Ah, you are talking about this:
> /* Is the user requesting attributes that might need
> revalidation? */
> if (!(request_mask &
> 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
> 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.
Linux NFS client maintainer, Hammerspace