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

[Huacai Chen <chenhuacai@kernel.org>]

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
>
>