| Message ID | 20181019122049.27121-5-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] orangefs: fix request_mask misuse | expand |
Miklos Szeredi <mszeredi@redhat.com> wrote: > As per statx(2) man page only clear out flags that are unsupported by the > fs or have an unrepresentable value. Atime is supported by NFS as long as > it's supported on the server. > > So the STATX_ATIME flag should not be cleared in the result_mask if the > operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount. > > This patch doesn't change the revalidation algorithm in any way, just the > clearing of flags in stat->result_mask. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Fixes: 9ccee940bd5b ("Support statx() mask and query flags parameters") > Cc: Trond Myklebust <trond.myklebust@primarydata.com> Reviewed-by: David Howells <dhowells@redhat.com>
On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote: > As per statx(2) man page only clear out flags that are unsupported by > the > fs or have an unrepresentable value. Atime is supported by NFS as > long as > it's supported on the server. > > So the STATX_ATIME flag should not be cleared in the result_mask if > the > operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount. > > This patch doesn't change the revalidation algorithm in any way, just > the > clearing of flags in stat->result_mask. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Fixes: 9ccee940bd5b ("Support statx() mask and query flags > parameters") > Cc: Trond Myklebust <trond.myklebust@primarydata.com> > --- > fs/nfs/inode.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > index b65aee481d13..34bb3e591709 100644 > --- a/fs/nfs/inode.c > +++ b/fs/nfs/inode.c > @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path, struct > kstat *stat, > if (!(request_mask & > (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME| > STATX_MTIME|STATX_UID|STATX_GID > | > STATX_SIZE|STATX_BLOCKS))) > - goto out_no_revalidate; > + goto out_no_update; > > /* Check whether the cached attributes are stale */ > do_update |= force_sync || nfs_attribute_cache_expired(inode); > @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path, struct > kstat *stat, > goto out; > } else > nfs_readdirplus_parent_cache_hit(path->dentry); > -out_no_revalidate: > - /* Only return attributes that were revalidated. */ > - stat->result_mask &= request_mask; > out_no_update: > generic_fillattr(inode, stat); > stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode)); NACK. When we don't revalidate the attribute, then the content of the field contains junk values. The above code is very intentional.
On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote: >> As per statx(2) man page only clear out flags that are unsupported by >> the >> fs or have an unrepresentable value. Atime is supported by NFS as >> long as >> it's supported on the server. >> >> So the STATX_ATIME flag should not be cleared in the result_mask if >> the >> operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount. >> >> This patch doesn't change the revalidation algorithm in any way, just >> the >> clearing of flags in stat->result_mask. >> >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> >> Fixes: 9ccee940bd5b ("Support statx() mask and query flags >> parameters") >> Cc: Trond Myklebust <trond.myklebust@primarydata.com> >> --- >> fs/nfs/inode.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c >> index b65aee481d13..34bb3e591709 100644 >> --- a/fs/nfs/inode.c >> +++ b/fs/nfs/inode.c >> @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path, struct >> kstat *stat, >> if (!(request_mask & >> (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME| >> STATX_MTIME|STATX_UID|STATX_GID >> | >> STATX_SIZE|STATX_BLOCKS))) >> - goto out_no_revalidate; >> + goto out_no_update; >> >> /* Check whether the cached attributes are stale */ >> do_update |= force_sync || nfs_attribute_cache_expired(inode); >> @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path, struct >> kstat *stat, >> goto out; >> } else >> nfs_readdirplus_parent_cache_hit(path->dentry); >> -out_no_revalidate: >> - /* Only return attributes that were revalidated. */ >> - stat->result_mask &= request_mask; >> out_no_update: >> generic_fillattr(inode, stat); >> stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode)); > > NACK. > > When we don't revalidate the attribute, then the content of the field > contains junk values. The above code is very intentional. How is it then that only STATX_ATIME is cleared and not the other fields? 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. Thanks, Miklos
On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote: > On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust > <trondmy@hammerspace.com> wrote: > > On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote: > > > As per statx(2) man page only clear out flags that are > > > unsupported by > > > the > > > fs or have an unrepresentable value. Atime is supported by NFS > > > as > > > long as > > > it's supported on the server. > > > > > > So the STATX_ATIME flag should not be cleared in the result_mask > > > if > > > the > > > operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount. > > > > > > This patch doesn't change the revalidation algorithm in any way, > > > just > > > the > > > clearing of flags in stat->result_mask. > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > Fixes: 9ccee940bd5b ("Support statx() mask and query flags > > > parameters") > > > Cc: Trond Myklebust <trond.myklebust@primarydata.com> > > > --- > > > fs/nfs/inode.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > > > index b65aee481d13..34bb3e591709 100644 > > > --- a/fs/nfs/inode.c > > > +++ b/fs/nfs/inode.c > > > @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path, > > > struct > > > kstat *stat, > > > if (!(request_mask & > > > (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME| > > > STATX_MTIME|STATX_UID|STATX > > > _GID > > > > > > > > > > STATX_SIZE|STATX_BLOCKS))) > > > - goto out_no_revalidate; > > > + goto out_no_update; > > > > > > /* Check whether the cached attributes are stale */ > > > do_update |= force_sync || > > > nfs_attribute_cache_expired(inode); > > > @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path, > > > struct > > > kstat *stat, > > > goto out; > > > } else > > > nfs_readdirplus_parent_cache_hit(path->dentry); > > > -out_no_revalidate: > > > - /* Only return attributes that were revalidated. */ > > > - stat->result_mask &= request_mask; > > > out_no_update: > > > generic_fillattr(inode, stat); > > > stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode)); > > > > NACK. > > > > When we don't revalidate the attribute, then the content of the > > field > > contains junk values. The above code is very intentional. > > 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. The default behaviour for "stat(2)" is to revalidate attributes that we know or suspect are stale. We never knowingly return stale attributes.
On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust <trondmy@hammerspace.com> 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. 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). I can do a patch, but not tonight... Thanks, Miklos Thanks, Miklos
On Fri, 2018-10-19 at 22:48 +0200, Miklos Szeredi wrote: > On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust > <trondmy@hammerspace.com> 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). > 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.
On Oct 20, 2018, at 11:46 AM, Trond Myklebust <trondmy@hammerspace.com> 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@hammerspace.com> 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
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index b65aee481d13..34bb3e591709 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path, struct kstat *stat, if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME| STATX_MTIME|STATX_UID|STATX_GID| STATX_SIZE|STATX_BLOCKS))) - goto out_no_revalidate; + goto out_no_update; /* Check whether the cached attributes are stale */ do_update |= force_sync || nfs_attribute_cache_expired(inode); @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path, struct kstat *stat, goto out; } else nfs_readdirplus_parent_cache_hit(path->dentry); -out_no_revalidate: - /* Only return attributes that were revalidated. */ - stat->result_mask &= request_mask; out_no_update: generic_fillattr(inode, stat); stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
As per statx(2) man page only clear out flags that are unsupported by the fs or have an unrepresentable value. Atime is supported by NFS as long as it's supported on the server. So the STATX_ATIME flag should not be cleared in the result_mask if the operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount. This patch doesn't change the revalidation algorithm in any way, just the clearing of flags in stat->result_mask. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Fixes: 9ccee940bd5b ("Support statx() mask and query flags parameters") Cc: Trond Myklebust <trond.myklebust@primarydata.com> --- fs/nfs/inode.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)