* Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: David Howells @ 2018-10-19 15:59 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: <20181019143932.6697-1-mszeredi@redhat.com>
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
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Trond Myklebust @ 2018-10-19 15:52 UTC (permalink / raw)
To: mszeredi@redhat.com, linux-fsdevel@vger.kernel.org
Cc: amir73il@gmail.com, linux-kernel@vger.kernel.org,
adilger@dilger.ca, mtk.manpages@gmail.com, fw@deneb.enyo.de,
linux-api@vger.kernel.org, dhowells@redhat.com
In-Reply-To: <20181019122049.27121-5-mszeredi@redhat.com>
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
^ permalink raw reply
* Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL
From: David Howells @ 2018-10-19 15:47 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <CAJfpegviiy=9fru3zButbqLnhshH85srH3qyZO2zf-_RPqSs=g@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: David Howells @ 2018-10-19 15:44 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, linux-fsdevel, linux-kernel, linux-api, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein, Trond Myklebust
In-Reply-To: <20181019122049.27121-5-mszeredi@redhat.com>
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>
^ permalink raw reply
* Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL
From: Miklos Szeredi @ 2018-10-19 15:40 UTC (permalink / raw)
To: David Howells
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <16382.1539963322@warthog.procyon.org.uk>
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
^ permalink raw reply
* Re: [PATCH v2 4/5] statx: don't clear STATX_ATIME on SB_RDONLY
From: David Howells @ 2018-10-19 15:40 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: <20181019122049.27121-4-mszeredi@redhat.com>
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>
^ permalink raw reply
* Re: [PATCH v2 3/5] statx: add STATX_ATTRIBUTES flag
From: David Howells @ 2018-10-19 15:36 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: <20181019122049.27121-3-mszeredi@redhat.com>
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>
^ permalink raw reply
* Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL
From: David Howells @ 2018-10-19 15:35 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: <20181019122049.27121-2-mszeredi@redhat.com>
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
^ permalink raw reply
* [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: Miklos Szeredi @ 2018-10-19 14:39 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
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
^ permalink raw reply related
* Re: [PATCH v2 1/5] orangefs: fix request_mask misuse
From: Andreas Dilger @ 2018-10-19 14:27 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux FS-devel Mailing List, Linux Kernel Mailing List, linux-api,
David Howells, Michael Kerrisk, Florian Weimer, Amir Goldstein,
Mike Marshall
In-Reply-To: <20181019122049.27121-1-mszeredi@redhat.com>
[-- 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 --]
^ permalink raw reply
* [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Miklos Szeredi @ 2018-10-19 12:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein, Trond Myklebust
In-Reply-To: <20181019122049.27121-1-mszeredi@redhat.com>
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
^ permalink raw reply related
* [PATCH v2 4/5] statx: don't clear STATX_ATIME on SB_RDONLY
From: Miklos Szeredi @ 2018-10-19 12:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181019122049.27121-1-mszeredi@redhat.com>
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
^ permalink raw reply related
* [PATCH v2 3/5] statx: add STATX_ATTRIBUTES flag
From: Miklos Szeredi @ 2018-10-19 12:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181019122049.27121-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 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
^ permalink raw reply related
* [PATCH v2 2/5] uapi: deprecate STATX_ALL
From: Miklos Szeredi @ 2018-10-19 12:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181019122049.27121-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.
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
^ permalink raw reply related
* [PATCH v2 1/5] orangefs: fix request_mask misuse
From: Miklos Szeredi @ 2018-10-19 12:20 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-kernel, linux-api, David Howells, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein, Mike Marshall
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
^ permalink raw reply related
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Andreas Dilger @ 2018-10-19 2:25 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux FS-devel Mailing List, Linux Kernel Mailing List, Linux API,
David Howells, Michael Kerrisk, Florian Weimer, Amir Goldstein,
Mark Fasheh, Joel Becker, Steve French
In-Reply-To: <20181018131125.6303-2-mszeredi@redhat.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH 3/3] statx: add STATX_ATTRIBUTES flag
From: Andreas Dilger @ 2018-10-19 1:48 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, linux-kernel, linux-api, David Howells,
Michael Kerrisk, Florian Weimer, Amir Goldstein
In-Reply-To: <20181018131125.6303-3-mszeredi@redhat.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH 2/3] uapi: get rid of STATX_ALL
From: Andreas Dilger @ 2018-10-19 1:45 UTC (permalink / raw)
To: Florian Weimer
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, linux-api,
David Howells, Michael Kerrisk, Amir Goldstein
In-Reply-To: <875zxzxuo9.fsf@mid.deneb.enyo.de>
[-- 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 --]
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dave Chinner @ 2018-10-19 0:43 UTC (permalink / raw)
To: Jan Kara
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-ext4-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20181018145555.GS23493-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Thu, Oct 18, 2018 at 04:55:55PM +0200, Jan Kara wrote:
> 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?
Yes, I think so. It still needs to do COW on first write fault,
but then the app has direct access to the data buffer until it is
cleaned and put back in place. The "put back in place" is just an
atomic swap of metadata pointers, so it doesn't need the page cache
at all...
> 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.
Not for the application, but the filesystem still wants to support
snapshots and other such functionality that requires COW. And NOVA
doesn't have write-in-place functionality at all - it always COWs
on the clean->dirty transition.
> Most users of O_DIRECT are concerned about getting close
> to media speed performance and low DRAM usage...
*nod*
> > > 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.
Yup, not the least that setting MAP_DIRECT can race with a
reflink....
> > 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.
It's still better than apps randomly getting SIGBUS.
FWIW, this suggests that we probably need to be able to host both
DAX pages and page cache pages on the same file at the same time,
and be able to handle page faults based on the type of page being
mapped (different sets of fault ops for different page types?)
and have fallback paths when the page type needs to be changed
between direct and cached during the fault....
> 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.
Which is just like the situation where O_DIRECT on ext3 was not very
useful, but on other filesystems like XFS it was fully functional.
IMO, the fact that a specific filesytem has a suboptimal fallback
path for an uncommon behaviour isn't an argument against MAP_DIRECT
as a hint - it's actually a feature. If MAP_DIRECT can't be used
until it's always direct access, then most filesystems wouldn't be
able to provide any faster paths at all. It's much better to have
partial functionality now than it is to never have the functionality
at all, and so we need to design in the flexibility we need to
iteratively improve implementations without needing API changes that
will break applications.
Cheers,
Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [PATCH 1/3] printk: Introduce per-console loglevel setting
From: Sergey Senozhatsky @ 2018-10-19 0:04 UTC (permalink / raw)
To: Calvin Owens
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-api,
linux-doc, linux-kernel, kernel-team
In-Reply-To: <08c1dc1a96afd6b6aecc5ff3c7c0e62c36670893.1506644730.git.calvinowens@fb.com>
On (09/28/17 17:43), Calvin Owens wrote:
> Not all consoles are created equal: depending on the actual hardware,
> the latency of a printk() call can vary dramatically. The worst examples
> are serial consoles, where it can spin for tens of milliseconds banging
> the UART to emit a message, which can cause application-level problems
> when the kernel spews onto the console.
>
> At Facebook we use netconsole to monitor our fleet, but we still have
> serial consoles attached on each host for live debugging, and the latter
> has caused problems. An obvious solution is to disable the kernel
> console output to ttyS0, but this makes live debugging frustrating,
> since crashes become silent and opaque to the ttyS0 user. Enabling it on
> the fly when needed isn't feasible, since boxes you need to debug via
> serial are likely to be borked in ways that make this impossible.
>
> That puts us between a rock and a hard place: we'd love to set
> kernel.printk to KERN_INFO and get all the logs. But while netconsole is
> fast enough to permit that without perturbing userspace, ttyS0 is not,
> and we're forced to limit console logging to KERN_WARNING and higher.
>
> This patch introduces a new per-console loglevel setting, and changes
> console_unlock() to use max(global_level, per_console_level) when
> deciding whether or not to emit a given log message.
>
> This lets us have our cake and eat it too: instead of being forced to
> limit all consoles verbosity based on the speed of the slowest one, we
> can "promote" the faster console while still using a conservative system
> loglevel setting to avoid disturbing applications.
Hi Calvin,
Do you have time to address the review feedback and re-spin v2?
-ss
^ permalink raw reply
* Re: [PATCH v3 2/2] sysctl: handle overflow for file-max
From: Andrea Arcangeli @ 2018-10-18 21:58 UTC (permalink / raw)
To: Al Viro
Cc: Christian Brauner, keescook, linux-kernel, ebiederm, mcgrof, akpm,
joe.lawrence, longman, linux, adobriyan, linux-api,
Miklos Szeredi, Eric Dumazet
In-Reply-To: <20181017003548.GA32577@ZenIV.linux.org.uk>
Hi Al,
On Wed, Oct 17, 2018 at 01:35:48AM +0100, Al Viro wrote:
> On Wed, Oct 17, 2018 at 12:33:22AM +0200, Christian Brauner wrote:
> > Currently, when writing
> >
> > echo 18446744073709551616 > /proc/sys/fs/file-max
> >
> > /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> > crashes the system.
> > This commit sets the max and min value for file-max and returns -EINVAL
> > when a long int is exceeded. Any higher value cannot currently be used as
> > the percpu counters are long ints and not unsigned integers. This behavior
> > also aligns with other tuneables that return -EINVAL when their range is
> > exceeded. See e.g. [1], [2] and others.
>
> Mostly sane, but... get_max_files() users are bloody odd. The one in
> file-max limit reporting looks like a half-arsed attempt in "[PATCH] fix
> file counting". The one in af_unix.c, though... I don't remember how
> that check had come to be - IIRC that was a strange fallout of a thread
> with me, Andrea and ANK involved, circa 1999, but I don't remember details;
> Andrea, any memories? It might be worth reconsidering... The change in
> question is in 2.2.4pre6; what do we use unix_nr_socks for? We try to
> limit the number of PF_UNIX socks by 2 * max_files, but max_files can be
> huge *and* non-constant (i.e. it can decrease). What's more, unix_tot_inflight
> is unsigned int and max_files might exceed 2^31 just fine since "fs: allow
> for more than 2^31 files" back in 2010... Something's fishy there...
Feels like a lifetime ago :), but looking into I remembered some of
it. That thread was about some instability in unix sockets for an
unrelated bug in the garbage collector. While reviewing your fix, I
probably incidentally found a resource exhaustion problem in doing a
connect();close() loop on a listening stream af_unix. I found an
exploit somewhere in my home dated in 99 in ls -l. Then ANK found
another resource exhaustion by sending datagram sockets, which I also
found an exploit for in my home.
ANK pointed out that a connect syscall allocates two sockets, one to
be accepted by the listening process, the other is the connect
itself. That must be the explanation of the "*2".
The "max_files*2" is probably a patch was from you (which was not
overflowing back then), in attempt to fix the garbage collector issue
which initially looked like resource exhaustion.
I may have suggested to check sk_max_ack_backlog and fail connect() in
such case to solve the resource exhaustion, but my proposal was
obviously broken because connect() would return an error when the
backlog was full and I suppose I didn't implement anything like
unix_wait_for_peer. So I guess (not 100% sure) the get_max_files()*2
check stayed, not because of the bug in the garbage collector that was
fixed independently, but as a stop gap measure for the
connect();close() loop resource exhaustion.
I tried the exploit that does a connect();close() in a loop and it
gracefully hangs in unix_wait_for_peer() after sk_max_ack_backlog
connects.
Out of curiosity I tried also the dgram exploit and it hangs in
sock_alloc_send_pskb with sk_wmem_alloc_get(sk) < sk->sk_sndbuf
check. The file*2 limit couldn't have helped that one anyway.
If I set /proc/sys/net/core/somaxconn to 1000000 the exploit works
fine again and the connect;close loop again allocates infinite amount
of kernel RAM into a tiny RSS process and it triggered OOM (there was
no OOM killer in v2.2 I suppose). By default it's 128. There's also
sysctl_max_dgram_qlen for dgram that on Android is set to 600 (by
default 10).
I tend to think these resources are now capped by other means (notably
somaxconn, sysctl_max_dgram_qlen, sk_wmem_alloc_get) and unix_nr_socks
can be dropped. Or if that atomic counter is still needed it's not for
a trivial exploit anymore than just does listen(); SIGSTOP() from one
process and a connect();close() loop in another process. It'd require
more than a listening socket and heavily forking or a large increase
on the max number of file descriptors (a privileged op) to do a ton of
listens, but forking has its own memory footprint in userland too. At
the very least it should be a per-cpu counter synced to the atomic
global after a threshold.
The other reason for dropping is that it wasn't ideal that the trivial
exploit could still allocated max_files*2 SYN skbs with a loop of
connect;close, max_files*2 is too much already so I suppose it was
only a stop-gap measure to begin with.
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH 1/3] orangefs: fix request_mask misuse
From: martin @ 2018-10-18 21:45 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, linux-kernel, linux-api, David Howells,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein,
Mike Marshall
In-Reply-To: <20181018131125.6303-1-mszeredi@redhat.com>
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
>
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jeff Moyer @ 2018-10-18 21:05 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>
Dave,
Thanks for the detailed response! I hadn't considered the NOVA use case
at all.
Cheers,
Jeff
^ permalink raw reply
* Re: statx(2) API and documentation
From: Miklos Szeredi @ 2018-10-18 20:21 UTC (permalink / raw)
To: David Howells; +Cc: Michael Kerrisk, linux-fsdevel, linux-kernel, Linux API
In-Reply-To: <4893.1539878663@warthog.procyon.org.uk>
On Thu, Oct 18, 2018 at 6:04 PM, David Howells <dhowells@redhat.com> wrote:
> 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?
Being worked on, yes.
>
>> - 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.
Look at IS_NOATIME definition in <linux/fs.h>
You probably wanted inode->i_sb->s_flags & SB_NOATIME instead.
>> 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.
In this case generic_fillattr() and nfs_getattr() are simply buggy.
>
> 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.
Okay.
Thanks,
Miklos
^ 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