[PATCH v3 0/2] pipe: don't update {a,c,m}time for anonymous pipes

[PATCH v3 0/2] pipe: don't update {a,c,m}time for anonymous pipes

[Oleg Nesterov]

OK, let me send v3 right now...

Changes: make pipeanon_fops static.

Link to v1: https://lore.kernel.org/all/20250204132153.GA20921@redhat.com/
Link to v2: https://lore.kernel.org/all/20250205161636.GA1001@redhat.com/

Oleg.
---

 fs/pipe.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 15 deletions(-)

[PATCH v3 1/2] pipe: introduce struct file_operations pipeanon_fops

[Oleg Nesterov]

So that fifos and anonymous pipes could have different f_op methods.
Preparation to simplify the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/pipe.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 94b59045ab44..2eacfde61e74 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -878,6 +878,8 @@ static const struct dentry_operations pipefs_dentry_operations = {
 	.d_dname	= pipefs_dname,
 };
 
+static const struct file_operations pipeanon_fops;
+
 static struct inode * get_pipe_inode(void)
 {
 	struct inode *inode = new_inode_pseudo(pipe_mnt->mnt_sb);
@@ -895,7 +897,7 @@ static struct inode * get_pipe_inode(void)
 	inode->i_pipe = pipe;
 	pipe->files = 2;
 	pipe->readers = pipe->writers = 1;
-	inode->i_fop = &pipefifo_fops;
+	inode->i_fop = &pipeanon_fops;
 
 	/*
 	 * Mark the inode dirty from the very beginning,
@@ -938,7 +940,7 @@ int create_pipe_files(struct file **res, int flags)
 
 	f = alloc_file_pseudo(inode, pipe_mnt, "",
 				O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT)),
-				&pipefifo_fops);
+				&pipeanon_fops);
 	if (IS_ERR(f)) {
 		free_pipe_info(inode->i_pipe);
 		iput(inode);
@@ -949,7 +951,7 @@ int create_pipe_files(struct file **res, int flags)
 	f->f_pipe = 0;
 
 	res[0] = alloc_file_clone(f, O_RDONLY | (flags & O_NONBLOCK),
-				  &pipefifo_fops);
+				  &pipeanon_fops);
 	if (IS_ERR(res[0])) {
 		put_pipe_info(inode, inode->i_pipe);
 		fput(f);
@@ -1107,8 +1109,8 @@ static void wake_up_partner(struct pipe_inode_info *pipe)
 
 static int fifo_open(struct inode *inode, struct file *filp)
 {
+	bool is_pipe = inode->i_fop == &pipeanon_fops;
 	struct pipe_inode_info *pipe;
-	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
 	int ret;
 
 	filp->f_pipe = 0;
@@ -1241,6 +1243,17 @@ const struct file_operations pipefifo_fops = {
 	.splice_write	= iter_file_splice_write,
 };
 
+static const struct file_operations pipeanon_fops = {
+	.open		= fifo_open,
+	.read_iter	= pipe_read,
+	.write_iter	= pipe_write,
+	.poll		= pipe_poll,
+	.unlocked_ioctl	= pipe_ioctl,
+	.release	= pipe_release,
+	.fasync		= pipe_fasync,
+	.splice_write	= iter_file_splice_write,
+};
+
 /*
  * Currently we rely on the pipe array holding a power-of-2 number
  * of pages. Returns 0 on error.
@@ -1388,7 +1401,9 @@ struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice)
 {
 	struct pipe_inode_info *pipe = file->private_data;
 
-	if (file->f_op != &pipefifo_fops || !pipe)
+	if (!pipe)
+		return NULL;
+	if (file->f_op != &pipefifo_fops && file->f_op != &pipeanon_fops)
 		return NULL;
 	if (for_splice && pipe_has_watch_queue(pipe))
 		return NULL;
-- 
2.25.1.362.g51ebf55

[PATCH v3 2/2] pipe: don't update {a,c,m}time for anonymous pipes

[Oleg Nesterov]

These numbers are visible in fstat() but hopefully nobody uses this
information and file_accessed/file_update_time are not that cheap.
Stupid test-case:

	#include <stdio.h>
	#include <stdlib.h>
	#include <unistd.h>
	#include <assert.h>
	#include <sys/ioctl.h>
	#include <sys/time.h>

	static char buf[17 * 4096];
	static struct timeval TW, TR;

	int wr(int fd, int size)
	{
		int c, r;
		struct timeval t0, t1;

		gettimeofday(&t0, NULL);
		for (c = 0; (r = write(fd, buf, size)) > 0; c += r);
		gettimeofday(&t1, NULL);
		timeradd(&TW, &t1, &TW);
		timersub(&TW, &t0, &TW);

		return c;
	}

	int rd(int fd, int size)
	{
		int c, r;
		struct timeval t0, t1;

		gettimeofday(&t0, NULL);
		for (c = 0; (r = read(fd, buf, size)) > 0; c += r);
		gettimeofday(&t1, NULL);
		timeradd(&TR, &t1, &TR);
		timersub(&TR, &t0, &TR);

		return c;
	}

	int main(int argc, const char *argv[])
	{
		int fd[2], nb = 1, loop, size;

		assert(argc == 3);
		loop = atoi(argv[1]);
		size = atoi(argv[2]);

		assert(pipe(fd) == 0);
		assert(ioctl(fd[0], FIONBIO, &nb) == 0);
		assert(ioctl(fd[1], FIONBIO, &nb) == 0);

		assert(size <= sizeof(buf));
		while (loop--)
			assert(wr(fd[1], size) == rd(fd[0], size));

		struct timeval tt;
		timeradd(&TW, &TR, &tt);
		printf("TW = %lu.%03lu TR = %lu.%03lu TT = %lu.%03lu\n",
			TW.tv_sec, TW.tv_usec/1000,
			TR.tv_sec, TR.tv_usec/1000,
			tt.tv_sec, tt.tv_usec/1000);

		return 0;
	}

Before:
	# for i in 1 2 3; do /host/tmp/test 10000 100; done
	TW = 8.047 TR = 5.845 TT = 13.893
	TW = 8.091 TR = 5.872 TT = 13.963
	TW = 8.083 TR = 5.885 TT = 13.969
After:
	# for i in 1 2 3; do /host/tmp/test 10000 100; done
	TW = 4.752 TR = 4.664 TT = 9.416
	TW = 4.684 TR = 4.608 TT = 9.293
	TW = 4.736 TR = 4.652 TT = 9.388

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/pipe.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 2eacfde61e74..2ae75adfba64 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -248,7 +248,7 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
 }
 
 static ssize_t
-pipe_read(struct kiocb *iocb, struct iov_iter *to)
+anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
 {
 	size_t total_len = iov_iter_count(to);
 	struct file *filp = iocb->ki_filp;
@@ -404,8 +404,15 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 	if (wake_next_reader)
 		wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM);
 	kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
+	return ret;
+}
+
+static ssize_t
+fifo_pipe_read(struct kiocb *iocb, struct iov_iter *to)
+{
+	int ret = anon_pipe_read(iocb, to);
 	if (ret > 0)
-		file_accessed(filp);
+		file_accessed(iocb->ki_filp);
 	return ret;
 }
 
@@ -426,7 +433,7 @@ static inline bool pipe_writable(const struct pipe_inode_info *pipe)
 }
 
 static ssize_t
-pipe_write(struct kiocb *iocb, struct iov_iter *from)
+anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *filp = iocb->ki_filp;
 	struct pipe_inode_info *pipe = filp->private_data;
@@ -604,11 +611,21 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 	kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
 	if (wake_next_writer)
 		wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
-	if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) {
-		int err = file_update_time(filp);
-		if (err)
-			ret = err;
-		sb_end_write(file_inode(filp)->i_sb);
+	return ret;
+}
+
+static ssize_t
+fifo_pipe_write(struct kiocb *iocb, struct iov_iter *from)
+{
+	int ret = anon_pipe_write(iocb, from);
+	if (ret > 0) {
+		struct file *filp = iocb->ki_filp;
+		if (sb_start_write_trylock(file_inode(filp)->i_sb)) {
+			int err = file_update_time(filp);
+			if (err)
+				ret = err;
+			sb_end_write(file_inode(filp)->i_sb);
+		}
 	}
 	return ret;
 }
@@ -1234,8 +1251,8 @@ static int fifo_open(struct inode *inode, struct file *filp)
 
 const struct file_operations pipefifo_fops = {
 	.open		= fifo_open,
-	.read_iter	= pipe_read,
-	.write_iter	= pipe_write,
+	.read_iter	= fifo_pipe_read,
+	.write_iter	= fifo_pipe_write,
 	.poll		= pipe_poll,
 	.unlocked_ioctl	= pipe_ioctl,
 	.release	= pipe_release,
@@ -1245,8 +1262,8 @@ const struct file_operations pipefifo_fops = {
 
 static const struct file_operations pipeanon_fops = {
 	.open		= fifo_open,
-	.read_iter	= pipe_read,
-	.write_iter	= pipe_write,
+	.read_iter	= anon_pipe_read,
+	.write_iter	= anon_pipe_write,
 	.poll		= pipe_poll,
 	.unlocked_ioctl	= pipe_ioctl,
 	.release	= pipe_release,
-- 
2.25.1.362.g51ebf55

Re: [PATCH v3 0/2] pipe: don't update {a,c,m}time for anonymous pipes

[K Prateek Nayak]

Hello Oleg,

On 2/5/2025 11:47 PM, Oleg Nesterov wrote:
> OK, let me send v3 right now...

Tested this series with sched-messaging on my 3rd Generation EPYC
system (2 x64c/128T, boost on, C2 disabled) and I see slight
improvements:

   ==================================================================
   Test          : sched-messaging
   Units         : Normalized time in seconds
   Interpretation: Lower is better
   Statistic     : AMean
   ==================================================================
   Case:      upstream[pct imp](CV)    skip_{a,c,m}_time[pct imp](CV)
    1-groups     1.00 [ -0.00]( 9.88)     1.05 [ -5.16]( 7.19) *
    2-groups     1.00 [ -0.00]( 3.49)     0.97 [  2.70]( 3.54)
    4-groups     1.00 [ -0.00]( 1.22)     0.97 [  2.70]( 2.78)
    8-groups     1.00 [ -0.00]( 0.80)     0.99 [  0.94]( 1.04)
   16-groups     1.00 [ -0.00]( 1.40)     0.98 [  2.43]( 1.02)
   
   * Disregard these data points due to large run to run variation

Feel free to add:

Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>

I'll go test the pipe_{read,write}() cleanup you had posted on the
other thread.

-- 
Thanks and Regards,
Prateek

> 
> Changes: make pipeanon_fops static.
> 
> Link to v1: https://lore.kernel.org/all/20250204132153.GA20921@redhat.com/
> Link to v2: https://lore.kernel.org/all/20250205161636.GA1001@redhat.com/
> 
> Oleg.
> ---
> 
>   fs/pipe.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 47 insertions(+), 15 deletions(-)
> 

Re: [PATCH v3 0/2] pipe: don't update {a,c,m}time for anonymous pipes

[Christian Brauner]

On Wed, 05 Feb 2025 19:17:16 +0100, Oleg Nesterov wrote:
> OK, let me send v3 right now...
> 
> Changes: make pipeanon_fops static.
> 
> Link to v1: https://lore.kernel.org/all/20250204132153.GA20921@redhat.com/
> Link to v2: https://lore.kernel.org/all/20250205161636.GA1001@redhat.com/
> 
> [...]

Applied to the vfs-6.15.pipe branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.pipe branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.pipe

[1/2] pipe: introduce struct file_operations pipeanon_fops
      https://git.kernel.org/vfs/vfs/c/262b2fa99cbe
[2/2] pipe: don't update {a,c,m}time for anonymous pipes
      https://git.kernel.org/vfs/vfs/c/f017b0a4951f

Re: [PATCH v3 0/2] pipe: don't update {a,c,m}time for anonymous pipes

[Jeff Layton]

On Wed, 2025-02-05 at 19:17 +0100, Oleg Nesterov wrote:
> OK, let me send v3 right now...
> 
> Changes: make pipeanon_fops static.
> 
> Link to v1: https://lore.kernel.org/all/20250204132153.GA20921@redhat.com/
> Link to v2: https://lore.kernel.org/all/20250205161636.GA1001@redhat.com/
> 
> Oleg.
> ---
> 
>  fs/pipe.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 15 deletions(-)
> 

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