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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
