[PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Mateusz Guzik]

The newly used helper also checks for empty ("") paths.

NULL paths with any flag value other than AT_EMPTY_PATH go the usual
route and end up with -EFAULT to retain compatibility (Rust is abusing
calls of the sort to detect availability of statx).

This avoids path lookup code, lockref management, memory allocation and
in case of NULL path userspace memory access (which can be quite
expensive with SMAP on x86_64).

Benchmarked with statx(..., AT_EMPTY_PATH, ...) running on Sapphire
Rapids, with the "" path for the first two cases and NULL for the last
one.

Results in ops/s:
stock:     4231237
pre-check: 5944063 (+40%)
NULL path: 6601619 (+11%/+56%)

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

Diffed against fs-next and assumes c050122bdbb4 ("fs: new helper
vfs_empty_path()") from vfs.empty.path is already applied.

WARNING: io_uring remains untested (modulo compilation). I presume
Jens has a handy way of making sure things still work. 

While the io_uring part can be added at a later date, I'm trying to
avoid a scenario where someone has code which works with the NULL path
and breaks when moving to io_uring. I am not going to argue about it ,
worst case changes to io_uring can be trivially dropped and someone(tm)
can add their own variant whenever they see fit.

v3:
- also support AT_STATX* flags for NULL and ""

v2:
- support glibc passing AT_NO_AUTOMOUNT | AT_EMPTY_PATH
- tidy up some commentary
- drop the fdget_raw CLASS addition as it is already present in newer
  trees

 fs/internal.h    |   2 +
 fs/stat.c        | 107 +++++++++++++++++++++++++++++++++++------------
 io_uring/statx.c |  22 ++++++----
 3 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 84f371193f74..1d820018e6dc 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -247,6 +247,8 @@ extern const struct dentry_operations ns_dentry_operations;
 int getname_statx_lookup_flags(int flags);
 int do_statx(int dfd, struct filename *filename, unsigned int flags,
 	     unsigned int mask, struct statx __user *buffer);
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+		struct statx __user *buffer);
 
 /*
  * fs/splice.c:
diff --git a/fs/stat.c b/fs/stat.c
index 5039c34a385d..3778e605eac8 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -214,6 +214,43 @@ int getname_statx_lookup_flags(int flags)
 	return lookup_flags;
 }
 
+static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
+			  u32 request_mask)
+{
+	int error = vfs_getattr(path, stat, request_mask, flags);
+
+	if (request_mask & STATX_MNT_ID_UNIQUE) {
+		stat->mnt_id = real_mount(path->mnt)->mnt_id_unique;
+		stat->result_mask |= STATX_MNT_ID_UNIQUE;
+	} else {
+		stat->mnt_id = real_mount(path->mnt)->mnt_id;
+		stat->result_mask |= STATX_MNT_ID;
+	}
+
+	if (path->mnt->mnt_root == path->dentry)
+		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
+	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
+
+	/* Handle STATX_DIOALIGN for block devices. */
+	if (request_mask & STATX_DIOALIGN) {
+		struct inode *inode = d_backing_inode(path->dentry);
+
+		if (S_ISBLK(inode->i_mode))
+			bdev_statx_dioalign(inode, stat);
+	}
+
+	return error;
+}
+
+static int vfs_statx_fd(int fd, int flags, struct kstat *stat,
+			  u32 request_mask)
+{
+	CLASS(fd_raw, f)(fd);
+	if (!f.file)
+		return -EBADF;
+	return vfs_statx_path(&f.file->f_path, flags, stat, request_mask);
+}
+
 /**
  * vfs_statx - Get basic and extra attributes by filename
  * @dfd: A file descriptor representing the base dir for a relative filename
@@ -243,36 +280,13 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
 retry:
 	error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
 	if (error)
-		goto out;
-
-	error = vfs_getattr(&path, stat, request_mask, flags);
-
-	if (request_mask & STATX_MNT_ID_UNIQUE) {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
-		stat->result_mask |= STATX_MNT_ID_UNIQUE;
-	} else {
-		stat->mnt_id = real_mount(path.mnt)->mnt_id;
-		stat->result_mask |= STATX_MNT_ID;
-	}
-
-	if (path.mnt->mnt_root == path.dentry)
-		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
-	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
-
-	/* Handle STATX_DIOALIGN for block devices. */
-	if (request_mask & STATX_DIOALIGN) {
-		struct inode *inode = d_backing_inode(path.dentry);
-
-		if (S_ISBLK(inode->i_mode))
-			bdev_statx_dioalign(inode, stat);
-	}
-
+		return error;
+	error = vfs_statx_path(&path, flags, stat, request_mask);
 	path_put(&path);
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
 		goto retry;
 	}
-out:
 	return error;
 }
 
@@ -683,16 +697,40 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,
 	return cp_statx(&stat, buffer);
 }
 
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+	     struct statx __user *buffer)
+{
+	struct kstat stat;
+	int error;
+
+	if (mask & STATX__RESERVED)
+		return -EINVAL;
+	if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
+		return -EINVAL;
+
+	/* STATX_CHANGE_COOKIE is kernel-only for now. Ignore requests
+	 * from userland.
+	 */
+	mask &= ~STATX_CHANGE_COOKIE;
+
+	error = vfs_statx_fd(fd, flags, &stat, mask);
+	if (error)
+		return error;
+
+	return cp_statx(&stat, buffer);
+}
+
 /**
  * sys_statx - System call to get enhanced stats
  * @dfd: Base directory to pathwalk from *or* fd to stat.
- * @filename: File to stat or "" with AT_EMPTY_PATH
+ * @filename: File to stat or either NULL or "" with AT_EMPTY_PATH
  * @flags: AT_* flags to control pathwalk.
  * @mask: Parts of statx struct actually required.
  * @buffer: Result buffer.
  *
  * Note that fstat() can be emulated by setting dfd to the fd of interest,
- * supplying "" as the filename and setting AT_EMPTY_PATH in the flags.
+ * supplying "" (or preferably NULL) as the filename and setting AT_EMPTY_PATH
+ * in the flags.
  */
 SYSCALL_DEFINE5(statx,
 		int, dfd, const char __user *, filename, unsigned, flags,
@@ -700,8 +738,23 @@ SYSCALL_DEFINE5(statx,
 		struct statx __user *, buffer)
 {
 	int ret;
+	unsigned lflags;
 	struct filename *name;
 
+	/*
+	 * Short-circuit handling of NULL and "" paths.
+	 *
+	 * For a NULL path we require and accept only the AT_EMPTY_PATH flag
+	 * (possibly |'d with AT_STATX flags).
+	 *
+	 * However, glibc on 32-bit architectures implements fstatat as statx
+	 * with the "" pathname and AT_NO_AUTOMOUNT | AT_EMPTY_PATH flags.
+	 * Supporting this results in the uglification below.
+	 */
+	lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE);
+	if (lflags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
+		return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
+
 	name = getname_flags(filename, getname_statx_lookup_flags(flags));
 	ret = do_statx(dfd, name, flags, mask, buffer);
 	putname(name);
diff --git a/io_uring/statx.c b/io_uring/statx.c
index f7f9b202eec0..2b27af51fc12 100644
--- a/io_uring/statx.c
+++ b/io_uring/statx.c
@@ -23,6 +23,7 @@ struct io_statx {
 int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_statx *sx = io_kiocb_to_cmd(req, struct io_statx);
+	struct filename *filename;
 	const char __user *path;
 
 	if (sqe->buf_index || sqe->splice_fd_in)
@@ -36,14 +37,16 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	sx->buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
 	sx->flags = READ_ONCE(sqe->statx_flags);
 
-	sx->filename = getname_flags(path,
-				     getname_statx_lookup_flags(sx->flags));
-
-	if (IS_ERR(sx->filename)) {
-		int ret = PTR_ERR(sx->filename);
-
+	if ((sx->flags & (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE)) ==
+	    (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE) &&
+	    vfs_empty_path(sx->dfd, path)) {
 		sx->filename = NULL;
-		return ret;
+	} else {
+		filename = getname_flags(path,
+					 getname_statx_lookup_flags(sx->flags));
+		if (IS_ERR(filename))
+			return PTR_ERR(filename);
+		sx->filename = filename;
 	}
 
 	req->flags |= REQ_F_NEED_CLEANUP;
@@ -58,7 +61,10 @@ int io_statx(struct io_kiocb *req, unsigned int issue_flags)
 
 	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
 
-	ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
+	if (sx->filename == NULL)
+		ret = do_statx_fd(sx->dfd, sx->flags, sx->mask, sx->buffer);
+	else
+		ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
 	io_req_set_res(req, ret, 0);
 	return IOU_OK;
 }
-- 
2.43.0

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Xi Ruoyao]

On Tue, 2024-06-25 at 17:18 +0200, Mateusz Guzik wrote:
> +	if ((sx->flags & (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE)) ==
> +	    (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE) &&
> +	    vfs_empty_path(sx->dfd, path)) {
>  		sx->filename = NULL;
> -		return ret;

AT_STATX_SYNC_TYPE == AT_STATX_FORCE_SYNC | AT_STATX_DONT_SYNC but
AT_STATX_FORCE_SYNC and AT_STATX_DONT_SYNC obviously contradicts with
each other.  Thus valid uses of statx won't satisfy this condition.

And I guess the condition here should be same as the condition in
SYSCALL_DEFINE5(statx) or am I wrong?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Xi Ruoyao]

On Tue, 2024-06-25 at 17:18 +0200, Mateusz Guzik wrote:
> The newly used helper also checks for empty ("") paths.
> 
> NULL paths with any flag value other than AT_EMPTY_PATH go the usual
> route and end up with -EFAULT to retain compatibility (Rust is abusing
> calls of the sort to detect availability of statx).
> 
> This avoids path lookup code, lockref management, memory allocation and
> in case of NULL path userspace memory access (which can be quite
> expensive with SMAP on x86_64).
> 
> Benchmarked with statx(..., AT_EMPTY_PATH, ...) running on Sapphire
> Rapids, with the "" path for the first two cases and NULL for the last
> one.
> 
> Results in ops/s:
> stock:     4231237
> pre-check: 5944063 (+40%)
> NULL path: 6601619 (+11%/+56%)
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> Diffed against fs-next and assumes c050122bdbb4 ("fs: new helper
> vfs_empty_path()") from vfs.empty.path is already applied.
> 
> WARNING: io_uring remains untested (modulo compilation). I presume
> Jens has a handy way of making sure things still work. 

For non-io_uring part:

On LoongArch the time usage of 10000000 calls:

baseline
Glibc fstat: ./a.out  0.44s user 3.56s system 99% cpu 4.013 total
bare statx:  ./a.out  0.39s user 3.54s system 99% cpu 3.927 total

patched
Glibc fstat: ./a.out  0.49s user 1.34s system 99% cpu 1.841 total
bare statx:  ./a.out  0.42s user 1.32s system 99% cpu 1.748 total
statx NULL:  ./a.out  0.44s user 1.29s system 99% cpu 1.734 total

Tested-by: Xi Ruoyao <xry111@xry111.site>

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Christian Brauner]

On Tue, 25 Jun 2024 17:18:06 +0200, Mateusz Guzik wrote:
> The newly used helper also checks for empty ("") paths.
> 
> NULL paths with any flag value other than AT_EMPTY_PATH go the usual
> route and end up with -EFAULT to retain compatibility (Rust is abusing
> calls of the sort to detect availability of statx).
> 
> This avoids path lookup code, lockref management, memory allocation and
> in case of NULL path userspace memory access (which can be quite
> expensive with SMAP on x86_64).
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)
      https://git.kernel.org/vfs/vfs/c/33b321ac3a51

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Mateusz Guzik]

On Wed, Jun 26, 2024 at 4:59 AM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Tue, 2024-06-25 at 17:18 +0200, Mateusz Guzik wrote:
> > +     if ((sx->flags & (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE)) ==
> > +         (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE) &&
> > +         vfs_empty_path(sx->dfd, path)) {
> >               sx->filename = NULL;
> > -             return ret;
>
> AT_STATX_SYNC_TYPE == AT_STATX_FORCE_SYNC | AT_STATX_DONT_SYNC but
> AT_STATX_FORCE_SYNC and AT_STATX_DONT_SYNC obviously contradicts with
> each other.  Thus valid uses of statx won't satisfy this condition.
>

I don't know wtf I was thinking, this is indeed bogus.

> And I guess the condition here should be same as the condition in
> SYSCALL_DEFINE5(statx) or am I wrong?
>

That I disagree with. The AUTOMOUNT thing is a glibc-local problem for
fstatat. Unless if you mean the if should be of similar sort modulo
the flag. :)

I am going to fix this up and write a io_uring testcase, then submit a
v4. Maybe today or tomorrow.

> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University



-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Christian Brauner]

On Wed, Jun 26, 2024 at 03:39:47PM GMT, Mateusz Guzik wrote:
> On Wed, Jun 26, 2024 at 4:59 AM Xi Ruoyao <xry111@xry111.site> wrote:
> >
> > On Tue, 2024-06-25 at 17:18 +0200, Mateusz Guzik wrote:
> > > +     if ((sx->flags & (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE)) ==
> > > +         (AT_EMPTY_PATH | AT_STATX_SYNC_TYPE) &&
> > > +         vfs_empty_path(sx->dfd, path)) {
> > >               sx->filename = NULL;
> > > -             return ret;
> >
> > AT_STATX_SYNC_TYPE == AT_STATX_FORCE_SYNC | AT_STATX_DONT_SYNC but
> > AT_STATX_FORCE_SYNC and AT_STATX_DONT_SYNC obviously contradicts with
> > each other.  Thus valid uses of statx won't satisfy this condition.
> >
> 
> I don't know wtf I was thinking, this is indeed bogus.
> 
> > And I guess the condition here should be same as the condition in
> > SYSCALL_DEFINE5(statx) or am I wrong?
> >
> 
> That I disagree with. The AUTOMOUNT thing is a glibc-local problem for
> fstatat. Unless if you mean the if should be of similar sort modulo
> the flag. :)
> 
> I am going to fix this up and write a io_uring testcase, then submit a
> v4. Maybe today or tomorrow.

Fwiw, I had already dropped the io_uring bits when I pushed to
#vfs.mount. So just put that as a separate patch on top.

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Jens Axboe]

On 6/26/24 7:39 AM, Mateusz Guzik wrote:
> I am going to fix this up and write a io_uring testcase, then submit a
> v4. Maybe today or tomorrow.

Sounds good, on both incremental and test case. I can just stage it
separately in a branch with Christian's branch pulled in.

Thanks!

-- 
Jens Axboe

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Xi Ruoyao]

On Tue, 2024-06-25 at 17:18 +0200, Mateusz Guzik wrote:
> The newly used helper also checks for empty ("") paths.
> 
> NULL paths with any flag value other than AT_EMPTY_PATH go the usual
> route and end up with -EFAULT to retain compatibility (Rust is abusing
> calls of the sort to detect availability of statx).
> 
> This avoids path lookup code, lockref management, memory allocation
> and
> in case of NULL path userspace memory access (which can be quite
> expensive with SMAP on x86_64).
> 
> Benchmarked with statx(..., AT_EMPTY_PATH, ...) running on Sapphire
> Rapids, with the "" path for the first two cases and NULL for the last
> one.
> 
> Results in ops/s:
> stock:     4231237
> pre-check: 5944063 (+40%)
> NULL path: 6601619 (+11%/+56%)
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Hi Mateusz and Christian,

There's a special case, AT_FDCWD + NULL + AT_EMPTY_PATH, still resulting
EFAULT, while AT_FDCWD + "" + AT_EMPTY_PATH is OK (returning the stat of
current directory).

I know allowing NULL with AT_FDCWD won't produce any performance gain,
but it seems the difference would make the document of the API more
nasty.

So is it acceptable to make the kernel "hide" this difference, i.e.
accept AT_FDCWD + NULL + AT_EMPTY_PATH as-is AT_FDCWD + "" +
AT_EMPTY_PATH?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Mateusz Guzik]

On Wed, Sep 25, 2024 at 4:30 PM Xi Ruoyao <xry111@xry111.site> wrote:
> There's a special case, AT_FDCWD + NULL + AT_EMPTY_PATH, still resulting
> EFAULT, while AT_FDCWD + "" + AT_EMPTY_PATH is OK (returning the stat of
> current directory).
>
> I know allowing NULL with AT_FDCWD won't produce any performance gain,
> but it seems the difference would make the document of the API more
> nasty.
>
> So is it acceptable to make the kernel "hide" this difference, i.e.
> accept AT_FDCWD + NULL + AT_EMPTY_PATH as-is AT_FDCWD + "" +
> AT_EMPTY_PATH?
>

huh, that indeed makes sense to add. kind of weird this was not sorted
out at the time, but i'm not going to pointer a finger at myself :) so
ACK from me as far as the idea goes

I presume you can do the honors? :)

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH v3] vfs: support statx(..., NULL, AT_EMPTY_PATH, ...)

[Xi Ruoyao]

On Wed, 2024-09-25 at 17:56 +0200, Mateusz Guzik wrote:
> On Wed, Sep 25, 2024 at 4:30 PM Xi Ruoyao <xry111@xry111.site> wrote:
> > There's a special case, AT_FDCWD + NULL + AT_EMPTY_PATH, still resulting
> > EFAULT, while AT_FDCWD + "" + AT_EMPTY_PATH is OK (returning the stat of
> > current directory).
> > 
> > I know allowing NULL with AT_FDCWD won't produce any performance gain,
> > but it seems the difference would make the document of the API more
> > nasty.
> > 
> > So is it acceptable to make the kernel "hide" this difference, i.e.
> > accept AT_FDCWD + NULL + AT_EMPTY_PATH as-is AT_FDCWD + "" +
> > AT_EMPTY_PATH?
> > 
> 
> huh, that indeed makes sense to add. kind of weird this was not sorted
> out at the time, but i'm not going to pointer a finger at myself :) so
> ACK from me as far as the idea goes
> 
> I presume you can do the honors? :)

Should I use a Fixes: tag in the commit then? (I.e. should it be
backported to 6.11?)


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University