Re: [PATCH] fs: switch f_iocb_flags and f_version

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2024-08-22 at 16:14 +0200, Christian Brauner wrote:
> Now that we shrank struct file by 24 bytes we still have a 4 byte hole.
> Move f_version into the union and f_iocb_flags out of the union to fill
> that hole and shrink struct file by another 4 bytes. This brings struct
> file to 200 bytes down from 232 bytes.
> 
> I've tried to audit all codepaths that use f_version and none of them
> rely on it in file->f_op->release() and never have since commit
> 1da177e4c3f4 ("Linux-2.6.12-rc2").
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> struct file {
>         union {
>                 struct callback_head f_task_work;        /*     0    16 */
>                 struct llist_node  f_llist;              /*     0     8 */
>                 u64                f_version;            /*     0     8 */
>         };                                               /*     0    16 */
>         spinlock_t                 f_lock;               /*    16     4 */
>         fmode_t                    f_mode;               /*    20     4 */
>         atomic_long_t              f_count;              /*    24     8 */
>         struct mutex               f_pos_lock;           /*    32    32 */
>         /* --- cacheline 1 boundary (64 bytes) --- */
>         loff_t                     f_pos;                /*    64     8 */
>         unsigned int               f_flags;              /*    72     4 */
>         unsigned int               f_iocb_flags;         /*    76     4 */
>         struct fown_struct *       f_owner;              /*    80     8 */
>         const struct cred  *       f_cred;               /*    88     8 */
>         struct file_ra_state       f_ra;                 /*    96    32 */
>         /* --- cacheline 2 boundary (128 bytes) --- */
>         struct path                f_path;               /*   128    16 */
>         struct inode *             f_inode;              /*   144     8 */
>         const struct file_operations  * f_op;            /*   152     8 */
>         void *                     f_security;           /*   160     8 */
>         void *                     private_data;         /*   168     8 */
>         struct hlist_head *        f_ep;                 /*   176     8 */
>         struct address_space *     f_mapping;            /*   184     8 */
>         /* --- cacheline 3 boundary (192 bytes) --- */
>         errseq_t                   f_wb_err;             /*   192     4 */
>         errseq_t                   f_sb_err;             /*   196     4 */
> 
>         /* size: 200, cachelines: 4, members: 20 */
>         /* last cacheline: 8 bytes */
> };
> ---
>  include/linux/fs.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7eb4f706d59f..7a2994405e8e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -998,9 +998,8 @@ struct file {
>  		struct callback_head 	f_task_work;
>  		/* fput() must use workqueue (most kernel threads). */
>  		struct llist_node	f_llist;
> -		unsigned int 		f_iocb_flags;
> +		u64			f_version;
>  	};
> -
>  	/*
>  	 * Protects f_ep, f_flags.
>  	 * Must not be taken from IRQ context.
> @@ -1011,6 +1010,7 @@ struct file {
>  	struct mutex		f_pos_lock;
>  	loff_t			f_pos;
>  	unsigned int		f_flags;
> +	unsigned int 		f_iocb_flags;
>  	struct fown_struct	*f_owner;
>  	const struct cred	*f_cred;
>  	struct file_ra_state	f_ra;
> @@ -1018,7 +1018,6 @@ struct file {
>  	struct inode		*f_inode;	/* cached value */
>  	const struct file_operations	*f_op;
>  
> -	u64			f_version;
>  #ifdef CONFIG_SECURITY
>  	void			*f_security;
>  #endif

Reviewed-by: Jeff Layton <jlayton@kernel.org>