Re: [PATCH 04/19] struct fd: representation change

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 7, 2024 at 5:00 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
>         The absolute majority of instances comes from fdget() and its
> relatives; the underlying primitives actually return a struct file
> reference and a couple of flags encoded into an unsigned long - the lower
> two bits of file address are always zero, so we can stash the flags
> into those.  On the way out we use __to_fd() to unpack that unsigned
> long into struct fd.
>
>         Let's use that representation for struct fd itself - make it
> a structure with a single unsigned long member (.word), with the value
> equal either to (unsigned long)p | flags, p being an address of some
> struct file instance, or to 0 for an empty fd.
>
>         Note that we never used a struct fd instance with NULL ->file
> and non-zero ->flags; the emptiness had been checked as (!f.file) and
> we expected e.g. fdput(empty) to be a no-op.  With new representation
> we can use (!f.word) for emptiness check; that is enough for compiler
> to figure out that (f.word & FDPUT_FPUT) will be false and that fdput(f)
> will be a no-op in such case.
>
>         For now the new predicate (fd_empty(f)) has no users; all the
> existing checks have form (!fd_file(f)).  We will convert to fd_empty()
> use later; here we only define it (and tell the compiler that it's
> unlikely to return true).
>
>         This commit only deals with representation change; there will
> be followups.
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/infiniband/core/uverbs_cmd.c |  2 +-
>  fs/overlayfs/file.c                  | 28 +++++++++++++++-------------
>  fs/xfs/xfs_handle.c                  |  2 +-
>  include/linux/file.h                 | 22 ++++++++++++++++------
>  kernel/events/core.c                 |  2 +-
>  net/socket.c                         |  2 +-
>  6 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 03ea3afcb31f..efe3cc3debba 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -572,7 +572,7 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
>         struct inode                   *inode = NULL;
>         int                             new_xrcd = 0;
>         struct ib_device *ib_dev;
> -       struct fd f = {};
> +       struct fd f = EMPTY_FD;
>         int ret;
>
>         ret = uverbs_request(attrs, &cmd, sizeof(cmd));
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index c4963d0c5549..458299873780 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -93,11 +93,11 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
>                                bool allow_meta)
>  {
>         struct dentry *dentry = file_dentry(file);
> +       struct file *private = file->private_data;

In future versions please rename s/private/realfile.

Thanks,
Amir.

>         struct path realpath;
>         int err;
>
> -       real->flags = 0;
> -       real->file = file->private_data;
> +       real->word = (unsigned long)private;
>
>         if (allow_meta) {
>                 ovl_path_real(dentry, &realpath);
> @@ -113,16 +113,17 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
>                 return -EIO;
>
>         /* Has it been copied up since we'd opened it? */
> -       if (unlikely(file_inode(real->file) != d_inode(realpath.dentry))) {
> -               real->flags = FDPUT_FPUT;
> -               real->file = ovl_open_realfile(file, &realpath);
> -
> -               return PTR_ERR_OR_ZERO(real->file);
> +       if (unlikely(file_inode(private) != d_inode(realpath.dentry))) {
> +               struct file *f = ovl_open_realfile(file, &realpath);
> +               if (IS_ERR(f))
> +                       return PTR_ERR(f);
> +               real->word = (unsigned long)ovl_open_realfile(file, &realpath) | FDPUT_FPUT;
> +               return 0;
>         }
>
>         /* Did the flags change since open? */
> -       if (unlikely((file->f_flags ^ real->file->f_flags) & ~OVL_OPEN_FLAGS))
> -               return ovl_change_flags(real->file, file->f_flags);
> +       if (unlikely((file->f_flags ^ private->f_flags) & ~OVL_OPEN_FLAGS))
> +               return ovl_change_flags(private, file->f_flags);
>
>         return 0;
>  }
> @@ -130,10 +131,11 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
>  static int ovl_real_fdget(const struct file *file, struct fd *real)
>  {
>         if (d_is_dir(file_dentry(file))) {
> -               real->flags = 0;
> -               real->file = ovl_dir_real_file(file, false);
> -
> -               return PTR_ERR_OR_ZERO(real->file);
> +               struct file *f = ovl_dir_real_file(file, false);
> +               if (IS_ERR(f))
> +                       return PTR_ERR(f);
> +               real->word = (unsigned long)f;
> +               return 0;
>         }
>
>         return ovl_real_fdget_meta(file, real, false);
> diff --git a/fs/xfs/xfs_handle.c b/fs/xfs/xfs_handle.c
> index 445a2daff233..bb250f4246b3 100644
> --- a/fs/xfs/xfs_handle.c
> +++ b/fs/xfs/xfs_handle.c
> @@ -86,7 +86,7 @@ xfs_find_handle(
>         int                     hsize;
>         xfs_handle_t            handle;
>         struct inode            *inode;
> -       struct fd               f = {NULL};
> +       struct fd               f = EMPTY_FD;
>         struct path             path;
>         int                     error;
>         struct xfs_inode        *ip;
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 0964408727a7..39eb10a1bbfc 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -35,18 +35,28 @@ static inline void fput_light(struct file *file, int fput_needed)
>                 fput(file);
>  }
>
> +/* either a reference to struct file + flags
> + * (cloned vs. borrowed, pos locked), with
> + * flags stored in lower bits of value,
> + * or empty (represented by 0).
> + */
>  struct fd {
> -       struct file *file;
> -       unsigned int flags;
> +       unsigned long word;
>  };
>  #define FDPUT_FPUT       1
>  #define FDPUT_POS_UNLOCK 2
>
> -#define fd_file(f) ((f).file)
> +#define fd_file(f) ((struct file *)((f).word & ~3))
> +static inline bool fd_empty(struct fd f)
> +{
> +       return unlikely(!f.word);
> +}
> +
> +#define EMPTY_FD (struct fd){0}
>
>  static inline void fdput(struct fd fd)
>  {
> -       if (fd.flags & FDPUT_FPUT)
> +       if (fd.word & FDPUT_FPUT)
>                 fput(fd_file(fd));
>  }
>
> @@ -60,7 +70,7 @@ extern void __f_unlock_pos(struct file *);
>
>  static inline struct fd __to_fd(unsigned long v)
>  {
> -       return (struct fd){(struct file *)(v & ~3),v & 3};
> +       return (struct fd){v};
>  }
>
>  static inline struct fd fdget(unsigned int fd)
> @@ -80,7 +90,7 @@ static inline struct fd fdget_pos(int fd)
>
>  static inline void fdput_pos(struct fd f)
>  {
> -       if (f.flags & FDPUT_POS_UNLOCK)
> +       if (f.word & FDPUT_POS_UNLOCK)
>                 __f_unlock_pos(fd_file(f));
>         fdput(f);
>  }
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7acf44111a6e..fd4621cd9c23 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12438,7 +12438,7 @@ SYSCALL_DEFINE5(perf_event_open,
>         struct perf_event_attr attr;
>         struct perf_event_context *ctx;
>         struct file *event_file = NULL;
> -       struct fd group = {NULL, 0};
> +       struct fd group = EMPTY_FD;
>         struct task_struct *task = NULL;
>         struct pmu *pmu;
>         int event_fd;
> diff --git a/net/socket.c b/net/socket.c
> index 50b074f52147..a2c509363d4d 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -559,7 +559,7 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
>         if (fd_file(f)) {
>                 sock = sock_from_file(fd_file(f));
>                 if (likely(sock)) {
> -                       *fput_needed = f.flags & FDPUT_FPUT;
> +                       *fput_needed = f.word & FDPUT_FPUT;
>                         return sock;
>                 }
>                 *err = -ENOTSOCK;
> --
> 2.39.2
>
>





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux