Re: [PATCH] fs: switch f_iocb_flags and f_version

[Christian Brauner <brauner@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Thu, Aug 22, 2024 at 10:17:37AM GMT, Jens Axboe wrote:
> On 8/22/24 9:10 AM, Christian Brauner wrote:
> >> Do we want to add a comment to this effect? I know it's obvious from
> >> sharing with f_task_work, but...
> > 
> > I'll add one.
> 
> Sounds good. You can add my:
> 
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> 
> as well, forgot to mention that in the original reply.

I think we can deliver 192 bytes aka 3 cachelines.
Afaict we can move struct file_ra_state into the union instead of
f_version. See the appended patch I'm testing now. If that works then
we're down by 40 bytes this cycle.

[-- Attachment #2: 0001-fs-switch-f_iocb_flags-and-f_ra.patch --]
[-- Type: text/x-diff, Size: 1652 bytes --]

From 51d5327717b370041733af2f3c6ea3cd75d793e2 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Thu, 22 Aug 2024 16:14:46 +0200
Subject: [PATCH] fs: switch f_iocb_flags and f_ra

Now that we shrank struct file by 24 bytes we still have a 4 byte hole.
If we move struct file_ra_state into the union and f_iocb_flags out of
the union we close that whole and bring down struct file to 192 bytes.
Which means struct file is 3 cachelines and we managed to shrink it by
40 bytes this cycle.

I've tried to audit all codepaths that use f_ra and none of them seem to
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>
---
 include/linux/fs.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7eb4f706d59f..6c19f87ea615 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -998,9 +998,9 @@ 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;
+		/* Invalid after last fput(). */
+		struct file_ra_state	f_ra;
 	};
-
 	/*
 	 * Protects f_ep, f_flags.
 	 * Must not be taken from IRQ context.
@@ -1011,9 +1011,9 @@ 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;
 	struct path		f_path;
 	struct inode		*f_inode;	/* cached value */
 	const struct file_operations	*f_op;
-- 
2.43.0