[PATCH v2 1/5] orangefs: fix request_mask misuse

[PATCH v2 1/5] orangefs: fix request_mask misuse

[Miklos Szeredi]

Orangefs only handles STATX_BASIC_STATS in its getattr implementation, so
mask off all other flags.  Not doing so results in statx(2) forcing a
refresh of cached attributes on any other requested flag (i.e. STATX_BTIME
currently) due to the following test in orangefs_inode_getattr():

  (request_mask & orangefs_inode->getattr_mask) == request_mask

Also clean up gratuitous uses of STATX_ALL.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Reviewed-by: Martin Brandenburg <martin@omnibond.com>
Cc: Mike Marshall <hubcap@omnibond.com>
---
 fs/orangefs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 31932879b716..bd7f15a831dc 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -256,7 +256,8 @@ int orangefs_getattr(const struct path *path, struct kstat *stat,
 		     "orangefs_getattr: called on %pd\n",
 		     path->dentry);
 
-	ret = orangefs_inode_getattr(inode, 0, 0, request_mask);
+	ret = orangefs_inode_getattr(inode, 0, 0,
+				     request_mask & STATX_BASIC_STATS);
 	if (ret == 0) {
 		generic_fillattr(inode, stat);
 
@@ -408,7 +409,7 @@ struct inode *orangefs_iget(struct super_block *sb,
 	if (!inode || !(inode->i_state & I_NEW))
 		return inode;
 
-	error = orangefs_inode_getattr(inode, 1, 1, STATX_ALL);
+	error = orangefs_inode_getattr(inode, 1, 1, STATX_BASIC_STATS);
 	if (error) {
 		iget_failed(inode);
 		return ERR_PTR(error);
@@ -453,7 +454,7 @@ struct inode *orangefs_new_inode(struct super_block *sb, struct inode *dir,
 	orangefs_set_inode(inode, ref);
 	inode->i_ino = hash;	/* needed for stat etc */
 
-	error = orangefs_inode_getattr(inode, 1, 1, STATX_ALL);
+	error = orangefs_inode_getattr(inode, 1, 1, STATX_BASIC_STATS);
 	if (error)
 		goto out_iput;
 
-- 
2.14.3

[PATCH v2 2/5] uapi: deprecate STATX_ALL

[Miklos Szeredi]

Constants of the *_ALL type can be actively harmful due to the fact that
developers will usually fail to consider the possible effects of future
changes to the definition.

Deprecate STATX_ALL in the uapi, while no damage has been done yet.

We could keep something like this around in the kernel, but there's
actually no point, since all filesystems should be explicitly checking
flags that they support and not rely on the VFS masking unknown ones out: a
flag could be known to the VFS, yet not known to the filesystem (see
orangefs bug fixed in the previous patch).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
 fs/stat.c                       |  1 -
 include/uapi/linux/stat.h       | 11 ++++++++++-
 samples/statx/test-statx.c      |  2 +-
 tools/include/uapi/linux/stat.h | 11 ++++++++++-
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index f8e6fb2c3657..8d297a279991 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -73,7 +73,6 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 
 	memset(stat, 0, sizeof(*stat));
 	stat->result_mask |= STATX_BASIC_STATS;
-	request_mask &= STATX_ALL;
 	query_flags &= KSTAT_QUERY_FLAGS;
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(path, stat, request_mask,
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 7b35e98d3c58..ed456ac0f90d 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -148,9 +148,18 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
-#define STATX_ALL		0x00000fffU	/* All currently supported flags */
+
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
+#ifndef __KERNEL__
+/*
+ * This is deprecated, and shall remain the same value in the future.  To avoid
+ * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
+ * instead.
+ */
+#define STATX_ALL		0x00000fffU
+#endif
+
 /*
  * Attributes to be found in stx_attributes and masked in stx_attributes_mask.
  *
diff --git a/samples/statx/test-statx.c b/samples/statx/test-statx.c
index d4d77b09412c..e354048dea3c 100644
--- a/samples/statx/test-statx.c
+++ b/samples/statx/test-statx.c
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
 	struct statx stx;
 	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
 
-	unsigned int mask = STATX_ALL;
+	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
 
 	for (argv++; *argv; argv++) {
 		if (strcmp(*argv, "-F") == 0) {
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index 7b35e98d3c58..ed456ac0f90d 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -148,9 +148,18 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
-#define STATX_ALL		0x00000fffU	/* All currently supported flags */
+
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
+#ifndef __KERNEL__
+/*
+ * This is deprecated, and shall remain the same value in the future.  To avoid
+ * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
+ * instead.
+ */
+#define STATX_ALL		0x00000fffU
+#endif
+
 /*
  * Attributes to be found in stx_attributes and masked in stx_attributes_mask.
  *
-- 
2.14.3

[PATCH v2 3/5] statx: add STATX_ATTRIBUTES flag

[Miklos Szeredi]

FUSE will want to know if stx_attributes is interesting or not, because
there's a non-zero cost of retreiving it.

This just a "want" flag, since stx_attributes_mask already indicates
whether we "got" stx_attributes or not.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
 include/uapi/linux/stat.h       | 1 +
 samples/statx/test-statx.c      | 2 +-
 tools/include/uapi/linux/stat.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index ed456ac0f90d..7d3cce078652 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -148,6 +148,7 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
+#define STATX_ATTRIBUTES	0x00001000U	/* Want stx_attributes */
 
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
diff --git a/samples/statx/test-statx.c b/samples/statx/test-statx.c
index e354048dea3c..deef9a68ff68 100644
--- a/samples/statx/test-statx.c
+++ b/samples/statx/test-statx.c
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
 	struct statx stx;
 	int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW;
 
-	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME;
+	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_ATTRIBUTES;
 
 	for (argv++; *argv; argv++) {
 		if (strcmp(*argv, "-F") == 0) {
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index ed456ac0f90d..60cd0a3f52e7 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -148,6 +148,7 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
+#define STATX_ATTRIBUTES	0x00001000U	/* Want/got stx_attributes */
 
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
-- 
2.14.3

[PATCH v2 4/5] statx: don't clear STATX_ATIME on SB_RDONLY

[Miklos Szeredi]

IS_NOATIME(inode) is defined as __IS_FLG(inode, SB_RDONLY|SB_NOATIME), so
generic_fillattr() will clear STATX_ATIME from the result_mask if the super
block is marked read only.

This was probably not the intention, so fix to only clear STATX_ATIME if
the fs doesn't support atime at all.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: David Howells <dhowells@redhat.com>
---
 fs/stat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/stat.c b/fs/stat.c
index 8d297a279991..b46583df70d4 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -46,7 +46,8 @@ void generic_fillattr(struct inode *inode, struct kstat *stat)
 	stat->blksize = i_blocksize(inode);
 	stat->blocks = inode->i_blocks;
 
-	if (IS_NOATIME(inode))
+	/* SB_NOATIME means filesystem supplies dummy atime value */
+	if (inode->i_sb->s_flags & SB_NOATIME)
 		stat->result_mask &= ~STATX_ATIME;
 	if (IS_AUTOMOUNT(inode))
 		stat->attributes |= STATX_ATTR_AUTOMOUNT;
-- 
2.14.3

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

[Miklos Szeredi]

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));
-- 
2.14.3

Re: [PATCH v2 1/5] orangefs: fix request_mask misuse

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Oct 19, 2018, at 6:20 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> 
> Orangefs only handles STATX_BASIC_STATS in its getattr implementation, so
> mask off all other flags.  Not doing so results in statx(2) forcing a
> refresh of cached attributes on any other requested flag (i.e. STATX_BTIME
> currently) due to the following test in orangefs_inode_getattr():
> 
> (request_mask & orangefs_inode->getattr_mask) == request_mask
> 
> Also clean up gratuitous uses of STATX_ALL.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Reviewed-by: Martin Brandenburg <martin@omnibond.com>
> Cc: Mike Marshall <hubcap@omnibond.com>
> ---
> fs/orangefs/inode.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index 31932879b716..bd7f15a831dc 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -256,7 +256,8 @@ int orangefs_getattr(const struct path *path, struct kstat *stat,
> 		     "orangefs_getattr: called on %pd\n",
> 		     path->dentry);
> 
> -	ret = orangefs_inode_getattr(inode, 0, 0, request_mask);
> +	ret = orangefs_inode_getattr(inode, 0, 0,
> +				     request_mask & STATX_BASIC_STATS);

Does it make sense to mask this off at the caller, rather than within
orangefs_inode_getattr()?  Otherwise, orangefs_inode_getattr() will
never see additional stats passed in, even if it is enhanced to return
other values.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> +/*
> + * This is deprecated, and shall remain the same value in the future.  To avoid
> + * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
> + * instead.
> + */
> +#define STATX_ALL		0x00000fffU

The comment is misleading.  STATX_ALL is *not* equivalent to
STATX_BASIC_STATS | STATX_BTIME, even though it might be numerically the
same.  You would need to update the comment when you add STATX_ATTRIBUTES
to mention that also.

Apart from that, I'm okay with this.

Further, please provide a manpage update also.

David

Re: [PATCH v2 3/5] statx: add STATX_ATTRIBUTES flag

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> FUSE will want to know if stx_attributes is interesting or not, because
> there's a non-zero cost of retreiving it.
> 
> This just a "want" flag, since stx_attributes_mask already indicates
> whether we "got" stx_attributes or not.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>

Subject to fixing the comment on STAT_ALL:

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v2 4/5] statx: don't clear STATX_ATIME on SB_RDONLY

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> IS_NOATIME(inode) is defined as __IS_FLG(inode, SB_RDONLY|SB_NOATIME), so
> generic_fillattr() will clear STATX_ATIME from the result_mask if the super
> block is marked read only.
> 
> This was probably not the intention, so fix to only clear STATX_ATIME if
> the fs doesn't support atime at all.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL

[Miklos Szeredi]

On Fri, Oct 19, 2018 at 5:35 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> +/*
>> + * This is deprecated, and shall remain the same value in the future.  To avoid
>> + * confusion please use the equivalent (STATX_BASIC_STATS | STATX_BTIME)
>> + * instead.
>> + */
>> +#define STATX_ALL            0x00000fffU
>
> The comment is misleading.  STATX_ALL is *not* equivalent to
> STATX_BASIC_STATS | STATX_BTIME, even though it might be numerically the
> same.  You would need to update the comment when you add STATX_ATTRIBUTES
> to mention that also.

The definition of STATX_ALL is, and will remain, equivalent to
STATX_BASIC_STATS | STATX_BTIME.

What is misleading about this?

If you feel confused by this comment, then maybe I should just drop that part.

Thanks,
Miklos

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

[David Howells]

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>

Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> What is misleading about this?

The manpage says:

           STATX_ALL           [All currently available fields]

> If you feel confused by this comment, then maybe I should just drop that part.

I think that would be better.  Don't try to give it a definition.

David

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL

[Miklos Szeredi]

On Fri, Oct 19, 2018 at 5:47 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> What is misleading about this?
>
> The manpage says:
>
>            STATX_ALL           [All currently available fields]

Ah, the manpage needs to be fixed, obviously.

>
>> If you feel confused by this comment, then maybe I should just drop that part.
>
> I think that would be better.  Don't try to give it a definition.

Okay.

Thanks,
Miklos

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

[Miklos Szeredi]

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

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Miklos Szeredi]

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

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

[Trond Myklebust]

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.
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]

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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]