| Message ID | 20181019143932.6697-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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
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
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;
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(+)