[PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[Miklos Szeredi]

This request_mask flag indicates that the caller is interested in the
result_mask.  At the moment all ->getattr() callers discard the result mask
except sys_statx().

FUSE needs this, because it uses legacy inode initialization, that doesn't
return a result_mask, so needs a refresh when caller asks for it with
statx().

It might make sense later to promote this to a proper statx mask flag and
return it in stx_mask to userspace.  This needs some more work to make sure
only filesystems set this flag where the result_mask is valid (e.g. NFS
wouldn't be able to set it, since it doesn't query the server about
supported fields).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/stat.c            | 1 +
 include/linux/stat.h | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/fs/stat.c b/fs/stat.c
index b46583df70d4..0b71e5bdbc6b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -576,6 +576,7 @@ SYSCALL_DEFINE5(statx,
 	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
 		return -EINVAL;
 
+	mask |= STATX_RESULT_MASK;
 	error = vfs_statx(dfd, filename, flags, &stat, mask);
 	if (error)
 		return error;
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 765573dc17d6..65448b1163d3 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -21,6 +21,12 @@
 
 #define KSTAT_QUERY_FLAGS (AT_STATX_SYNC_TYPE)
 
+/*
+ * This is an internal mask value, meaning: caller is interested in
+ * .result_mask, i.e. it's sys_statx().
+ */
+#define STATX_RESULT_MASK STATX__RESERVED
+
 struct kstat {
 	u32		result_mask;	/* What fields the user got */
 	umode_t		mode;
-- 
2.14.3

Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> FUSE needs this, because it uses legacy inode initialization, that doesn't
> return a result_mask, so needs a refresh when caller asks for it with
> statx().

Can't you just make it up in fuse?  Presumably, fuse doesn't support any of
the non-basic statx fields either?

> It might make sense later to promote this to a proper statx mask flag and
> return it in stx_mask to userspace.

That sounds kind of recursive - a bit in stx_mask would be saying whether or
not stx_mask can be used.

Besides, what would it mean if that bit says you can't use stx_mask?  None of
the stx_* fields are valid?

> +#define STATX_RESULT_MASK STATX__RESERVED

Please don't use that bit.


Sorry, this patch doesn't make sense.  Just set result_mask to
STATX_BASIC_STATS in fuse_fill_attr().

David

Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[Miklos Szeredi]

On Fri, Oct 19, 2018 at 5:59 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> FUSE needs this, because it uses legacy inode initialization, that doesn't
>> return a result_mask, so needs a refresh when caller asks for it with
>> statx().
>
> Can't you just make it up in fuse?  Presumably, fuse doesn't support any of
> the non-basic statx fields either?

This is very much about the basic statx fields.  I.e. what does
result_mask == STATX_BASIC_STATS means?

 a) this is a legacy stat() implementation that doesn't fill in the
result_mask properly

 b) this is an up-todate stat() implementation that does fill the
result_mask properly and all fields are valid

There's no way to tell which one it is, and no way to request that
filesystem try b) even if it's more costly.

>> It might make sense later to promote this to a proper statx mask flag and
>> return it in stx_mask to userspace.
>
> That sounds kind of recursive - a bit in stx_mask would be saying whether or
> not stx_mask can be used.

Yes.  See above.

>> +#define STATX_RESULT_MASK STATX__RESERVED
>
> Please don't use that bit.

Using it internally is perfectly harmless.   If we'll need to extend
statx in the future and make use of this flag externally, then we can
easily move the internal flag somewhere else (e.g. extend request_mask
to 64bit, which we'll probably need to do anyway in that case).

Thanks,
Miklos

Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[Andreas Dilger]

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

On Oct 19, 2018, at 11:42 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> +#define STATX_RESULT_MASK STATX__RESERVED
>> 
>> Please don't use that bit.
> 
> Using it internally is perfectly harmless.   If we'll need to extend
> statx in the future and make use of this flag externally, then we can
> easily move the internal flag somewhere else (e.g. extend request_mask
> to 64bit, which we'll probably need to do anyway in that case).

I was thinking about this - what is the point of returning an error
if STATX__RESERVED is set?  If this is used to indicate the presence
of e.g. stx_mask2, then newer applications trying to request any of the
flags encoded into stx_mask2 will get an error, rather than the expected
behaviour of "ignore flags you don't understand, and don't set them in
the return stx_mask".

Essentially, this will make STATX__RESERVED useless in the future, since
no application will be able to use it without getting an error if they
are running on an old kernel.

Cheers, Andreas






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

Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> This is very much about the basic statx fields.  I.e. what does
> result_mask == STATX_BASIC_STATS means?
> 
>  a) this is a legacy stat() implementation that doesn't fill in the
> result_mask properly
> 
>  b) this is an up-todate stat() implementation that does fill the
> result_mask properly and all fields are valid
> 
> There's no way to tell which one it is, and no way to request that
> filesystem try b) even if it's more costly.

Okay, I think I see what you're getting at.  We need to be able to tell the
user that we don't actually know what's good and what's not.  I guess this
doesn't only apply to fuse, but could also apply to nfs potentially as nfs
doesn't necessarily know what the server is capable of and where it's making
stuff up (imagine an NFS server backing both an ext4 and a fat filesystem).

I would be tempted to represent this with an attribute flag instead, say:

	STATX_ATTR_UNCERTAIN_VALUES

Your attributes are actually in one of at least three states, not two:
unsupported, definite and uncertain.  Definite things might include such as
STATX_TYPE - after all, if you don't know what type a file is, you can't
really use it.

If someone really needs to know, it might be worth deferring further
information to fsinfo().

David