| Message ID | 20240625110029.606032-3-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | statx NULL path support | expand |
On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote: > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) Could it be if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename)) instead? When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at least Glibc developers think it's needed: #if FSTATAT_USE_STATX static inline int fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, int flag) { /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also 64-bit time_t support is done through statx syscall. */ struct statx tmp; int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, STATX_BASIC_STATS, &tmp); so "flags == AT_EMPTY_PATH" won't be true if Glibc implements fstatat and fstat via statx (on LoongArch and 32-bit platforms). I was just surprised when I saw a 100%+ improve for statx("", AT_EMPTY_PATH) but not stat on the Loongson machine. > + return do_statx_fd(dfd, flags, mask, buffer); > + > name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
On Tue, 2024-06-25 at 21:24 +0800, Xi Ruoyao wrote: > On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote: > > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) > > Could it be > > if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename)) > > instead? > > When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at > least Glibc developers think it's needed: /* snip */ > I was just surprised when I saw a 100%+ improve for statx("", > AT_EMPTY_PATH) but not stat on the Loongson machine. ^^^^ fstat I cannot type :(
On Tue, Jun 25, 2024 at 3:24 PM Xi Ruoyao <xry111@xry111.site> wrote: > > On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote: > > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) > > Could it be > > if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename)) > > instead? > > When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at > least Glibc developers think it's needed: > > #if FSTATAT_USE_STATX > > static inline int > fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, > int flag) > { > /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also > 64-bit time_t support is done through statx syscall. */ > struct statx tmp; > int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, > STATX_BASIC_STATS, &tmp); > > so "flags == AT_EMPTY_PATH" won't be true if Glibc implements fstatat > and fstat via statx (on LoongArch and 32-bit platforms). > > I was just surprised when I saw a 100%+ improve for statx("", > AT_EMPTY_PATH) but not stat on the Loongson machine. > It can't be like that specifically because we still need to catch bogus AT flags. I'm going to poke a little bit and send a v2, thanks.
On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> wrote: > > The newly used helper also checks for 0-sized buffers. > > 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). > > statx with AT_EMPTY_PATH paired with "" or NULL argument as appropriate > issued on Sapphire Rapids (ops/s): > stock: 4231237 > 0-check: 5944063 (+40%) > NULL path: 6601619 (+11%/+56%) > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Hi, Ruoyao, I'm a bit confused. Ii this patch a replacement of your recent patch? Huacai > --- > fs/internal.h | 2 ++ > fs/stat.c | 90 ++++++++++++++++++++++++++++++++++-------------- > io_uring/statx.c | 23 +++++++------ > 3 files changed, 80 insertions(+), 35 deletions(-) > > diff --git a/fs/internal.h b/fs/internal.h > index 1caa6a8f666f..0a018ebcaf49 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -244,6 +244,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 106684034fdb..1214826f3a36 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; > } > > @@ -677,6 +691,29 @@ 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. > @@ -696,6 +733,9 @@ SYSCALL_DEFINE5(statx, > int ret; > struct filename *name; > > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) > + return do_statx_fd(dfd, flags, mask, buffer); > + > name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL); > ret = do_statx(dfd, name, flags, mask, buffer); > putname(name); > diff --git a/io_uring/statx.c b/io_uring/statx.c > index abb874209caa..fe967ecb1762 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,15 +37,14 @@ 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), > - NULL); > - > - if (IS_ERR(sx->filename)) { > - int ret = PTR_ERR(sx->filename); > - > - sx->filename = NULL; > - return ret; > + sx->filename = NULL; > + if (!(sx->flags == AT_EMPTY_PATH && vfs_empty_path(sx->dfd, path))) { > + filename = getname_flags(path, > + getname_statx_lookup_flags(sx->flags), > + NULL); > + if (IS_ERR(filename)) > + return PTR_ERR(filename); > + sx->filename = filename; > } > > req->flags |= REQ_F_NEED_CLEANUP; > @@ -59,7 +59,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 > >
On Tue, 2024-06-25 at 22:09 +0800, Huacai Chen wrote: > On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> > wrote: > > > > The newly used helper also checks for 0-sized buffers. > > > > 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). > > > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > > appropriate > > issued on Sapphire Rapids (ops/s): > > stock: 4231237 > > 0-check: 5944063 (+40%) > > NULL path: 6601619 (+11%/+56%) > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > Hi, Ruoyao, > > I'm a bit confused. Ii this patch a replacement of your recent patch? Yes, both Linus and Christian hates introducing a new AT_ flag for this. This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave like statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the performance issue and it's also audit-able by seccomp BPF.
diff --git a/fs/internal.h b/fs/internal.h index 1caa6a8f666f..0a018ebcaf49 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -244,6 +244,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 106684034fdb..1214826f3a36 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; } @@ -677,6 +691,29 @@ 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. @@ -696,6 +733,9 @@ SYSCALL_DEFINE5(statx, int ret; struct filename *name; + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename)) + return do_statx_fd(dfd, flags, mask, buffer); + name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL); ret = do_statx(dfd, name, flags, mask, buffer); putname(name); diff --git a/io_uring/statx.c b/io_uring/statx.c index abb874209caa..fe967ecb1762 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,15 +37,14 @@ 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), - NULL); - - if (IS_ERR(sx->filename)) { - int ret = PTR_ERR(sx->filename); - - sx->filename = NULL; - return ret; + sx->filename = NULL; + if (!(sx->flags == AT_EMPTY_PATH && vfs_empty_path(sx->dfd, path))) { + filename = getname_flags(path, + getname_statx_lookup_flags(sx->flags), + NULL); + if (IS_ERR(filename)) + return PTR_ERR(filename); + sx->filename = filename; } req->flags |= REQ_F_NEED_CLEANUP; @@ -59,7 +59,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; }
The newly used helper also checks for 0-sized buffers. 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). statx with AT_EMPTY_PATH paired with "" or NULL argument as appropriate issued on Sapphire Rapids (ops/s): stock: 4231237 0-check: 5944063 (+40%) NULL path: 6601619 (+11%/+56%) Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- fs/internal.h | 2 ++ fs/stat.c | 90 ++++++++++++++++++++++++++++++++++-------------- io_uring/statx.c | 23 +++++++------ 3 files changed, 80 insertions(+), 35 deletions(-)