* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-18 7:00 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810171256330.60837@chino.kir.corp.google.com>
On Wed 17-10-18 12:59:18, David Rientjes wrote:
> On Wed, 17 Oct 2018, Michal Hocko wrote:
>
> > Do you know of any other userspace except your usecase? Is there
> > anything fundamental that would prevent a proper API adoption for you?
> >
>
> Yes, it would require us to go back in time and build patched binaries.
I read that as there is a fundamental problem to update existing
binaries. If that is the case then there surely is no way around it
and another sad page in the screwed up APIs book we provide.
But I was under impression that the SW stack which actually does the
monitoring is under your controll. Moreover I was under impression that
you do not use the current vanilla kernel so there is no need for an
immediate change on your end. It is trivial to come up with a backward
compatible way to check for the new flag (if it is not present then
fallback to vma flags).
I am sorry for pushing here but if this is just a matter of a _single_
user which _can_ be fixed with a reasonable effort then I would love to
see the future api unscrewed.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-18 7:23 UTC (permalink / raw)
To: Andreas Dilger
Cc: Michael Kerrisk, David Howells, Linux FS-devel Mailing List,
Linux Kernel Mailing List, Linux API
In-Reply-To: <E9903911-9289-4038-9835-9353762A3BAF@dilger.ca>
On Wed, Oct 17, 2018 at 10:22 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> On Oct 17, 2018, at 1:04 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> So what's the point exactly?
>
> Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
> to mask off flags that it doesn't currently understand.
And even there it's quite pointless, since no sane implementation
would put the request mask into the result mask (aka. stx_mask)
verbatim, without picking out bits which correspond to *supported*
fields.
So it's pointless across the board. But since it's already published
in a userspace API, the best we can do is leave it at its current
value, but with a comment to warn that it should not be updated when
new flags are added. And remove all traces of it from the kernel.
>> Right, OTOH I sort of see the value in NFS: no roundtrip to server if
>> atime value is useless anyway.
>
> Well, "noatime" might be a client-only option, which doesn't mean that the
> server or some other client isn't updating the atime. Returning an old
> atime isn't the same as not returning anything at all.
Fs is allowed (and in case of basic stats, required) to fill in dummy
values for even unsupported fields.
I understand your argument, since that was my first thought, but I
also understand the reason behind clearing STATX_ATIME. And atime is
something that very few applications care about anyway. But I do
think such behavior should be clarified in the documentation and
handled more consistently in the code.
> To my thinking, if the user/app requests btime/ctime/size/blocks the kernel
> should return these fields. If DONT_SYNC is set, it means the kernel can
> return potentially stale values, but if the kernel doesn't have *any*
> values for these fields at all it still should query the server instead of
> just returning random garbage.
I'm not saying it should return garbage, instead it can just not set
the bit in the result mask to indicate that that field cannot be
filled (without network traffic, in this case).
This requires generalizing the definition of the result mask, because
the above (clearing STATX_ATIME on ro/noatim or clearing STATX_BTIME
on DONT_SYNC) don't fit the current "unsupported or unrepresentable"
definition.
> That said, it seems that it isn't even possible to get into nfs_getattr()
> without the VFS having instantiated a dentry and inode (i.e. queried the
> server via nfs_lookup() at some point), so the inode fields have already
> been filled with *something*.
There's no "i_btime" field.
But it's hard to argue this point with btime, so lets instead think
about a theoretical attribute that is:
- hard to calculate, so filesystem will not want to cache it on inode
initialization (because it's very unlikely to be actually needed)
- isn't constant and can be cached, so all sync types make sense.
I think the following semantics would be the most useful and sane:
FORCE_SYNC or SYNC_AS_STAT would always retrieve the attribute if
supported, and set the bit in the result mask; DONT_SYNC would only
set the bit in the result mask if the attribute is already cached.
Thanks,
Miklos
^ permalink raw reply
* Re: statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-18 7:37 UTC (permalink / raw)
To: Florian Weimer
Cc: Andreas Dilger, Michael Kerrisk, David Howells,
Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API
In-Reply-To: <878t2wb4dr.fsf@mid.deneb.enyo.de>
On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Andreas Dilger:
>
>>> So what's the point exactly?
>>
>> Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
>> to mask off flags that it doesn't currently understand. It doesn't make
>> much sense for applications to specify STATX_ALL, since they don't have any
>> way to know what each flag means unless they are hard-coded to check each of
>> the STATX_* flags individually. They should build up a mask of STATX_* flags
>> based on what they care about (e.g. "find" should only request attributes
>> based on the command-line options given).
>
> Could you remove it from the UAPI header? I didn't want to put it
> into the glibc header, but was overruled.
To summarize Linus' rule of backward incompatibility: you can do it as
long as nobody notices. So yeah, we could try removing STATX_ALL from
the uapi header, but we'd have to put it back in, once somebody
complains.
Thanks,
Miklos
^ permalink raw reply
* Re: statx(2) API and documentation
From: Florian Weimer @ 2018-10-18 7:39 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Andreas Dilger, Michael Kerrisk, David Howells,
Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API
In-Reply-To: <CAJfpegscdd7Q7waB4me7RMC_SJv4=rQwEGXBYNGAr2F3WE5=zA@mail.gmail.com>
* Miklos Szeredi:
> On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> * Andreas Dilger:
>>
>>>> So what's the point exactly?
>>>
>>> Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
>>> to mask off flags that it doesn't currently understand. It doesn't make
>>> much sense for applications to specify STATX_ALL, since they don't have any
>>> way to know what each flag means unless they are hard-coded to check each of
>>> the STATX_* flags individually. They should build up a mask of STATX_* flags
>>> based on what they care about (e.g. "find" should only request attributes
>>> based on the command-line options given).
>>
>> Could you remove it from the UAPI header? I didn't want to put it
>> into the glibc header, but was overruled.
>
> To summarize Linus' rule of backward incompatibility: you can do it as
> long as nobody notices. So yeah, we could try removing STATX_ALL from
> the uapi header, but we'd have to put it back in, once somebody
> complains.
I don't recall a rule about backwards-incompatible API changes. This
wouldn't impact ABI at all.
^ permalink raw reply
* Re: statx(2) API and documentation
From: Jan Kara @ 2018-10-18 7:41 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, Andreas Dilger, Michael Kerrisk (man-pages),
David Howells, linux-fsdevel, linux-kernel, linux-api, Jan Kara
In-Reply-To: <CAOQ4uxjqS3R_0O6gKVFEUJR7BvB+rpaHekjZm+fhMA+7on8AwQ@mail.gmail.com>
On Thu 18-10-18 01:15:13, Amir Goldstein wrote:
> On Wed, Oct 17, 2018 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > >> - STATX_ALL definition is unclear, can this change, or is it fixed?
> > >> If it's the former, than that's a backward compatibility nightmare.
> > >> If it's the latter, then what's the point?
> > >
> > > The value can change over time. It is intended to reflect the current
> > > state of affairs at the time the userspace program and kernel are compiled.
> > > The value sent from userspace lets the kernel know what fields are in
> > > the userspace struct, so it doesn't try to set fields that aren't there.
> >
> > What's the point of a userspace program specifying STATX_ALL? Without
> > a way to programmatically query the interface definition it's useless:
> > there's no way to guess which mask bit corresponds to which field, and
> > what that field represents.
> >
> > And there will be programs out there which specify STATX_ALL without
> > considering that in the future it may become slower as it is now due
> > to a recompile.
> >
> > So what's the point exactly?
> >
> > > The value in the kernel allows masking off new fields from userspace that
> > > it doesn't understand.
> >
> > Okay, but that has nothing to do with the UAPI. Even as an internal
> > flag we should be careful, as it might grow uses which can have
> > similar issues as the userspace one above.
> >
>
> FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
> add new flags and did not want to change the UAPI _ALL_ constants.
> This is how we plan to solve it:
> https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc
Yeah, after fanotify experience I find foo_ALL constants useless if not
dangerous for userspace. Kernel internal constants like this are IMO
useful.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-18 7:42 UTC (permalink / raw)
To: Florian Weimer
Cc: Andreas Dilger, Michael Kerrisk, David Howells,
Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API
In-Reply-To: <87sh13ya8m.fsf@mid.deneb.enyo.de>
On Thu, Oct 18, 2018 at 9:39 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Miklos Szeredi:
>
>> On Thu, Oct 18, 2018 at 12:22 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>> * Andreas Dilger:
>>>
>>>>> So what's the point exactly?
>>>>
>>>> Ah, I see your point... STATX_ALL seems to be mostly useful for the kernel
>>>> to mask off flags that it doesn't currently understand. It doesn't make
>>>> much sense for applications to specify STATX_ALL, since they don't have any
>>>> way to know what each flag means unless they are hard-coded to check each of
>>>> the STATX_* flags individually. They should build up a mask of STATX_* flags
>>>> based on what they care about (e.g. "find" should only request attributes
>>>> based on the command-line options given).
>>>
>>> Could you remove it from the UAPI header? I didn't want to put it
>>> into the glibc header, but was overruled.
>>
>> To summarize Linus' rule of backward incompatibility: you can do it as
>> long as nobody notices. So yeah, we could try removing STATX_ALL from
>> the uapi header, but we'd have to put it back in, once somebody
>> complains.
>
> I don't recall a rule about backwards-incompatible API changes. This
> wouldn't impact ABI at all.
Right, API rules maybe are softer. I'll do some patches...
Thanks,
Miklos
^ permalink raw reply
* Re: statx(2) API and documentation
From: Florian Weimer @ 2018-10-18 7:49 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, Miklos Szeredi, Andreas Dilger,
Michael Kerrisk (man-pages), David Howells, linux-fsdevel,
linux-kernel, linux-api
In-Reply-To: <20181018074101.GJ23493@quack2.suse.cz>
* Jan Kara:
> On Thu 18-10-18 01:15:13, Amir Goldstein wrote:
>> FYI, I identified a similar anti-pattern in fanotify UAPI when I wanted to
>> add new flags and did not want to change the UAPI _ALL_ constants.
>> This is how we plan to solve it:
>> https://github.com/amir73il/linux/commit/8c2b1acadb88ee4505ccc8bfdc665863111fb4cc
>
> Yeah, after fanotify experience I find foo_ALL constants useless if not
> dangerous for userspace. Kernel internal constants like this are IMO
> useful.
There are also various *_MAX constants which are increased regularly.
Some of them are part of the UAPI headers (INET_DIAG_MAX appears to be
a relevant example), some are no longer in UAPI, but still in custom
glibc headers (AF_MAX/PF_MAX). These appear to be equally useless.
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Pavel Machek @ 2018-10-18 9:24 UTC (permalink / raw)
To: Borislav Petkov
Cc: Randy Dunlap, Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz
In-Reply-To: <20181017225829.GA32023@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 877 bytes --]
On Thu 2018-10-18 00:58:29, Borislav Petkov wrote:
> On Wed, Oct 17, 2018 at 03:39:47PM -0700, Randy Dunlap wrote:
> > Would you mind explaining this request? (requirement?)
> > Other than to say that it is the preference of some maintainers,
> > please say Why it is preferred.
> >
> > and since the <type>s above won't typically be the same length,
> > it's not for variable name alignment, right?
>
> Searching the net a little, it shows you have asked that question
> before. So what is it you really wanna know?
Why do you think sorting local variables is good idea (for includes it
reduces collision, hopefully you don't have that for local variables),
and where is it documented in CodingStyle.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Borislav Petkov @ 2018-10-18 9:26 UTC (permalink / raw)
To: Randy Dunlap
Cc: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <fde4c237-6210-f4e4-5362-c2e24a9916a2@infradead.org>
On Wed, Oct 17, 2018 at 04:17:01PM -0700, Randy Dunlap wrote:
> I asked what I really wanted to know.
Then the answer is a bit better readability, I'd guess.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Pavel Machek @ 2018-10-18 9:31 UTC (permalink / raw)
To: Borislav Petkov
Cc: Randy Dunlap, Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz
In-Reply-To: <20181018092603.GB20831@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 643 bytes --]
On Thu 2018-10-18 11:26:03, Borislav Petkov wrote:
> On Wed, Oct 17, 2018 at 04:17:01PM -0700, Randy Dunlap wrote:
> > I asked what I really wanted to know.
>
> Then the answer is a bit better readability, I'd guess.
Normally, similar local variables are grouped together, with
initialized variables giving additional constraints.
Additionally sorting them with length ... gives too much constraints
to the author. We want readable sources, not neat ascii art everywhere.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v9 00/24] ILP32 for ARM64
From: Catalin Marinas @ 2018-10-18 11:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andy Lutomirski, open list:DOCUMENTATION, Szabolcs Nagy,
Heiko Carstens, Yury Norov, Philipp Tomsich, Joseph Myers,
linux-arch, Steve Ellcey, Prasun Kapoor, Andreas Schwab,
Alexander Graf, Geert Uytterhoeven, Bamvor Zhangjian, Dave Martin,
Adam Borowski, Manuel A. Fernandez Montecelo, James Hogan
In-Reply-To: <CAK8P3a1Yhhv1TcQTcF0MinicGovgp_0g_sXqADe3fjVTtOK_aw@mail.gmail.com>
On Sun, Oct 14, 2018 at 09:49:01PM +0200, Arnd Bergmann wrote:
> On Sat, Oct 13, 2018 at 9:36 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > On Wed, May 16, 2018 at 1:19 AM Yury Norov <ynorov@caviumnetworks.com> wrote:
> > >
> > > This series enables AARCH64 with ILP32 mode.
> > >
> > > As supporting work, it introduces ARCH_32BIT_OFF_T configuration
> > > option that is enabled for existing 32-bit architectures but disabled
> > > for new arches (so 64-bit off_t userspace type is used by new userspace).
> > > Also it deprecates getrlimit and setrlimit syscalls prior to prlimit64.
> >
> > Second, ILP32 user code is highly unlikely
> > to end up with the same struct layout as ILP64 code. The latter seems
> > like it should be solved entirely in userspace by adding a way to
> > annotate a structure as being a kernel ABI structure and getting the
> > toolchain to lay it out as if it were ILP64 even though the target is
> > ILP32.
>
> The syscall ABI could be almost completely abstracted in glibc, the
> main issue is ioctl and a couple of related interfaces that pass data
> structures (read() on /dev/input/*, mmap on /dev/snd/*
> or raw sockets, fcntl).
There is another case on struct siginfo which has some pointers and it
wouldn't look like an LP64 structure at all (and glibc doesn't normally
intercept the sighandler call to rewrite the structure). We could add
padding around void * members as the kernel zeros them, I don't recall
the kernel reading these pointers from user. Anyway, using something
that resembles compat_siginfo looked the simplest for ILP32.
--
Catalin
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Borislav Petkov @ 2018-10-18 12:10 UTC (permalink / raw)
To: Pavel Machek
Cc: Randy Dunlap, Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, linux-kernel, linux-doc, linux-mm, linux-arch,
linux-api, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz
In-Reply-To: <20181018093125.GB10861@amd>
On Thu, Oct 18, 2018 at 11:31:25AM +0200, Pavel Machek wrote:
> We want readable sources, not neat ascii art everywhere.
And we want pink ponies.
Reverse xmas tree order is and has been the usual variable sorting in
the tip tree for years.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* [PATCH 1/3] orangefs: fix request_mask misuse
From: Miklos Szeredi @ 2018-10-18 13:11 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein, Mike Marshall,
Martin Brandenburg
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
^ permalink raw reply related
* [PATCH 2/3] uapi: get rid of STATX_ALL
From: Miklos Szeredi @ 2018-10-18 13:11 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181018131125.6303-1-mszeredi@redhat.com>
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
^ permalink raw reply related
* [PATCH 3/3] statx: add STATX_ATTRIBUTES flag
From: Miklos Szeredi @ 2018-10-18 13:11 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181018131125.6303-1-mszeredi@redhat.com>
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
^ permalink raw reply related
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Florian Weimer @ 2018-10-18 13:15 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, linux-kernel, linux-api, David Howells,
Michael Kerrisk, Andreas Dilger, Amir Goldstein
In-Reply-To: <20181018131125.6303-2-mszeredi@redhat.com>
* 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?
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Miklos Szeredi @ 2018-10-18 14:32 UTC (permalink / raw)
To: Florian Weimer
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
David Howells, Michael Kerrisk, Andreas Dilger, Amir Goldstein
In-Reply-To: <875zxzxuo9.fsf@mid.deneb.enyo.de>
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
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Miklos Szeredi @ 2018-10-18 14:34 UTC (permalink / raw)
To: Florian Weimer
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
David Howells, Michael Kerrisk, Andreas Dilger, Amir Goldstein
In-Reply-To: <CAJfpeguM-TU=nxj=T4t1DshniiEFBGq6yo+qo7Cx1QDnoNocMw@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Amir Goldstein @ 2018-10-18 14:51 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, linux-kernel, linux-api, David Howells,
Michael Kerrisk (man-pages), Andreas Dilger, fw
In-Reply-To: <20181018131125.6303-2-mszeredi@redhat.com>
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.
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-18 14:55 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Jan Kara,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181018002510.GC6311@dastard>
On Thu 18-10-18 11:25:10, Dave Chinner wrote:
> On Wed, Oct 17, 2018 at 04:23:50PM -0400, Jeff Moyer wrote:
> > MAP_SYNC
> > - file system guarantees that metadata required to reach faulted-in file
> > data is consistent on media before a write fault is completed. A
> > side-effect is that the page cache will not be used for
> > writably-mapped pages.
>
> I think you are conflating current implementation with API
> requirements - MAP_SYNC doesn't guarantee anything about page cache
> use. The man page definition simply says "supported only for files
> supporting DAX" and that it provides certain data integrity
> guarantees. It does not define the implementation.
>
> We've /implemented MAP_SYNC/ as O_DSYNC page fault behaviour,
> because that's the only way we can currently provide the required
> behaviour to userspace. However, if a filesystem can use the page
> cache to provide the required functionality, then it's free to do
> so.
>
> i.e. if someone implements a pmem-based page cache, MAP_SYNC data
> integrity could be provided /without DAX/ by any filesystem using
> that persistent page cache. i.e. MAP_SYNC really only requires
> mmap() of CPU addressable persistent memory - it does not require
> DAX. Right now, however, the only way to get this functionality is
> through a DAX capable filesystem on dax capable storage.
>
> And, FWIW, this is pretty much how NOVA maintains DAX w/ COW - it
> COWs new pages in pmem and attaches them a special per-inode cache
> on clean->dirty transition. Then on data sync, background writeback
> or crash recovery, it migrates them from the cache into the file map
> proper via atomic metadata pointer swaps.
>
> IOWs, NOVA provides the correct MAP_SYNC semantics by using a
> separate persistent per-inode write cache to provide the correct
> crash recovery semantics for MAP_SYNC.
Corect. NOVA would be able to provide MAP_SYNC semantics without DAX. But
effectively it will be also able to provide MAP_DIRECT semantics, right?
Because there won't be DRAM between app and persistent storage and I don't
think COW tricks or other data integrity methods are that interesting for
the application. Most users of O_DIRECT are concerned about getting close
to media speed performance and low DRAM usage...
> > and what I think Dan had proposed:
> >
> > mmap flag, MAP_DIRECT
> > - file system guarantees that page cache will not be used to front storage.
> > storage MUST be directly addressable. This *almost* implies MAP_SYNC.
> > The subtle difference is that a write fault /may/ not result in metadata
> > being written back to media.
>
> SIimilar to O_DIRECT, these semantics do not allow userspace apps to
> replace msync/fsync with CPU cache flush operations. So any
> application that uses this mode still needs to use either MAP_SYNC
> or issue msync/fsync for data integrity.
>
> If the app is using MAP_DIRECT, the what do we do if the filesystem
> can't provide the required semantics for that specific operation? In
> the case of O_DIRECT, we fall back to buffered IO because it has the
> same data integrity semantics as O_DIRECT and will always work. It's
> just slower and consumes more memory, but the app continues to work
> just fine.
>
> Sending SIGBUS to apps when we can't perform MAP_DIRECT operations
> without using the pagecache seems extremely problematic to me. e.g.
> an app already has an open MAP_DIRECT file, and a third party
> reflinks it or dedupes it and the fs has to fall back to buffered IO
> to do COW operations. This isn't the app's fault - the kernel should
> just fall back transparently to using the page cache for the
> MAP_DIRECT app and just keep working, just like it would if it was
> using O_DIRECT read/write.
There's another option of failing reflink / dedupe with EBUSY if the file
is mapped with MAP_DIRECT and the filesystem cannot support relink &
MAP_DIRECT together. But there are downsides to that as well.
> The point I'm trying to make here is that O_DIRECT is a /hint/, not
> a guarantee, and it's done that way to prevent applications from
> being presented with transient, potentially fatal error cases
> because a filesystem implementation can't do a specific operation
> through the direct IO path.
>
> IMO, MAP_DIRECT needs to be a hint like O_DIRECT and not a
> guarantee. Over time we'll end up with filesystems that can
> guarantee that MAP_DIRECT is always going to use DAX, in the same
> way we have filesystems that guarantee O_DIRECT will always be
> O_DIRECT (e.g. XFS). But if we decide that MAP_DIRECT must guarantee
> no page cache will ever be used, then we are basically saying
> "filesystems won't provide MAP_DIRECT even in common, useful cases
> because they can't provide MAP_DIRECT in all cases." And that
> doesn't seem like a very good solution to me.
These are good points. I'm just somewhat wary of the situation where users
will map files with MAP_DIRECT and then the machine starts thrashing
because the file got reflinked and thus pagecache gets used suddently.
With O_DIRECT the fallback to buffered IO is quite rare (at least for major
filesystems) so usually people just won't notice. If fallback for
MAP_DIRECT will be easy to hit, I'm not sure it would be very useful.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: David Howells @ 2018-10-18 15:16 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, linux-fsdevel, linux-kernel, linux-api, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181018131125.6303-2-mszeredi@redhat.com>
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
^ permalink raw reply
* Re: [PATCH 3/3] statx: add STATX_ATTRIBUTES flag
From: David Howells @ 2018-10-18 15:22 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, linux-fsdevel, linux-kernel, linux-api, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181018131125.6303-3-mszeredi@redhat.com>
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
^ permalink raw reply
* Re: statx(2) API and documentation
From: David Howells @ 2018-10-18 16:04 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, Michael Kerrisk, linux-fsdevel, linux-kernel, Linux API
In-Reply-To: <CAJfpegsxFUGeczcC-pzK0WWROjv48_PvXtNun-f5QJMoUDn1Dw@mail.gmail.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> I'm trying to implement statx for fuse and ran into the following issues:
>
> - Need a STATX_ATTRIBUTES bit, so that userspace can explicitly ask
> for stx_attribute; otherwise if querying has non-zero cost, then
> filesystem cannot do it without regressing performance.
Okay, though the way your patch implements it makes it superfluous. I presume
you have further patches that will actually make use of it from the fuse side?
> - STATX_ALL definition is unclear, can this change, or is it fixed?
> If it's the former, than that's a backward compatibility nightmare.
> If it's the latter, then what's the point?
It's the set of supported attributes known by the headers, and such can
only be added to over time. But yes, it's probably unnecessary. Asking
fsinfo() will be a better way of doing things.
> - STATX_ATIME is cleared from stx_mask on SB_RDONLY,
Ummm... Where? It's cleared on IS_NOATIME() in generic_fillattr(). I made
the assumption that IS_NOATIME() == false indicates that there isn't an atime
to be had.
> and on NFS it is also cleared on MNT_NOATIME, but not on MNT_RDONLY. We
> need some sort of guideline in the documentation about what constitutes
> "unsupported": does atime become unsupported because filesystem is remounted
> r/o? If so, why isn't this case handled consistently in the VFS and
> filesystems?
STATX_ATIME should mean there is an actual atime from the "medium" in
stx_atime, rather than something made up by the filesystem driver; it doesn't
necessarily promise that this will be updated.
There can still be an atime if the medium is read-only.
atime is even more complicated with MNT_NOATIME or MNT_RDONLY because that
doesn't stop the atime from actually being updated through another mountpoint
on the same system.
Note that stx_atime should always contain something that can be used directly
to fill in st_atime if emulating stat() - even if STATX_ATIME is cleared.
> - What about fields that are not cached when statx() is called with
> AT_STATX_DONT_SYNC? E.g. stx_btime is supported by the filesystem,
> but getting it requires a roundtrip to the server.
Not necessarily. It's not cached in *struct inode*, but that doesn't mean
that the filesystem can't cache it elsewhere.
> Requesting STATX_BTIME in the mask and adding AT_STATX_DONT_SYNC to the
> flags means the filesystem has to decide which it will honor. My feeling is
> that it should honor AT_STATX_DONT_SYNC and clear STATX_BTIME in stx_mask.
> Documentation has no word about this case.
>From the manpage:
AT_STATX_DONT_SYNC
Don't synchronize anything, but rather just take whatever the
system has cached if possible. ...
Note the "if possible". If it's not possible, you still need to go get it if
they explicitly asked for it.
David
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Florian Weimer @ 2018-10-18 16:11 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, linux-api,
David Howells, Michael Kerrisk (man-pages), Andreas Dilger
In-Reply-To: <CAOQ4uxiFrtphwYhDMpsiENV8zkHHKmthb7cB2T0PKU5BA_YTWw@mail.gmail.com>
* 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.
^ permalink raw reply
* Re: [PATCH v5 03/27] x86/fpu/xstate: Introduce XSAVES system states
From: Randy Dunlap @ 2018-10-18 18:33 UTC (permalink / raw)
To: Borislav Petkov
Cc: Yu-cheng Yu, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <20181018092603.GB20831@zn.tnic>
On 10/18/18 2:26 AM, Borislav Petkov wrote:
> On Wed, Oct 17, 2018 at 04:17:01PM -0700, Randy Dunlap wrote:
>> I asked what I really wanted to know.
>
> Then the answer is a bit better readability, I'd guess.
>
Thanks for the reply.
--
~Randy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox