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

[PATCH] 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 | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 94b59045ab44..baaa8c0817f1 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -247,6 +247,11 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
 	return tail;
 }
 
+static inline bool is_pipe_inode(struct inode *inode)
+{
+	return inode->i_sb->s_magic == PIPEFS_MAGIC;
+}
+
 static ssize_t
 pipe_read(struct kiocb *iocb, struct iov_iter *to)
 {
@@ -404,7 +409,7 @@ 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);
-	if (ret > 0)
+	if (ret > 0 && !is_pipe_inode(file_inode(filp)))
 		file_accessed(filp);
 	return ret;
 }
@@ -604,11 +609,13 @@ 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);
+	if (ret > 0 && !is_pipe_inode(file_inode(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;
 }
@@ -1108,7 +1115,7 @@ static void wake_up_partner(struct pipe_inode_info *pipe)
 static int fifo_open(struct inode *inode, struct file *filp)
 {
 	struct pipe_inode_info *pipe;
-	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
+	bool is_pipe = is_pipe_inode(inode);
 	int ret;
 
 	filp->f_pipe = 0;
-- 
2.25.1.362.g51ebf55

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

[Mateusz Guzik]

On Tue, Feb 4, 2025 at 2:22 PM Oleg Nesterov <oleg@redhat.com> wrote:
> These numbers are visible in fstat() but hopefully nobody uses this
> information and file_accessed/file_update_time are not that cheap.

I'll note majority of the problem most likely stems from
mnt_get_write_access rolling with:
        preempt_disable();
        mnt_inc_writers(mnt);
        /*
         * The store to mnt_inc_writers must be visible before we pass
         * MNT_WRITE_HOLD loop below, so that the slowpath can see our
         * incremented count after it has set MNT_WRITE_HOLD.
         */
        smp_mb();

Unfortunately as is the MNT_WRITE_HOLD thing gets set under a
spinlock, so it may be this will be hard to rework in the same vein in
which percpu rwsems are operating, but perhaps someone(tm) may take an
honest shot?

Would be of benefit everywhere and possibly obsolete this patch.

> +       if (ret > 0 && !is_pipe_inode(file_inode(filp))) {

Total nit in case there is a v2:

ret is typically > 0 and most of the time this is going to be an
anonymous pipe, so I would swap these conditions around.

A not nit is that "is_pipe_inode" is imo misleading -- a named pipe
inode is also a pipe inode. How about "is_anon_pipe"?

-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Oleg Nesterov]

On 02/04, Mateusz Guzik wrote:
>
> > +       if (ret > 0 && !is_pipe_inode(file_inode(filp))) {
>
> Total nit in case there is a v2:
>
> ret is typically > 0 and most of the time this is going to be an
> anonymous pipe, so I would swap these conditions around.
>
> A not nit is that "is_pipe_inode" is imo misleading -- a named pipe
> inode is also a pipe inode. How about "is_anon_pipe"?

OK, I'll wait for other reviews and send v2 with the changes you suggest.

Oleg.

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

[Jeff Layton]

On Tue, 2025-02-04 at 14:21 +0100, Oleg Nesterov wrote:
> 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 | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 94b59045ab44..baaa8c0817f1 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -247,6 +247,11 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
>  	return tail;
>  }
>  
> +static inline bool is_pipe_inode(struct inode *inode)
> +{
> +	return inode->i_sb->s_magic == PIPEFS_MAGIC;
> +}
> +
>  static ssize_t
>  pipe_read(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -404,7 +409,7 @@ 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);
> -	if (ret > 0)
> +	if (ret > 0 && !is_pipe_inode(file_inode(filp)))

pipe_read and pipe_write are the read_iter / write_iter ops for pipefs
inodes. Is there ever a situation where is_pipe_inode() will be false
here?

>  		file_accessed(filp);
>  	return ret;
>  }
> @@ -604,11 +609,13 @@ 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);
> +	if (ret > 0 && !is_pipe_inode(file_inode(filp))) {

Ditto.

> +		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;
>  }
> @@ -1108,7 +1115,7 @@ static void wake_up_partner(struct pipe_inode_info *pipe)
>  static int fifo_open(struct inode *inode, struct file *filp)
>  {
>  	struct pipe_inode_info *pipe;
> -	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
> +	bool is_pipe = is_pipe_inode(inode);

Same here: In what case is is_pipe() ever false here?

>  	int ret;
>  
>  	filp->f_pipe = 0;

-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

On Tue, 2025-02-04 at 14:21 +0100, Oleg Nesterov wrote:
> 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 | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 94b59045ab44..baaa8c0817f1 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -247,6 +247,11 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
>  	return tail;
>  }
>  
> +static inline bool is_pipe_inode(struct inode *inode)
> +{
> +	return inode->i_sb->s_magic == PIPEFS_MAGIC;
> +}
> +
>  static ssize_t
>  pipe_read(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -404,7 +409,7 @@ 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);
> -	if (ret > 0)
> +	if (ret > 0 && !is_pipe_inode(file_inode(filp)))
>  		file_accessed(filp);
>  	return ret;
>  }
> @@ -604,11 +609,13 @@ 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);
> +	if (ret > 0 && !is_pipe_inode(file_inode(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;
>  }
> @@ -1108,7 +1115,7 @@ static void wake_up_partner(struct pipe_inode_info *pipe)
>  static int fifo_open(struct inode *inode, struct file *filp)
>  {
>  	struct pipe_inode_info *pipe;
> -	bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
> +	bool is_pipe = is_pipe_inode(inode);
>  	int ret;
>  
>  	filp->f_pipe = 0;

Oh _anonymous_ pipes. You can disregard my earlier questions:

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

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

[Linus Torvalds]

On Tue, 4 Feb 2025 at 05:22, Oleg Nesterov <oleg@redhat.com> wrote:
>
> @@ -404,7 +409,7 @@ 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);
> -       if (ret > 0)
> +       if (ret > 0 && !is_pipe_inode(file_inode(filp)))
>                 file_accessed(filp);
>         return ret;
>  }

So I really don't kike that "is_pipe_inode()" thing.

Yes, it has superficial naming problems (it should be about
*anonymous* pipes, and that should be reflected in the name), but I
think the deeper problem is that I think this should just be a
different set of read/write functions entirely.

IOW, we should have "anon_pipe_read()" and "anon_pipe_write()", and
then the named pipes just do

    int ret = anon_pipe_read(...);
    if (ret > 0)
        file_accessed(filp);
    return ret;

instead. With no tests for "what kind of pipe is this" (same for the
write side, of course)

               Linus

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

[Oleg Nesterov]

On 02/04, Linus Torvalds wrote:
>
> IOW, we should have "anon_pipe_read()" and "anon_pipe_write()", and
> then the named pipes just do

OK, will do in v2.

Oleg.

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

[Christian Brauner]

On Tue, Feb 04, 2025 at 03:21:18PM +0100, Mateusz Guzik wrote:
> On Tue, Feb 4, 2025 at 2:22 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > These numbers are visible in fstat() but hopefully nobody uses this
> > information and file_accessed/file_update_time are not that cheap.
> 
> I'll note majority of the problem most likely stems from
> mnt_get_write_access rolling with:

So anonymous pipes are based on pipefs which isn't exposed to userspace
at all. Neither the superblock nor an individual mount can go read-only
or change mount properties. The superblock cannot be frozen etc.

So really that mnt_get_write_access() should be pointless for
anonymous pipes. In other words, couldn't this also just be:

diff --git a/fs/pipe.c b/fs/pipe.c
index 8df42e1ab3a3..e9989621362c 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -604,12 +604,25 @@ 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);
+       if (ret > 0) {
+               int err;
+
+               if (!is_anon_pipe(file)) {
+                       if (sb_start_write_trylock(file_inode(filp)->i_sb)) {
+                               err = file_update_time(filp);
+                               if (err)
+                                       ret = err;
+
+                               sb_end_write(file_inode(filp)->i_sb);
+                       }
+               } else {
+                       // Anonymous pipes don't need all the mount + sb bruah.
+                       err = inode_needs_update_time(inode);
+                       if (err > 0)
+                               ret = inode_update_time(file_inode(filp), err);
+               }
        }
+
        return ret;
 }

and then have zero regression risk?

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

[Linus Torvalds]

On Tue, 4 Feb 2025 at 08:34, Christian Brauner <brauner@kernel.org> wrote:
>
> So really that mnt_get_write_access() should be pointless for
> anonymous pipes. In other words, couldn't this also just be:

I bet you'll find that even just inode_update_times() isn't all that cheap.

Yes, the silly "get writability" thing is pointless for pipes, but the
"get current time" really isn't a no-op either. There's all the crazy
"coarse time vs fine time" crud, there's justv a *lot* of overhead.

That's not really noticeable on a "real" file where you never access
just one byte anyway, and where any write costs are elsewhere. But
pipes can be very cheap, so the silly overheads really show up.

                 Linus