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

[Andreas Dilger]

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