[PATCH 1/3] orangefs: fix request_mask misuse

[PATCH 1/3] 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>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Martin Brandenburg <martin@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 2/3] uapi: get rid of 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.

Remove STATX_ALL from 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       | 2 +-
 samples/statx/test-statx.c      | 2 +-
 tools/include/uapi/linux/stat.h | 2 +-
 4 files changed, 3 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..370f09d92fa6 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -148,7 +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_ALL		0x00000fffU	/* All currently supported flags */
+
 #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 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..370f09d92fa6 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -148,7 +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_ALL		0x00000fffU	/* All currently supported flags */
+
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
 /*
-- 
2.14.3

[PATCH 3/3] 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 is more of a "want" flag, since stx_attributes_mask already indicates
whether we "got" stx_attributes or not.  So for now we can just deal with
this flag in the generic code.

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

diff --git a/fs/stat.c b/fs/stat.c
index 8d297a279991..6bf86d57e2e3 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -535,6 +535,9 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
 	tmp.stx_size = stat->size;
 	tmp.stx_blocks = stat->blocks;
 	tmp.stx_attributes_mask = stat->attributes_mask;
+	/* Having anything in attributes_mask means attributes are valid. */
+	if (tmp.stx_attributes_mask)
+		tmp.stx_mask |= STATX_ATTRIBUTES;
 	tmp.stx_atime.tv_sec = stat->atime.tv_sec;
 	tmp.stx_atime.tv_nsec = stat->atime.tv_nsec;
 	tmp.stx_btime.tv_sec = stat->btime.tv_sec;
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 370f09d92fa6..aef0aba5dfe7 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/got 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 370f09d92fa6..aef0aba5dfe7 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

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Florian Weimer]

* Miklos Szeredi:

>  #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */

What about this?  Isn't it similar to STATX_ALL in the sense that we
don't know yet what it will mean?

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Miklos Szeredi]

On Thu, Oct 18, 2018 at 3:15 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Miklos Szeredi:
>
>>  #define STATX__RESERVED              0x80000000U     /* Reserved for future struct statx expansion */
>
> What about this?  Isn't it similar to STATX_ALL in the sense that we
> don't know yet what it will mean?

Kernel will return -EINVAL if request_mask contains STATX__RESERVED,
so it's definitely different from other flag values.

Specifying this in the UAPI sort of implies that other flag values
will *not* need a struct statx expansion, so it's safe to pass in any
random value not containing STATX__RESERVED on any past or future
kernel and it will not write beyond the current struct statx boundary.
Not sure if that's a useful thing or not, but it's not actively
harmful, like the STATX_ALL flag.

Thanks,
Miklos

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Miklos Szeredi]

On Thu, Oct 18, 2018 at 4:32 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Oct 18, 2018 at 3:15 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * Miklos Szeredi:
>>
>>>  #define STATX__RESERVED              0x80000000U     /* Reserved for future struct statx expansion */
>>
>> What about this?  Isn't it similar to STATX_ALL in the sense that we
>> don't know yet what it will mean?
>
> Kernel will return -EINVAL if request_mask contains STATX__RESERVED,
> so it's definitely different from other flag values.
>
> Specifying this in the UAPI sort of implies that other flag values
> will *not* need a struct statx expansion, so it's safe to pass in any
> random value not containing STATX__RESERVED on any past or future
> kernel and it will not write beyond the current struct statx boundary.
> Not sure if that's a useful thing or not, but it's not actively
> harmful, like the STATX_ALL flag.

In other words, if STATX_ALL was defined as 0x7fffffff, then that
would mean the same thing, and I wouldn't complain about it.

Thanks,
Miklos

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Amir Goldstein]

On Thu, Oct 18, 2018 at 4:11 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> 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.
>
> Remove STATX_ALL from the uapi, while no damage has been done yet.
>

Look. When Linus says "let's see if somebody notices" and referring to ABI
it means sooner or later someone will upgrade to newer kernel and complain
if something breaks.

But what does it mean with UAPI change?
How often do people re-build existing programs?
I, for one, build master for my testing, but never install uapi
headers from master.
I just can't wrap my head around the backward compatibiltiy nightmare a change
like this could create.

How about just leave  STATX_ALL in uapi header to rot forever
mark it with a "deprecated" comment and #undef it out in include/linux/stat.h,
so we can't use it in the kernel anymore.
No use experimenting with pain.

BTW, man page needs to be fixes as well, because man page promotes
STATX_ALL.

Thanks,
Amir.

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> 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.
> 
> Remove STATX_ALL from the uapi, while no damage has been done yet.

You don't know that someone's not using it.  It's been there a year and a
half, long enough.  So, regretfully, I don't think we can remove it at this
point.

David

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

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> +	/* Having anything in attributes_mask means attributes are valid. */
> +	if (tmp.stx_attributes_mask)
> +		tmp.stx_mask |= STATX_ATTRIBUTES;

That would be superfluous, since userspace can make this check too.

Note that fsinfo() might inform you better - if and when it arrives.

David

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Florian Weimer]

* Amir Goldstein:

> On Thu, Oct 18, 2018 at 4:11 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>>
>> 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.
>>
>> Remove STATX_ALL from the uapi, while no damage has been done yet.
>>
>
> Look. When Linus says "let's see if somebody notices" and referring to ABI
> it means sooner or later someone will upgrade to newer kernel and complain
> if something breaks.
>
> But what does it mean with UAPI change?  How often do people
> re-build existing programs?  I, for one, build master for my
> testing, but never install uapi headers from master.  I just can't
> wrap my head around the backward compatibiltiy nightmare a change
> like this could create.

So it appears that people use #ifdef STATX_ALL to check for struct
statx availability.  So the backwards compatibility impact is that you
silently lose features in a consistent manner, which is very hard to
spot. 8-(

Probably not a good idea, then.

Re: [PATCH 1/3] orangefs: fix request_mask misuse

[martin]

On Thu, Oct 18, 2018 at 03:11:23PM +0200, Miklos Szeredi 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>
> Cc: Mike Marshall <hubcap@omnibond.com>
> Cc: Martin Brandenburg <martin@omnibond.com>

Reviewed-by: Martin Brandenburg <martin@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
> 

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Andreas Dilger]

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


> On Oct 18, 2018, at 7:15 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> 
> * Miklos Szeredi:
> 
>> #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
> 
> What about this?  Isn't it similar to STATX_ALL in the sense that we
> don't know yet what it will mean?

No, this means that this last bit will be used for increasing the size of the
stx_mask field at some point in the future.  IMHO, this is present as a reminder
to any developer who is adding fields in the future not to use the last flag.

Cheers, Andreas






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

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

[Andreas Dilger]

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

On Oct 18, 2018, at 7:11 AM, 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 is more of a "want" flag, since stx_attributes_mask already indicates
> whether we "got" stx_attributes or not.  So for now we can just deal with
> this flag in the generic code.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/stat.c                       | 3 +++
> include/uapi/linux/stat.h       | 1 +
> samples/statx/test-statx.c      | 2 +-
> tools/include/uapi/linux/stat.h | 1 +
> 4 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 8d297a279991..6bf86d57e2e3 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -535,6 +535,9 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> 	tmp.stx_size = stat->size;
> 	tmp.stx_blocks = stat->blocks;
> 	tmp.stx_attributes_mask = stat->attributes_mask;
> +	/* Having anything in attributes_mask means attributes are valid. */
> +	if (tmp.stx_attributes_mask)
> +		tmp.stx_mask |= STATX_ATTRIBUTES;
> 	tmp.stx_atime.tv_sec = stat->atime.tv_sec;
> 	tmp.stx_atime.tv_nsec = stat->atime.tv_nsec;
> 	tmp.stx_btime.tv_sec = stat->btime.tv_sec;
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 370f09d92fa6..aef0aba5dfe7 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/got 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 370f09d92fa6..aef0aba5dfe7 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
> 


Cheers, Andreas






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

Re: [PATCH 2/3] uapi: get rid of STATX_ALL

[Andreas Dilger]

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

On Oct 18, 2018, at 7:11 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> 
> 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.
> 
> Remove STATX_ALL from 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).

What is actually strange is that the vfs_getattr_nosec() code is setting

	stat->result_mask = STATX_BASIC_STATS;

in the code before any of the filesystem ->getattr methods are called.
That means it is up to the filesystems to actively *clear* flags from
the result_mask as opposed to only *setting* flags for fields that it
is filling in.  That seems a bit backward to me?

It looks like NFS is _probably_ doing the right thing, though it is still
using the userspace-supplied request_mask as a mask for the bits being
returned,  but the saving grace is that result_mask is STATX_BASIC_STATS
set by vfs_getattr_nosec() AFAICS.

Looking at OCFS2, CIFS, GFS2, they are doing a full inode revalidation
and returning the basic stats without looking at flags, request_mask,
or result_mask at all, so I'd expect they could be more efficient
(i.e. not revalidating the inode and possibly doing an RPC at all if
only some minimal flags are requested, or if AT_STATX_FORCE_SYNC is
not set).

It looks like overlayfs, nfsd, and fuse at least (as statx callers)
are often requesting a small subset of flags (e.g. STATX_INO,
STATX_NLINK, STATX_ATIME | STATX_MTIME, etc.) so they could be more
efficient if the underlying filesystems did only what was asked.

Cheers, Andreas

> 
> 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       | 2 +-
> samples/statx/test-statx.c      | 2 +-
> tools/include/uapi/linux/stat.h | 2 +-
> 4 files changed, 3 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..370f09d92fa6 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -148,7 +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_ALL		0x00000fffU	/* All currently supported flags */
> +
> #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 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..370f09d92fa6 100644
> --- a/tools/include/uapi/linux/stat.h
> +++ b/tools/include/uapi/linux/stat.h
> @@ -148,7 +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_ALL		0x00000fffU	/* All currently supported flags */
> +
> #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
> 
> /*
> --
> 2.14.3
> 


Cheers, Andreas






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