* Re: [PATCH v4 3/3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-04-22 14:59 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
Arnd Bergmann, Pali Rohár, Paul Moore, James Morris,
Serge E. Hallyn, linux-alpha, linux-kernel, linux-arm-kernel,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, linux-security-module,
linux-api, linux-arch, linux-xfs
In-Reply-To: <20250321-xattrat-syscall-v4-3-3e82e6fb3264@kernel.org>
On Fri, Mar 21, 2025 at 08:48:42PM +0100, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
> extended attributes/flags. The syscalls take parent directory fd and
> path to the child together with struct fsxattr.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
>
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
>
> CC: linux-api@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> arch/alpha/kernel/syscalls/syscall.tbl | 2 +
> arch/arm/tools/syscall.tbl | 2 +
> arch/arm64/tools/syscall_32.tbl | 2 +
> arch/m68k/kernel/syscalls/syscall.tbl | 2 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
> arch/parisc/kernel/syscalls/syscall.tbl | 2 +
> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
> arch/s390/kernel/syscalls/syscall.tbl | 2 +
> arch/sh/kernel/syscalls/syscall.tbl | 2 +
> arch/sparc/kernel/syscalls/syscall.tbl | 2 +
> arch/x86/entry/syscalls/syscall_32.tbl | 2 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
> fs/inode.c | 130 ++++++++++++++++++++++++++++
> include/linux/syscalls.h | 6 ++
> include/uapi/asm-generic/unistd.h | 8 +-
> include/uapi/linux/fs.h | 3 +
> 20 files changed, 178 insertions(+), 1 deletion(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -506,3 +506,5 @@
> 574 common getxattrat sys_getxattrat
> 575 common listxattrat sys_listxattrat
> 576 common removexattrat sys_removexattrat
> +577 common getfsxattrat sys_getfsxattrat
> +578 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -481,3 +481,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
> index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
> --- a/arch/arm64/tools/syscall_32.tbl
> +++ b/arch/arm64/tools/syscall_32.tbl
> @@ -478,3 +478,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -466,3 +466,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -472,3 +472,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -405,3 +405,5 @@
> 464 n32 getxattrat sys_getxattrat
> 465 n32 listxattrat sys_listxattrat
> 466 n32 removexattrat sys_removexattrat
> +467 n32 getfsxattrat sys_getfsxattrat
> +468 n32 setfsxattrat sys_setfsxattrat
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -381,3 +381,5 @@
> 464 n64 getxattrat sys_getxattrat
> 465 n64 listxattrat sys_listxattrat
> 466 n64 removexattrat sys_removexattrat
> +467 n64 getfsxattrat sys_getfsxattrat
> +468 n64 setfsxattrat sys_setfsxattrat
> diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
> --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> @@ -454,3 +454,5 @@
> 464 o32 getxattrat sys_getxattrat
> 465 o32 listxattrat sys_listxattrat
> 466 o32 removexattrat sys_removexattrat
> +467 o32 getfsxattrat sys_getfsxattrat
> +468 o32 setfsxattrat sys_setfsxattrat
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -465,3 +465,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -557,3 +557,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -469,3 +469,5 @@
> 464 common getxattrat sys_getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat sys_setfsxattrat
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -470,3 +470,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -512,3 +512,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -472,3 +472,5 @@
> 464 i386 getxattrat sys_getxattrat
> 465 i386 listxattrat sys_listxattrat
> 466 i386 removexattrat sys_removexattrat
> +467 i386 getfsxattrat sys_getfsxattrat
> +468 i386 setfsxattrat sys_setfsxattrat
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -390,6 +390,8 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
>
> #
> # Due to a historical design error, certain syscalls are numbered differently
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -437,3 +437,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/fs/inode.c b/fs/inode.c
> index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..811debf379ab299f287ed90863277cfda27db30c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
I really dislike the name fsxattr for a lot of reasons but it definitely
shouldn't go in inode.c. Just add a new fs/file_attr.c file and move all
the relevant helpers from fs/ioctl.c in there and then add the system
calls. Otherwise it's just all very confusing.
> @@ -23,6 +23,9 @@
> #include <linux/rw_hint.h>
> #include <linux/seq_file.h>
> #include <linux/debugfs.h>
> +#include <linux/syscalls.h>
> +#include <linux/fileattr.h>
> +#include <linux/namei.h>
> #include <trace/events/writeback.h>
> #define CREATE_TRACE_POINTS
> #include <trace/events/timestamp.h>
> @@ -2953,3 +2956,130 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> return mode & ~S_ISGID;
> }
> EXPORT_SYMBOL(mode_strip_sgid);
> +
> +SYSCALL_DEFINE5(getfsxattrat, int, dfd, const char __user *, filename,
This is really misnamed. It will end up confusing userspace to no end:
getxattr()
getxattrat()
getfsxattrat()
Please name this file_setattr() and file_getattr(). There's also no need
for the *at() prefix. We've never been consistent with that. We have
plent of system calls that to fd+path without having an *at() prefix.
And here it's especially unneeded because there's no pre-existing system
calls that would even force the use of that prefix.
> + struct fsxattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa = {};
> + struct path filepath;
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name;
> + struct fsxattr fsx = {};
> +
> + BUILD_BUG_ON(sizeof(struct fsxattr) < FSXATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsxattr) != FSXATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSXATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (!name) {
This is broken as it doesn't handle AT_FDCWD correctly. You need:
name = getname_maybe_null(filename, at_flags);
if (IS_ERR(name))
return PTR_ERR(name);
if (!name && dfd >= 0) {
CLASS(fd, f)(dfd);
> + CLASS(fd, f)(dfd);
> +
> + if (fd_empty(f))
> + return -EBADF;
I'm pretty sure you can just do a:
path = fd_file(f_to)->f_path;
path_get(&path);
and then the vfs_fileattr_get() call and the path_put() call can be
shared between the two branches. Note that you can also use:
struct path path __free(path_put) = NULL;
and then the cleanup infrastructure will handle the path_put() for you.
> + error = vfs_fileattr_get(file_dentry(fd_file(f)), &fa);
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + goto out;
> + error = vfs_fileattr_get(filepath.dentry, &fa);
> + path_put(&filepath);
> + }
> + if (error == -ENOIOCTLCMD)
> + error = -EOPNOTSUPP;
> + if (!error) {
> + fileattr_to_fsxattr(&fa, &fsx);
> + error = copy_struct_to_user(ufsx, usize, &fsx,
> + sizeof(struct fsxattr), NULL);
> + }
> +out:
> + putname(name);
> + return error;
> +}
> +
> +SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename,
> + struct fsxattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa;
> + struct path filepath;
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name;
> + struct mnt_idmap *idmap;
> + struct dentry *dentry;
> + struct vfsmount *mnt;
> + struct fsxattr fsx = {};
> +
> + BUILD_BUG_ON(sizeof(struct fsxattr) < FSXATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsxattr) != FSXATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSXATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + error = copy_struct_from_user(&fsx, sizeof(struct fsxattr), ufsx, usize);
> + if (error)
> + return error;
> +
> + fsxattr_to_fileattr(&fsx, &fa);
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (!name) {
Same comment as above.
> + CLASS(fd, f)(dfd);
> +
> + if (fd_empty(f))
> + return -EBADF;
> +
> + idmap = file_mnt_idmap(fd_file(f));
> + dentry = file_dentry(fd_file(f));
> + mnt = fd_file(f)->f_path.mnt;
This is a UAF. fd_file(f)->f_path.mnt and file_dentry(fd_file(f)) will
get auto cleaned at the end of the scope. By the time you call
vfs_fileattr_set() nothing pins them anymore...
In general, same comment about unifying the branches as for the get case
via path_get() as above. And just keep the path around don't store mount
and dentry separately.
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + return error;
> +
> + idmap = mnt_idmap(filepath.mnt);
> + dentry = filepath.dentry;
> + mnt = filepath.mnt;
> + }
> +
> + error = mnt_want_write(mnt);
> + if (!error) {
> + error = vfs_fileattr_set(idmap, dentry, &fa);
> + if (error == -ENOIOCTLCMD)
> + error = -EOPNOTSUPP;
> + mnt_drop_write(mnt);
> + }
> +
> + path_put(&filepath);
> + return error;
> +}
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index c6333204d45130eb022f6db460eea34a1f6e91db..e242ea39b3e63a8008bc777764b616fd63bd40c4 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -371,6 +371,12 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
> asmlinkage long sys_lremovexattr(const char __user *path,
> const char __user *name);
> asmlinkage long sys_fremovexattr(int fd, const char __user *name);
> +asmlinkage long sys_getfsxattrat(int dfd, const char __user *filename,
> + struct fsxattr __user *ufsx, size_t usize,
> + unsigned int at_flags);
> +asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
> + struct fsxattr __user *ufsx, size_t usize,
> + unsigned int at_flags);
> asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
> asmlinkage long sys_eventfd2(unsigned int count, int flags);
> asmlinkage long sys_epoll_create1(int flags);
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -850,8 +850,14 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
> #define __NR_removexattrat 466
> __SYSCALL(__NR_removexattrat, sys_removexattrat)
>
> +/* fs/inode.c */
> +#define __NR_getfsxattrat 467
> +__SYSCALL(__NR_getfsxattrat, sys_getfsxattrat)
> +#define __NR_setfsxattrat 468
> +__SYSCALL(__NR_setfsxattrat, sys_setfsxattrat)
> +
> #undef __NR_syscalls
> -#define __NR_syscalls 467
> +#define __NR_syscalls 469
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 7539717707337a8cb22396a869baba3bafa08371..aed753e5d50c97da9b895a187fdaecf0477db74b 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -139,6 +139,9 @@ struct fsxattr {
> unsigned char fsx_pad[8];
> };
>
> +#define FSXATTR_SIZE_VER0 28
> +#define FSXATTR_SIZE_LATEST FSXATTR_SIZE_VER0
> +
> /*
> * Flags for the fsx_xflags field
> */
>
> --
> 2.47.2
>
^ permalink raw reply
* Re: [PATCH v4 3/3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-04-22 15:14 UTC (permalink / raw)
To: Amir Goldstein
Cc: Andrey Albershteyn, Richard Henderson, Matt Turner, Russell King,
Catalin Marinas, Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
Arnd Bergmann, Pali Rohár, Paul Moore, James Morris,
Serge E. Hallyn, linux-alpha, linux-kernel, linux-arm-kernel,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, linux-security-module,
linux-api, linux-arch, linux-xfs
In-Reply-To: <20250422-suchen-filmpreis-3573a913457c@brauner>
On Tue, Apr 22, 2025 at 04:31:29PM +0200, Christian Brauner wrote:
> On Thu, Mar 27, 2025 at 12:39:28PM +0100, Amir Goldstein wrote:
> > On Thu, Mar 27, 2025 at 10:33 AM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> > >
> > > On 2025-03-23 09:56:25, Amir Goldstein wrote:
> > > > On Fri, Mar 21, 2025 at 8:49 PM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> > > > >
> > > > > From: Andrey Albershteyn <aalbersh@redhat.com>
> > > > >
> > > > > Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
> > > > > extended attributes/flags. The syscalls take parent directory fd and
> > > > > path to the child together with struct fsxattr.
> > > > >
> > > > > This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> > > > > that file don't need to be open as we can reference it with a path
> > > > > instead of fd. By having this we can manipulated inode extended
> > > > > attributes not only on regular files but also on special ones. This
> > > > > is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> > > > > we can not call ioctl() directly on the filesystem inode using fd.
> > > > >
> > > > > This patch adds two new syscalls which allows userspace to get/set
> > > > > extended inode attributes on special files by using parent directory
> > > > > and a path - *at() like syscall.
> > > > >
> > > > > CC: linux-api@vger.kernel.org
> > > > > CC: linux-fsdevel@vger.kernel.org
> > > > > CC: linux-xfs@vger.kernel.org
> > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > > > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > > > ---
> > > > ...
> > > > > +SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename,
> > > > > + struct fsxattr __user *, ufsx, size_t, usize,
> > > > > + unsigned int, at_flags)
> > > > > +{
> > > > > + struct fileattr fa;
> > > > > + struct path filepath;
> > > > > + int error;
> > > > > + unsigned int lookup_flags = 0;
> > > > > + struct filename *name;
> > > > > + struct mnt_idmap *idmap;.
> > > >
> > > > > + struct dentry *dentry;
> > > > > + struct vfsmount *mnt;
> > > > > + struct fsxattr fsx = {};
> > > > > +
> > > > > + BUILD_BUG_ON(sizeof(struct fsxattr) < FSXATTR_SIZE_VER0);
> > > > > + BUILD_BUG_ON(sizeof(struct fsxattr) != FSXATTR_SIZE_LATEST);
> > > > > +
> > > > > + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> > > > > + lookup_flags |= LOOKUP_FOLLOW;
> > > > > +
> > > > > + if (at_flags & AT_EMPTY_PATH)
> > > > > + lookup_flags |= LOOKUP_EMPTY;
> > > > > +
> > > > > + if (usize > PAGE_SIZE)
> > > > > + return -E2BIG;
> > > > > +
> > > > > + if (usize < FSXATTR_SIZE_VER0)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + error = copy_struct_from_user(&fsx, sizeof(struct fsxattr), ufsx, usize);
> > > > > + if (error)
> > > > > + return error;
> > > > > +
> > > > > + fsxattr_to_fileattr(&fsx, &fa);
> > > > > +
> > > > > + name = getname_maybe_null(filename, at_flags);
> > > > > + if (!name) {
> > > > > + CLASS(fd, f)(dfd);
> > > > > +
> > > > > + if (fd_empty(f))
> > > > > + return -EBADF;
> > > > > +
> > > > > + idmap = file_mnt_idmap(fd_file(f));
> > > > > + dentry = file_dentry(fd_file(f));
> > > > > + mnt = fd_file(f)->f_path.mnt;
> > > > > + } else {
> > > > > + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> > > > > + NULL);
> > > > > + if (error)
> > > > > + return error;
> > > > > +
> > > > > + idmap = mnt_idmap(filepath.mnt);
> > > > > + dentry = filepath.dentry;
> > > > > + mnt = filepath.mnt;
> > > > > + }
> > > > > +
> > > > > + error = mnt_want_write(mnt);
> > > > > + if (!error) {
> > > > > + error = vfs_fileattr_set(idmap, dentry, &fa);
> > > > > + if (error == -ENOIOCTLCMD)
> > > > > + error = -EOPNOTSUPP;
> > > >
> > > > This is awkward.
> > > > vfs_fileattr_set() should return -EOPNOTSUPP.
> > > > ioctl_setflags() could maybe convert it to -ENOIOCTLCMD,
> > > > but looking at similar cases ioctl_fiemap(), ioctl_fsfreeze() the
> > > > ioctl returns -EOPNOTSUPP.
> > > >
> > > > I don't think it is necessarily a bad idea to start returning
> > > > -EOPNOTSUPP instead of -ENOIOCTLCMD for the ioctl
> > > > because that really reflects the fact that the ioctl is now implemented
> > > > in vfs and not in the specific fs.
> > > >
> > > > and I think it would not be a bad idea at all to make that change
> > > > together with the merge of the syscalls as a sort of hint to userspace
> > > > that uses the ioctl, that the sycalls API exists.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > >
> > > Hmm, not sure what you're suggesting here. I see it as:
> > > - get/setfsxattrat should return EOPNOTSUPP as it make more sense
> > > than ENOIOCTLCMD
> > > - ioctl_setflags returns ENOIOCTLCMD which also expected
> > >
> > > Don't really see a reason to change what vfs_fileattr_set() returns
> > > and then copying this if() to other places or start returning
> > > EOPNOTSUPP.
> >
> > ENOIOCTLCMD conceptually means that the ioctl command is unknown
> > This is not the case since ->fileattr_[gs]et() became a vfs API
>
> vfs_fileattr_{g,s}et() should not return ENOIOCTLCMD. Change the return
> code to EOPNOTSUPP and then make EOPNOTSUPP be translated to ENOTTY on
> on overlayfs and to ENOIOCTLCMD in ecryptfs and in fs/ioctl.c. This way
> we get a clean VFS api while retaining current behavior. Amir can do his
> cleanup based on that.
Also this get/set dance is not something new apis should do. It should
be handled like setattr_prepare() or generic_fillattr() where the
filesystem calls a VFS helper and that does all of this based on the
current state of the inode instead of calling into the filesystem twice:
int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
struct fileattr *fa)
{
<snip>
inode_lock(inode);
err = vfs_fileattr_get(dentry, &old_ma);
if (!err) {
/* initialize missing bits from old_ma */
if (fa->flags_valid) {
<snip>
err = fileattr_set_prepare(inode, &old_ma, fa);
if (!err && !security_inode_setfsxattr(inode, fa))
err = inode->i_op->fileattr_set(idmap, dentry, fa);
^ permalink raw reply
* Re: newlines in filenames; POSIX.1-2024
From: Theodore Ts'o @ 2025-04-22 22:21 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: linux-kernel, linux-api, linux-man
In-Reply-To: <iezzxq25mqdcapusb32euu3fgvz7djtrn5n66emb72jb3bqltx@lr2545vnc55k>
On Wed, Apr 16, 2025 at 06:50:00PM +0200, Alejandro Colomar wrote:
>
> I'm updating the manual pages for POSIX.1-2024. One of the changes
> in this revision is that POSIX now encourages implementations to
> disallow using new-line characters in file names.
>
> Historically, Linux (and maybe all existing POSIX systems?) has
> allowed new-line characters in file names.
Do we have any information of which implementations (if any) might
decide to disallow new-line characters?
If the Austin Group is going to add these sorts of "encouragements"
without engaging with us dirctly, it seems to be much like King Canute
commanding that the tide not come in....
Personally, I'm not convinced a newline is any different from any
number of weird-sh*t characters, such as zero-width space Unicode
characters, ASCII ETX or EOF characters, etc.
I suppose we could add a new mount option which disallows the
weird-sh*t characters, but I bet it will break some userspace
programs, and it also begs the question of *which* weird-sh*t
characters should be disallowed by the kernel.
> I guess there's no intention to change that behavior. But I should
> ask. I thought of adding this paragraph to all pages that create
> file names:
>
> +.SH CAVEATS
> +POSIX.1-2024 encourages implementations to
> +disallow creation of filenames containing new-line characters.
> +Linux doesn't follow this,
> +and allows using new-line characters.
>
> Are there any comments?
I think this is giving the Austin Group way more attention/respect
than they deserve, especially when it's an optional "encourage", but
whatever...
- Ted
^ permalink raw reply
* Re: [PATCH v8 05/15] xfs: ignore HW which cannot atomic write a single block
From: Darrick J. Wong @ 2025-04-23 0:38 UTC (permalink / raw)
To: John Garry
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-6-john.g.garry@oracle.com>
On Tue, Apr 22, 2025 at 12:27:29PM +0000, John Garry wrote:
> Currently only HW which can write at least 1x block is supported.
>
> For supporting atomic writes > 1x block, a CoW-based method will also be
> used and this will not be resticted to using HW which can write >= 1x
> block.
>
> However for deciding if HW-based atomic writes can be used, we need to
> start adding checks for write length < HW min, which complicates the code.
> Indeed, a statx field similar to unit_max_opt should also be added for this
> minimum, which is undesirable.
>
> HW which can only write > 1x blocks would be uncommon and quite weird, so
> let's just not support it.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> fs/xfs/xfs_inode.h | 17 ++++++++---------
> fs/xfs/xfs_mount.c | 14 ++++++++++++++
> fs/xfs/xfs_mount.h | 4 ++++
> 3 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index cff643cd03fc..725cd7c16a6e 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -355,20 +355,19 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
> #define xfs_inode_buftarg(ip) \
> (XFS_IS_REALTIME_INODE(ip) ? \
> (ip)->i_mount->m_rtdev_targp : (ip)->i_mount->m_ddev_targp)
> +/*
> + * Return max atomic write unit for a given inode.
> + */
> +#define xfs_inode_hw_atomicwrite_max(ip) \
> + (XFS_IS_REALTIME_INODE(ip) ? \
> + (ip)->i_mount->m_rt_awu_hw_max : \
> + (ip)->i_mount->m_dd_awu_hw_max)
>
> static inline bool
> xfs_inode_can_hw_atomicwrite(
> struct xfs_inode *ip)
> {
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_buftarg *target = xfs_inode_buftarg(ip);
> -
> - if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min)
> - return false;
> - if (mp->m_sb.sb_blocksize > target->bt_bdev_awu_max)
> - return false;
> -
> - return true;
> + return xfs_inode_hw_atomicwrite_max(ip);
> }
>
> /*
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 00b53f479ece..ee68c026e6cd 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1082,6 +1082,20 @@ xfs_mountfs(
> xfs_zone_gc_start(mp);
> }
>
> + /*
> + * Set atomic write unit max for mp. Ignore devices which cannot atomic
> + * a single block, as they would be uncommon and more difficult to
> + * support.
> + */
> + if (mp->m_ddev_targp->bt_bdev_awu_min <= mp->m_sb.sb_blocksize &&
> + mp->m_ddev_targp->bt_bdev_awu_max >= mp->m_sb.sb_blocksize)
> + mp->m_dd_awu_hw_max = mp->m_ddev_targp->bt_bdev_awu_max;
If we don't want to use the device's atomic write capabilities due to
fsblock alignment problems, why not just zero out bt_bdev_awu_min/max?
That would cut down on the number of "awu" variables around the
codebase.
/*
* Ignore hardware atomic writes if the device can't handle a single
* fsblock for us. Most devices set the min_awu to the LBA size, but
* the spec allows for a functionality gap.
*/
static void
xfs_buftarg_reconcile_awu(
struct xfs_buftarg *btp)
{
struct xfs_mount *mp = btp->bt_mount;
if (btp->bt_bdev_awu_min > mp->m_sb.sb_blocksize ||
btp->bt_bdev_awu_max < mp->m_sb.sb_blocksize) {
btp->bt_bdev_awu_min = 0;
btp->bt_bdev_awu_max = 0;
}
}
xfs_buftarg_reconcile_awu(mp->m_ddev_targp);
if (mp->m_rtdev_targp)
xfs_buftarg_reconcile_awu(mp->m_rtdev_targp);
Hrm?
--D
> +
> + if (mp->m_rtdev_targp &&
> + mp->m_rtdev_targp->bt_bdev_awu_min <= mp->m_sb.sb_blocksize &&
> + mp->m_rtdev_targp->bt_bdev_awu_max >= mp->m_sb.sb_blocksize)
> + mp->m_rt_awu_hw_max = mp->m_rtdev_targp->bt_bdev_awu_max;
> +
> return 0;
>
> out_agresv:
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index e5192c12e7ac..2819e160f0e9 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -231,6 +231,10 @@ typedef struct xfs_mount {
> unsigned int m_max_open_zones;
> unsigned int m_zonegc_low_space;
>
> + /* ddev and rtdev HW max atomic write size */
> + unsigned int m_dd_awu_hw_max;
> + unsigned int m_rt_awu_hw_max;
> +
> /*
> * Bitsets of per-fs metadata that have been checked and/or are sick.
> * Callers must hold m_sb_lock to access these two fields.
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Luis Chamberlain @ 2025-04-23 5:18 UTC (permalink / raw)
To: John Garry
Cc: Chris Mason, Josef Bacik, brauner, djwong, hch, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, Pankaj Raghav, Daniel Gomez
In-Reply-To: <69302bf1-78b4-4b95-8e9b-df56dd1091c0@oracle.com>
On Tue, Apr 22, 2025 at 07:08:32AM +0100, John Garry wrote:
> On 21/04/2025 22:18, Luis Chamberlain wrote:
> > > /*
> > > + * The retry mechanism is based on the ->iomap_begin method returning
> > > + * -ENOPROTOOPT, which would be when the REQ_ATOMIC-based write is not
> > > + * possible. The REQ_ATOMIC-based method typically not be possible if
> > > + * the write spans multiple extents or the disk blocks are misaligned.
> > > + */
> > > + if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
> > Based on feedback from LSFMM, due to the performance variaibility this
> > can introduce, it sounded like some folks would like to opt-in to not
> > have a software fallback and just require an error out.
> > > Could an option be added to not allow the software fallback?
>
> I still don't see the use in this.
Its not the use, its the concern for underdeterminism in performance.
> So consider userspace wants to write something atomically and we fail as a
> HW-based atomic write is not possible.
Sounds like a terrible predicant for those that want hw atomics and
reliability for it.
> What is userspace going to do next?
It would seem that would depend on their analysis on the number of
software fallbacks where a software atomic based solution is used and
the impact on performance.
> I heard something like "if HW-based atomics are not possible, then something
> has not been configured properly for the FS" - that something would be
> extent granularity and alignment, but we don't have a method to ensure this.
> That is the whole point of having a FS fallback.
We do with LBS. Its perfectly deterministic to be aligned with a sector
size matching the block size, even for metadata writes.
> > If so, then I think the next patch would also need updating.
> >
> > Or are you suggesting that without the software fallback atomic writes
> > greater than fs block size are not possible?
>
> Yes, as XFS has no method to guarantee extent granularity and alignment.
Ah, I think the documentation for this featuer should make this clear,
it was not clear up to this point in patch review.
Luis
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Christoph Hellwig @ 2025-04-23 5:42 UTC (permalink / raw)
To: Darrick J. Wong
Cc: John Garry, brauner, hch, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <20250421164241.GD25700@frogsfrogsfrogs>
On Mon, Apr 21, 2025 at 09:42:41AM -0700, Darrick J. Wong wrote:
> Well it turns out that was a stupid question -- zoned=1 can't be enabled
> with reflink, which means there's no cow fallback so atomic writes just
> plain don't work:
Exactly. It is still on my todo list to support it, but there are a
few higher priority items on it as well, in addition to constant
interruptions for patch reviews :)
> I /think/ all you'd have to do is create an xfs_zoned_end_atomic_io
> function that does what xfs_zoned_end_io but with a single
> tr_atomic_ioend transaction; figure out how to convey "this is an
> atomic out of place write" to xfs_end_ioend so that it knows to call
> xfs_zoned_end_atomic_io; and then update the xfs_get_atomic_write*
> helpers.
Roughly, yes.
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Christoph Hellwig @ 2025-04-23 5:44 UTC (permalink / raw)
To: John Garry
Cc: Luis Chamberlain, Chris Mason, Josef Bacik, brauner, djwong, hch,
viro, jack, cem, linux-fsdevel, dchinner, linux-xfs, linux-kernel,
ojaswin, ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, Pankaj Raghav, Daniel Gomez
In-Reply-To: <69302bf1-78b4-4b95-8e9b-df56dd1091c0@oracle.com>
On Tue, Apr 22, 2025 at 07:08:32AM +0100, John Garry wrote:
> So consider userspace wants to write something atomically and we fail as a
> HW-based atomic write is not possible. What is userspace going to do next?
Exactly.
>
> I heard something like "if HW-based atomics are not possible, then
> something has not been configured properly for the FS" - that something
> would be extent granularity and alignment, but we don't have a method to
> ensure this. That is the whole point of having a FS fallback.
We now have the opt limit, right? (I'll review the reposted series
ASAP, but for now I'll assume it) They can just tune their applications
to it, and trigger on a trace point for the fallback to monitor it.
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: John Garry @ 2025-04-23 7:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Luis Chamberlain, Chris Mason, Josef Bacik, brauner, djwong, viro,
jack, cem, linux-fsdevel, dchinner, linux-xfs, linux-kernel,
ojaswin, ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, Pankaj Raghav, Daniel Gomez
In-Reply-To: <20250423054420.GB23087@lst.de>
On 23/04/2025 06:44, Christoph Hellwig wrote:
> On Tue, Apr 22, 2025 at 07:08:32AM +0100, John Garry wrote:
>> So consider userspace wants to write something atomically and we fail as a
>> HW-based atomic write is not possible. What is userspace going to do next?
> Exactly.
>
>> I heard something like "if HW-based atomics are not possible, then
>> something has not been configured properly for the FS" - that something
>> would be extent granularity and alignment, but we don't have a method to
>> ensure this. That is the whole point of having a FS fallback.
> We now have the opt limit, right?
right
> (I'll review the reposted series
> ASAP,
ok, cheers
> but for now I'll assume it) They can just tune their applications
> to it, and trigger on a trace point for the fallback to monitor it.
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: John Garry @ 2025-04-23 7:08 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Chris Mason, Josef Bacik, brauner, djwong, hch, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, Pankaj Raghav, Daniel Gomez
In-Reply-To: <aAh4L9crlnEf3uuJ@bombadil.infradead.org>
On 23/04/2025 06:18, Luis Chamberlain wrote:
> On Tue, Apr 22, 2025 at 07:08:32AM +0100, John Garry wrote:
>> On 21/04/2025 22:18, Luis Chamberlain wrote:
>>>> /*
>>>> + * The retry mechanism is based on the ->iomap_begin method returning
>>>> + * -ENOPROTOOPT, which would be when the REQ_ATOMIC-based write is not
>>>> + * possible. The REQ_ATOMIC-based method typically not be possible if
>>>> + * the write spans multiple extents or the disk blocks are misaligned.
>>>> + */
>>>> + if (ret == -ENOPROTOOPT && dops == &xfs_direct_write_iomap_ops) {
>>> Based on feedback from LSFMM, due to the performance variaibility this
>>> can introduce, it sounded like some folks would like to opt-in to not
>>> have a software fallback and just require an error out.
>>>> Could an option be added to not allow the software fallback?
>>
>> I still don't see the use in this.
>
> Its not the use, its the concern for underdeterminism in performance.
Sure, we don't offer RT performance guarantees, but what does?
>
>> So consider userspace wants to write something atomically and we fail as a
>> HW-based atomic write is not possible.
>
> Sounds like a terrible predicant for those that want hw atomics and
> reliability for it.
Well from our MySQL testing performance is good.
>
>> What is userspace going to do next?
>
> It would seem that would depend on their analysis on the number of
> software fallbacks where a software atomic based solution is used and
> the impact on performance.
sorry, but I don't understand this
>
>> I heard something like "if HW-based atomics are not possible, then something
>> has not been configured properly for the FS" - that something would be
>> extent granularity and alignment, but we don't have a method to ensure this.
>> That is the whole point of having a FS fallback.
>
> We do with LBS.
Sure, but not everyone wants LBS
> Its perfectly deterministic to be aligned with a sector
> size matching the block size, even for metadata writes.
>
>>> If so, then I think the next patch would also need updating.
>>>
>>> Or are you suggesting that without the software fallback atomic writes
>>> greater than fs block size are not possible?
>>
>> Yes, as XFS has no method to guarantee extent granularity and alignment.
>
> Ah, I think the documentation for this featuer should make this clear,
> it was not clear up to this point in patch review.
>
ok, that can be added
^ permalink raw reply
* Re: [PATCH v8 05/15] xfs: ignore HW which cannot atomic write a single block
From: John Garry @ 2025-04-23 7:15 UTC (permalink / raw)
To: Darrick J. Wong
Cc: brauner, hch, viro, jack, cem, linux-fsdevel, dchinner, linux-xfs,
linux-kernel, ojaswin, ritesh.list, martin.petersen, linux-ext4,
linux-block, catherine.hoang, linux-api
In-Reply-To: <20250423003823.GW25675@frogsfrogsfrogs>
On 23/04/2025 01:38, Darrick J. Wong wrote:
>>
>> + /*
>> + * Set atomic write unit max for mp. Ignore devices which cannot atomic
>> + * a single block, as they would be uncommon and more difficult to
>> + * support.
>> + */
>> + if (mp->m_ddev_targp->bt_bdev_awu_min <= mp->m_sb.sb_blocksize &&
>> + mp->m_ddev_targp->bt_bdev_awu_max >= mp->m_sb.sb_blocksize)
>> + mp->m_dd_awu_hw_max = mp->m_ddev_targp->bt_bdev_awu_max;
> If we don't want to use the device's atomic write capabilities due to
> fsblock alignment problems, why not just zero out bt_bdev_awu_min/max?
> That would cut down on the number of "awu" variables around the
> codebase.
Sure, I did consider this, but thought it a bit unpleasant to zero out
structure members like this.
Ideally we could have not set them in the first place, but need to know
the blocksize when xfs_alloc_buftarg() is called, but it is not yet set
for mp/sb. Is there any neat way to know the blocksize when
xfs_alloc_buftarg() is called?
>
> /*
> * Ignore hardware atomic writes if the device can't handle a single
> * fsblock for us. Most devices set the min_awu to the LBA size, but
> * the spec allows for a functionality gap.
> */
> static void
You would call this around the same point in xfs_mountfs(), right?
> xfs_buftarg_reconcile_awu(
> struct xfs_buftarg *btp)
> {
> struct xfs_mount *mp = btp->bt_mount;
>
> if (btp->bt_bdev_awu_min > mp->m_sb.sb_blocksize ||
> btp->bt_bdev_awu_max < mp->m_sb.sb_blocksize) {
> btp->bt_bdev_awu_min = 0;
> btp->bt_bdev_awu_max = 0;
> }
> }
>
> xfs_buftarg_reconcile_awu(mp->m_ddev_targp);
> if (mp->m_rtdev_targp)
> xfs_buftarg_reconcile_awu(mp->m_rtdev_targp);
>
> Hrm?
Cheers,
John
^ permalink raw reply
* Re: newlines in filenames; POSIX.1-2024
From: Alejandro Colomar @ 2025-04-23 7:31 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel, linux-api, linux-man
In-Reply-To: <20250422222131.GE569616@mit.edu>
[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]
Hi Ted,
On Tue, Apr 22, 2025 at 05:21:31PM -0500, Theodore Ts'o wrote:
> On Wed, Apr 16, 2025 at 06:50:00PM +0200, Alejandro Colomar wrote:
> >
> > I'm updating the manual pages for POSIX.1-2024. One of the changes
> > in this revision is that POSIX now encourages implementations to
> > disallow using new-line characters in file names.
> >
> > Historically, Linux (and maybe all existing POSIX systems?) has
> > allowed new-line characters in file names.
>
> Do we have any information of which implementations (if any) might
> decide to disallow new-line characters?
Such a list doesn't exist.
<http://austingroupbugs.net/view.php?id=251>
> If the Austin Group is going to add these sorts of "encouragements"
> without engaging with us dirctly, it seems to be much like King Canute
> commanding that the tide not come in....
>
> Personally, I'm not convinced a newline is any different from any
> number of weird-sh*t characters, such as zero-width space Unicode
> characters, ASCII ETX or EOF characters, etc.
Newline is slightly more problematic than those, especially in scripts.
But yes, other characters (mainly control characters) were also
discussed in that bug. From what I can read, it seems they were scared
that if they attempted to suggest banning all control characters at
once, there might be more opposition, and the standard would be toilet
paper.
> I suppose we could add a new mount option which disallows the
> weird-sh*t characters, but I bet it will break some userspace
> programs,
That's an interesting approach. Being an opt-in mount option, users
will only break at their will, and they can always go back to old mode
when they need to do some operation with weird-sh*t characters.
TBH, while I see the chances of breaking stuff (so I don't see this
being the default in a long time; maybe ever), I think an opt-in mode
would be interesting, for those that know that don't need to handle such
broken file names, to have a tighter system. I would enable such a mode
in my systems.
> and it also begs the question of *which* weird-sh*t
> characters should be disallowed by the kernel.
I think a mode for disallowing _any control characters_ (aka [:cntrl:],
aka 0-31) would be a good choice.
> > I guess there's no intention to change that behavior. But I should
> > ask. I thought of adding this paragraph to all pages that create
> > file names:
> >
> > +.SH CAVEATS
> > +POSIX.1-2024 encourages implementations to
> > +disallow creation of filenames containing new-line characters.
> > +Linux doesn't follow this,
> > +and allows using new-line characters.
> >
> > Are there any comments?
>
> I think this is giving the Austin Group way more attention/respect
> than they deserve, especially when it's an optional "encourage", but
> whatever...
I'm not worried about that, I was more worried about the churn in the
pages. I later remembered we have a pathname(7) page, so I'll put it
there, just once.
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Luis Chamberlain @ 2025-04-23 7:36 UTC (permalink / raw)
To: John Garry
Cc: Chris Mason, Josef Bacik, brauner, djwong, hch, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api, Pankaj Raghav, Daniel Gomez
In-Reply-To: <cf67f166-4c65-4d76-a3a2-1ad2614e89b7@oracle.com>
On Wed, Apr 23, 2025 at 08:08:40AM +0100, John Garry wrote:
> On 23/04/2025 06:18, Luis Chamberlain wrote:
> > On Tue, Apr 22, 2025 at 07:08:32AM +0100, John Garry wrote:
> > > On 21/04/2025 22:18, Luis Chamberlain wrote:
> >
> > Sounds like a terrible predicant for those that want hw atomics and
> > reliability for it.
>
> Well from our MySQL testing performance is good.
Good to hear, thanks!
Luis
^ permalink raw reply
* Re: [PATCH v8 05/15] xfs: ignore HW which cannot atomic write a single block
From: Christoph Hellwig @ 2025-04-23 8:10 UTC (permalink / raw)
To: John Garry
Cc: Darrick J. Wong, brauner, hch, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <f467a921-e7dd-4f5b-ac9f-c6e8c043143c@oracle.com>
On Wed, Apr 23, 2025 at 08:15:43AM +0100, John Garry wrote:
> Ideally we could have not set them in the first place, but need to know the
> blocksize when xfs_alloc_buftarg() is called, but it is not yet set for
> mp/sb. Is there any neat way to know the blocksize when xfs_alloc_buftarg()
> is called?
The buftarg is needed to read the superblock, which is used to determine
the block size, so no.
But maybe we should just delay setting the atomic values until later so
that it can be done in a single pass? E.g. into xfs_setsize_buftarg
which then should probably be rename to something like
xfs_buftarg_setup.
^ permalink raw reply
* Re: [PATCH v8 09/15] xfs: add xfs_atomic_write_cow_iomap_begin()
From: Christoph Hellwig @ 2025-04-23 8:12 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-10-john.g.garry@oracle.com>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 10/15] xfs: add large atomic writes checks in xfs_direct_write_iomap_begin()
From: Christoph Hellwig @ 2025-04-23 8:16 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-11-john.g.garry@oracle.com>
On Tue, Apr 22, 2025 at 12:27:34PM +0000, John Garry wrote:
> For when large atomic writes (> 1x FS block) are supported, there will be
> various occasions when HW offload may not be possible.
>
> Such instances include:
> - unaligned extent mapping wrt write length
> - extent mappings which do not cover the full write, e.g. the write spans
> sparse or mixed-mapping extents
> - the write length is greater than HW offload can support
or simply no hardware support at all.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 10/15] xfs: add large atomic writes checks in xfs_direct_write_iomap_begin()
From: John Garry @ 2025-04-23 8:18 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250423081614.GC28307@lst.de>
On 23/04/2025 09:16, Christoph Hellwig wrote:
> On Tue, Apr 22, 2025 at 12:27:34PM +0000, John Garry wrote:
>> For when large atomic writes (> 1x FS block) are supported, there will be
>> various occasions when HW offload may not be possible.
>>
>> Such instances include:
>> - unaligned extent mapping wrt write length
>> - extent mappings which do not cover the full write, e.g. the write spans
>> sparse or mixed-mapping extents
>> - the write length is greater than HW offload can support
>
> or simply no hardware support at all.
sure, will add
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
cheers
^ permalink raw reply
* Re: [PATCH v7 11/14] xfs: add xfs_file_dio_write_atomic()
From: Christoph Hellwig @ 2025-04-23 8:19 UTC (permalink / raw)
To: Darrick J. Wong
Cc: John Garry, brauner, hch, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <20250423054251.GA23087@lst.de>
On Wed, Apr 23, 2025 at 07:42:51AM +0200, Christoph Hellwig wrote:
> On Mon, Apr 21, 2025 at 09:42:41AM -0700, Darrick J. Wong wrote:
> > Well it turns out that was a stupid question -- zoned=1 can't be enabled
> > with reflink, which means there's no cow fallback so atomic writes just
> > plain don't work:
>
> Exactly. It is still on my todo list to support it, but there are a
> few higher priority items on it as well, in addition to constant
> interruptions for patch reviews :)
Actually, for zoned we don't need reflink support - as we always write
out place only the stuffing of multiple remaps into a single transaction
is needed. Still no need to force John to do this work, I can look into
this (probably fairly trivial) work once we have good enough test cases
in xfstests that I can trust them to verify I got things right.
^ permalink raw reply
* Re: [PATCH v8 11/15] xfs: commit CoW-based atomic writes atomically
From: Christoph Hellwig @ 2025-04-23 8:23 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-12-john.g.garry@oracle.com>
On Tue, Apr 22, 2025 at 12:27:35PM +0000, John Garry wrote:
> +STATIC void
Didn't we phase out STATIC for new code?
> +xfs_calc_default_atomic_ioend_reservation(
> + struct xfs_mount *mp,
> + struct xfs_trans_resv *resp)
> +{
> + if (xfs_has_reflink(mp))
> + resp->tr_atomic_ioend = resp->tr_itruncate;
> + else
> + memset(&resp->tr_atomic_ioend, 0,
> + sizeof(resp->tr_atomic_ioend));
> +}
What is the point of zeroing out the structure for the non-reflink
case? Just as a poision for not using it when not supported as no
code should be doing that? Just thinking of this because it is a
potentially nasty landmine for the zoned atomic support.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 12/15] xfs: add xfs_file_dio_write_atomic()
From: Christoph Hellwig @ 2025-04-23 8:25 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-13-john.g.garry@oracle.com>
On Tue, Apr 22, 2025 at 12:27:36PM +0000, John Garry wrote:
> Add xfs_file_dio_write_atomic() for dedicated handling of atomic writes.
>
> The function works based on two operating modes:
> - HW offload, i.e. REQ_ATOMIC-based
> - CoW based with out-of-places write and atomic extent remapping
>
> The preferred method is HW offload as it will be faster. If HW offload is
> not possible, then we fallback to the CoW-based method.
>
> HW offload would not be possible for the write length exceeding the HW
> offload limit, the write spanning multiple extents, unaligned disk blocks,
> etc.
>
> Apart from the write exceeding the HW offload limit, other conditions for
> HW offload can only be detected in the iomap handling for the write. As
> such, we use a fallback method to issue the write if we detect in the
> ->iomap_begin() handler that HW offload is not possible. Special code
> -ENOPROTOOPT is returned from ->iomap_begin() to inform that HW offload
> not possible.
This text could use a little rewrite starting with the fact that the
hardware offload now isn't required to start with and entirely
optional and then flow from the there to state when we can use it
instead of when we can't use it.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 13/15] xfs: add xfs_compute_atomic_write_unit_max()
From: Christoph Hellwig @ 2025-04-23 8:26 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-14-john.g.garry@oracle.com>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 14/15] xfs: update atomic write limits
From: Christoph Hellwig @ 2025-04-23 8:27 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-15-john.g.garry@oracle.com>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v8 12/15] xfs: add xfs_file_dio_write_atomic()
From: John Garry @ 2025-04-23 8:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: brauner, djwong, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250423082525.GB29539@lst.de>
On 23/04/2025 09:25, Christoph Hellwig wrote:
> On Tue, Apr 22, 2025 at 12:27:36PM +0000, John Garry wrote:
>> Add xfs_file_dio_write_atomic() for dedicated handling of atomic writes.
>>
>> The function works based on two operating modes:
>> - HW offload, i.e. REQ_ATOMIC-based
>> - CoW based with out-of-places write and atomic extent remapping
>>
>> The preferred method is HW offload as it will be faster. If HW offload is
>> not possible, then we fallback to the CoW-based method.
>>
>> HW offload would not be possible for the write length exceeding the HW
>> offload limit, the write spanning multiple extents, unaligned disk blocks,
>> etc.
>>
>> Apart from the write exceeding the HW offload limit, other conditions for
>> HW offload can only be detected in the iomap handling for the write. As
>> such, we use a fallback method to issue the write if we detect in the
>> ->iomap_begin() handler that HW offload is not possible. Special code
>> -ENOPROTOOPT is returned from ->iomap_begin() to inform that HW offload
>> not possible.
>
> This text could use a little rewrite starting with the fact that the
> hardware offload now isn't required to start with and entirely
> optional and then flow from the there to state when we can use it
> instead of when we can't use it.
ok, sure
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks!
^ permalink raw reply
* Re: [PATCH v8 05/15] xfs: ignore HW which cannot atomic write a single block
From: John Garry @ 2025-04-23 8:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, brauner, viro, jack, cem, linux-fsdevel,
dchinner, linux-xfs, linux-kernel, ojaswin, ritesh.list,
martin.petersen, linux-ext4, linux-block, catherine.hoang,
linux-api
In-Reply-To: <20250423081055.GA28307@lst.de>
On 23/04/2025 09:10, Christoph Hellwig wrote:
> On Wed, Apr 23, 2025 at 08:15:43AM +0100, John Garry wrote:
>> Ideally we could have not set them in the first place, but need to know the
>> blocksize when xfs_alloc_buftarg() is called, but it is not yet set for
>> mp/sb. Is there any neat way to know the blocksize when xfs_alloc_buftarg()
>> is called?
>
> The buftarg is needed to read the superblock, which is used to determine
> the block size, so no.
>
> But maybe we should just delay setting the atomic values until later so
> that it can be done in a single pass? E.g. into xfs_setsize_buftarg
> which then should probably be rename to something like
> xfs_buftarg_setup.
>
How about just do away with btp->bt_bdev_awu_{min, max} struct members,
and call bdev_atomic_write_unit_max(mp->m_ddev_targp->bt_bdev) [and same
for RT] to later to set the mp awu max values at mountfs time? I think
that would work..
^ permalink raw reply
* Re: [PATCH v8 15/15] xfs: allow sysadmins to specify a maximum atomic write limit at mount time
From: Christoph Hellwig @ 2025-04-23 8:32 UTC (permalink / raw)
To: John Garry
Cc: brauner, djwong, hch, viro, jack, cem, linux-fsdevel, dchinner,
linux-xfs, linux-kernel, ojaswin, ritesh.list, martin.petersen,
linux-ext4, linux-block, catherine.hoang, linux-api
In-Reply-To: <20250422122739.2230121-16-john.g.garry@oracle.com>
On Tue, Apr 22, 2025 at 12:27:39PM +0000, John Garry wrote:
> From: "Darrick J. Wong" <djwong@kernel.org>
>
> Introduce a mount option to allow sysadmins to specify the maximum size
> of an atomic write. If the filesystem can work with the supplied value,
> that becomes the new guaranteed maximum.
>
> The value mustn't be too big for the existing filesystem geometry (max
> write size, max AG/rtgroup size). We dynamically recompute the
> tr_atomic_write transaction reservation based on the given block size,
> check that the current log size isn't less than the new minimum log size
> constraints, and set a new maximum.
>
> The actual software atomic write max is still computed based off of
> tr_atomic_ioend the same way it has for the past few commits.
The cap is a good idea, but a mount option for something that has
strong effects for persistent application formats is a little suboptimal.
But adding a sb field and an incompat bit wouldn't be great either.
Maybe this another use case for a trusted xattr on the root inode like
the autofsck flag?
^ permalink raw reply
* Re: [PATCH v8 05/15] xfs: ignore HW which cannot atomic write a single block
From: Christoph Hellwig @ 2025-04-23 8:33 UTC (permalink / raw)
To: John Garry
Cc: Christoph Hellwig, Darrick J. Wong, brauner, viro, jack, cem,
linux-fsdevel, dchinner, linux-xfs, linux-kernel, ojaswin,
ritesh.list, martin.petersen, linux-ext4, linux-block,
catherine.hoang, linux-api
In-Reply-To: <f27ea8f7-700a-4fb1-b9cd-a0cba04c9e47@oracle.com>
On Wed, Apr 23, 2025 at 09:28:14AM +0100, John Garry wrote:
>> But maybe we should just delay setting the atomic values until later so
>> that it can be done in a single pass? E.g. into xfs_setsize_buftarg
>> which then should probably be rename to something like
>> xfs_buftarg_setup.
>>
>
> How about just do away with btp->bt_bdev_awu_{min, max} struct members, and
> call bdev_atomic_write_unit_max(mp->m_ddev_targp->bt_bdev) [and same for
> RT] to later to set the mp awu max values at mountfs time? I think that
> would work..
Sounds reasonable.
^ 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