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

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

[Oleg Nesterov]

Not sure this makes sense, but after 1/2 we can also split fifo_open()
and pipe_poll() to factor out the wait_for_partner/wake_up_partner logic.

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

Oleg.
---

 fs/internal.h |  1 +
 fs/pipe.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 46 insertions(+), 15 deletions(-)

[PATCH v2 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/internal.h |  1 +
 fs/pipe.c     | 23 ++++++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index e7f02ae1e098..dfdc2b2cf2f5 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -229,6 +229,7 @@ extern void d_genocide(struct dentry *);
  * pipe.c
  */
 extern const struct file_operations pipefifo_fops;
+extern const struct file_operations pipeanon_fops;
 
 /*
  * fs_pin.c
diff --git a/fs/pipe.c b/fs/pipe.c
index 94b59045ab44..b05cded28d9b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -895,7 +895,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 +938,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 +949,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 +1107,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 +1241,17 @@ const struct file_operations pipefifo_fops = {
 	.splice_write	= iter_file_splice_write,
 };
 
+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 +1399,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

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

[Linus Torvalds]

On Wed, 5 Feb 2025 at 07:34, Oleg Nesterov <oleg@redhat.com> wrote:
>
> So that fifos and anonymous pipes could have different f_op methods.
> Preparation to simplify the next patch.

Looks good, except:

> +++ b/fs/internal.h
>  extern const struct file_operations pipefifo_fops;
> +extern const struct file_operations pipeanon_fops;

I think this should just be 'static' to inside fs/pipe.c, no?

The only reason pipefifo_fops is in that header is because it's used
for named pipes outside the pipe code, in init_special_inode().

So I don't think pipeanon_fops should be exposed anywhere outside fs/pipe.c.

            Linus

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

[Oleg Nesterov]

On 02/05, Linus Torvalds wrote:
>
> On Wed, 5 Feb 2025 at 07:34, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > So that fifos and anonymous pipes could have different f_op methods.
> > Preparation to simplify the next patch.
>
> Looks good, except:
>
> > +++ b/fs/internal.h
> >  extern const struct file_operations pipefifo_fops;
> > +extern const struct file_operations pipeanon_fops;
>
> I think this should just be 'static' to inside fs/pipe.c, no?

I swear, this is what I did initially ;)

But then for some reason I thought someone would ask me to export
pipeanon_fops along with pipefifo_fops for consistency.

OK, I will wait for other reviews and send V3.

Oleg.

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

[Christian Brauner]

On Wed, Feb 05, 2025 at 05:16:37PM +0100, Oleg Nesterov wrote:
> On 02/05, Linus Torvalds wrote:
> >
> > On Wed, 5 Feb 2025 at 07:34, Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > So that fifos and anonymous pipes could have different f_op methods.
> > > Preparation to simplify the next patch.
> >
> > Looks good, except:
> >
> > > +++ b/fs/internal.h
> > >  extern const struct file_operations pipefifo_fops;
> > > +extern const struct file_operations pipeanon_fops;
> >
> > I think this should just be 'static' to inside fs/pipe.c, no?
> 
> I swear, this is what I did initially ;)
> 
> But then for some reason I thought someone would ask me to export
> pipeanon_fops along with pipefifo_fops for consistency.
> 
> OK, I will wait for other reviews and send V3.

It's fine. Minor things like this I can just fix myself when pulling
this. There's no need to generate more mail traffic for this.

[PATCH v2 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 b05cded28d9b..e772637c622c 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;
 }
@@ -1232,8 +1249,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,
@@ -1243,8 +1260,8 @@ const struct file_operations pipefifo_fops = {
 
 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