On Oct 20, 2018, at 11:46 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > 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 & (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 >> 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
Attachment:
signature.asc
Description: Message signed with OpenPGP
