[PATCH 1/4] include/pipe_fs_i.h: add missing #includes

[PATCH 1/4] include/pipe_fs_i.h: add missing #includes

[Max Kellermann]

To verify that this header's #includes are correct, include it first
in fs/pipe.c.

To: Alexander Viro <viro@zeniv.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
To: linux-kernel@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
 fs/pipe.c                 | 2 +-
 include/linux/pipe_fs_i.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index cc28623a67b6..da842d13029d 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -5,6 +5,7 @@
  *  Copyright (C) 1991, 1992, 1999  Linus Torvalds
  */
 
+#include <linux/pipe_fs_i.h>
 #include <linux/mm.h>
 #include <linux/file.h>
 #include <linux/poll.h>
@@ -16,7 +17,6 @@
 #include <linux/mount.h>
 #include <linux/pseudo_fs.h>
 #include <linux/magic.h>
-#include <linux/pipe_fs_i.h>
 #include <linux/uio.h>
 #include <linux/highmem.h>
 #include <linux/pagemap.h>
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index c00c618ef290..0e36a58adf0e 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -2,6 +2,9 @@
 #ifndef _LINUX_PIPE_FS_I_H
 #define _LINUX_PIPE_FS_I_H
 
+#include <linux/mutex.h>
+#include <linux/wait.h>
+
 #define PIPE_DEF_BUFFERS	16
 
 #define PIPE_BUF_FLAG_LRU	0x01	/* page is on the LRU */
-- 
2.34.0

[PATCH 2/4] fs/pipe: remove duplicate "offset" initializer

[Max Kellermann]

This code duplication was introduced by commit a194dfe6e6f6 ("pipe:
Rearrange sequence in pipe_write() to preallocate slot"), but since
the pipe's mutex is locked, nobody else can modify the value
meanwhile.

To: Alexander Viro <viro@zeniv.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
To: linux-kernel@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
 fs/pipe.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index da842d13029d..aca717a89631 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -535,7 +535,6 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 				break;
 			}
 			ret += copied;
-			buf->offset = 0;
 			buf->len = copied;
 
 			if (!iov_iter_count(from))
-- 
2.34.0

[PATCH 3/4] fs/pipe: remove unnecessary "buf" initializer

[Max Kellermann]

This one has no effect because this value is not used before it is
assigned again.

To: Alexander Viro <viro@zeniv.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
To: linux-kernel@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
 fs/pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index aca717a89631..b2075ecd4751 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -487,7 +487,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 		head = pipe->head;
 		if (!pipe_full(head, pipe->tail, pipe->max_usage)) {
 			unsigned int mask = pipe->ring_size - 1;
-			struct pipe_buffer *buf = &pipe->bufs[head & mask];
+			struct pipe_buffer *buf;
 			struct page *page = pipe->tmp_page;
 			int copied;
 
-- 
2.34.0

[PATCH 4/4] pipe_fs_i.h: add pipe_buf_init()

[Max Kellermann]

Adds one central function which shall be used to initialize a newly
allocated struct pipe_buffer.  This shall make the pipe code more
robust for the next time the pipe_buffer struct gets modified, to
avoid leaving new members uninitialized.  Instead, adding new members
should also add a new pipe_buf_init() parameter, which causes
compile-time errors in call sites that were not adapted.

This commit doesn't refactor fs/fuse/dev.c because this code looks
obscure to me; it initializes pipe_buffers incrementally through a
variety of functions, too complicated for me to understand.

To: Alexander Viro <viro@zeniv.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
To: linux-kernel@vger.kernel.org
Signed-off-by: Max Kellermann <max.kellermann@gmail.com>
---
 fs/pipe.c                 | 11 +++--------
 fs/splice.c               |  9 ++++-----
 include/linux/pipe_fs_i.h | 20 ++++++++++++++++++++
 kernel/watch_queue.c      |  8 +++-----
 lib/iov_iter.c            | 13 +++----------
 5 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index b2075ecd4751..6da11ea9da49 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -518,14 +518,9 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 
 			/* Insert it into the buffer array */
 			buf = &pipe->bufs[head & mask];
-			buf->page = page;
-			buf->ops = &anon_pipe_buf_ops;
-			buf->offset = 0;
-			buf->len = 0;
-			if (is_packetized(filp))
-				buf->flags = PIPE_BUF_FLAG_PACKET;
-			else
-				buf->flags = PIPE_BUF_FLAG_CAN_MERGE;
+			pipe_buf_init(buf, page, 0, 0,
+				      &anon_pipe_buf_ops,
+				      is_packetized(filp) ? PIPE_BUF_FLAG_PACKET : PIPE_BUF_FLAG_CAN_MERGE);
 			pipe->tmp_page = NULL;
 
 			copied = copy_page_from_iter(page, 0, PAGE_SIZE, from);
diff --git a/fs/splice.c b/fs/splice.c
index 5dbce4dcc1a7..d2e4205acc46 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -200,12 +200,11 @@ ssize_t splice_to_pipe(struct pipe_inode_info *pipe,
 	while (!pipe_full(head, tail, pipe->max_usage)) {
 		struct pipe_buffer *buf = &pipe->bufs[head & mask];
 
-		buf->page = spd->pages[page_nr];
-		buf->offset = spd->partial[page_nr].offset;
-		buf->len = spd->partial[page_nr].len;
+		pipe_buf_init(buf, spd->pages[page_nr],
+			      spd->partial[page_nr].offset,
+			      spd->partial[page_nr].len,
+			      spd->ops, 0);
 		buf->private = spd->partial[page_nr].private;
-		buf->ops = spd->ops;
-		buf->flags = 0;
 
 		head++;
 		pipe->head = head;
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 0e36a58adf0e..61639682cc4e 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -179,6 +179,26 @@ static inline unsigned int pipe_space_for_user(unsigned int head, unsigned int t
 	return p_space;
 }
 
+/**
+ * Initialize a struct pipe_buffer.
+ */
+static inline void pipe_buf_init(struct pipe_buffer *buf,
+				 struct page *page,
+				 unsigned int offset, unsigned int len,
+				 const struct pipe_buf_operations *ops,
+				 unsigned int flags)
+{
+	buf->page = page;
+	buf->offset = offset;
+	buf->len = len;
+	buf->ops = ops;
+	buf->flags = flags;
+
+	/* not initializing the "private" member because it is only
+	   used by pipe_buf_operations which inject it via struct
+	   partial_page / struct splice_pipe_desc */
+}
+
 /**
  * pipe_buf_get - get a reference to a pipe_buffer
  * @pipe:	the pipe that the buffer belongs to
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index 9c9eb20dd2c5..34720138cc22 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -106,12 +106,10 @@ static bool post_one_notification(struct watch_queue *wqueue,
 	kunmap_atomic(p);
 
 	buf = &pipe->bufs[head & mask];
-	buf->page = page;
+	pipe_buf_init(buf, page, offset, len,
+		      &watch_queue_pipe_buf_ops,
+		      PIPE_BUF_FLAG_WHOLE);
 	buf->private = (unsigned long)wqueue;
-	buf->ops = &watch_queue_pipe_buf_ops;
-	buf->offset = offset;
-	buf->len = len;
-	buf->flags = PIPE_BUF_FLAG_WHOLE;
 	pipe->head = head + 1;
 
 	if (!test_and_clear_bit(note, wqueue->notes_bitmap)) {
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6dd5330f7a99..289e96947fb5 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -413,12 +413,8 @@ static size_t copy_page_to_iter_pipe(struct page *page, size_t offset, size_t by
 	if (pipe_full(i_head, p_tail, pipe->max_usage))
 		return 0;
 
-	buf->ops = &page_cache_pipe_buf_ops;
-	buf->flags = 0;
 	get_page(page);
-	buf->page = page;
-	buf->offset = offset;
-	buf->len = bytes;
+	pipe_buf_init(buf, page, offset, bytes, &page_cache_pipe_buf_ops, 0);
 
 	pipe->head = i_head + 1;
 	i->iov_offset = offset + bytes;
@@ -577,11 +573,8 @@ static size_t push_pipe(struct iov_iter *i, size_t size,
 		if (!page)
 			break;
 
-		buf->ops = &default_pipe_buf_ops;
-		buf->flags = 0;
-		buf->page = page;
-		buf->offset = 0;
-		buf->len = min_t(ssize_t, left, PAGE_SIZE);
+		pipe_buf_init(buf, page, 0, min_t(ssize_t, left, PAGE_SIZE),
+			      &default_pipe_buf_ops, 0);
 		left -= buf->len;
 		iter_head++;
 		pipe->head = iter_head;
-- 
2.34.0

Re: [PATCH 4/4] pipe_fs_i.h: add pipe_buf_init()

[Al Viro]

On Fri, Feb 25, 2022 at 07:54:31PM +0100, Max Kellermann wrote:

>  			/* Insert it into the buffer array */
>  			buf = &pipe->bufs[head & mask];
> -			buf->page = page;
> -			buf->ops = &anon_pipe_buf_ops;
> -			buf->offset = 0;
> -			buf->len = 0;
> -			if (is_packetized(filp))
> -				buf->flags = PIPE_BUF_FLAG_PACKET;
> -			else
> -				buf->flags = PIPE_BUF_FLAG_CAN_MERGE;
> +			pipe_buf_init(buf, page, 0, 0,
> +				      &anon_pipe_buf_ops,
> +				      is_packetized(filp) ? PIPE_BUF_FLAG_PACKET : PIPE_BUF_FLAG_CAN_MERGE);

*cringe*
FWIW, packetized case is very rare, so how about turning that into
			pipe_buf_init(buf, page, 0, 0,
				      &anon_pipe_buf_ops,
				      PIPE_BUF_FLAG_CAN_MERGE);
			if (unlikely(is_packetized(filp)))
				buf->flags = PIPE_BUF_FLAG_PACKET;
Your pipe_buf_init() is inlined, so it shouldn't be worse from the optimizer
POV - it should be able to start with calculating that value and then storing
that, etc.

Re: [PATCH 4/4] pipe_fs_i.h: add pipe_buf_init()

[Matthew Wilcox]

On Sun, Mar 13, 2022 at 02:37:43AM +0000, Al Viro wrote:
> On Fri, Feb 25, 2022 at 07:54:31PM +0100, Max Kellermann wrote:
> 
> >  			/* Insert it into the buffer array */
> >  			buf = &pipe->bufs[head & mask];
> > -			buf->page = page;
> > -			buf->ops = &anon_pipe_buf_ops;
> > -			buf->offset = 0;
> > -			buf->len = 0;
> > -			if (is_packetized(filp))
> > -				buf->flags = PIPE_BUF_FLAG_PACKET;
> > -			else
> > -				buf->flags = PIPE_BUF_FLAG_CAN_MERGE;
> > +			pipe_buf_init(buf, page, 0, 0,
> > +				      &anon_pipe_buf_ops,
> > +				      is_packetized(filp) ? PIPE_BUF_FLAG_PACKET : PIPE_BUF_FLAG_CAN_MERGE);
> 
> *cringe*
> FWIW, packetized case is very rare, so how about turning that into
> 			pipe_buf_init(buf, page, 0, 0,
> 				      &anon_pipe_buf_ops,
> 				      PIPE_BUF_FLAG_CAN_MERGE);
> 			if (unlikely(is_packetized(filp)))
> 				buf->flags = PIPE_BUF_FLAG_PACKET;
> Your pipe_buf_init() is inlined, so it shouldn't be worse from the optimizer
> POV - it should be able to start with calculating that value and then storing
> that, etc.

That's not equivalent.  I think the better option here is to always
initialise flags to 0 (and not have a parameter for it):

			pipe_buf_init(buf, page, 0, 0, &anon_pipe_buf_ops);
			if (is_packetized(filp))
				buf->flags = PIPE_BUF_FLAG_PACKET;
			else
				buf->flags = PIPE_BUF_FLAG_CAN_MERGE;

Re: [PATCH 4/4] pipe_fs_i.h: add pipe_buf_init()

[Al Viro]

On Sun, Mar 13, 2022 at 02:48:10AM +0000, Matthew Wilcox wrote:

> That's not equivalent.  I think the better option here is to always
> initialise flags to 0 (and not have a parameter for it):
> 
> 			pipe_buf_init(buf, page, 0, 0, &anon_pipe_buf_ops);
> 			if (is_packetized(filp))
> 				buf->flags = PIPE_BUF_FLAG_PACKET;
> 			else
> 				buf->flags = PIPE_BUF_FLAG_CAN_MERGE;

Not equivalent in which sense?  IDGI...  Your variant is basically

	X = 0;
	if (Y == constant)
		X = 1;
	else
		X = 2;

If gcc can optimize that to 

	X = (Y == constant) ? 1 : 2;

it should be able to do the same to
	X = 1;
	if (Y != constant)
		X = 2;

	What obstacles are there, besides a (false) assumption that
X might alias Y?  Which would apply to both variants...  Granted, I'm
half-asleep right now, so I might be missing something obvious...

Re: [PATCH 4/4] pipe_fs_i.h: add pipe_buf_init()

[Max Kellermann]

On 2022/03/13 03:37, Al Viro <viro@zeniv.linux.org.uk> wrote:
> *cringe*
> FWIW, packetized case is very rare, so how about turning that into

I have no hard feelings how this API must look like, I only want it to
exist, to reduce code fragility a bit.  Tell me how you want it, and
I'll resubmit.

Re: [PATCH 1/4] include/pipe_fs_i.h: add missing #includes

[Al Viro]

On Fri, Feb 25, 2022 at 07:54:28PM +0100, Max Kellermann wrote:

> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index c00c618ef290..0e36a58adf0e 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -2,6 +2,9 @@
>  #ifndef _LINUX_PIPE_FS_I_H
>  #define _LINUX_PIPE_FS_I_H
>  
> +#include <linux/mutex.h>
> +#include <linux/wait.h>

TBH, I'd rather avoid breeding chain includes; sure, mutex.h and wait.h
are extremely common anyway.  Oh, well....

Re: [PATCH 1/4] include/pipe_fs_i.h: add missing #includes

[Max Kellermann]

On 2022/03/13 02:49, Al Viro <viro@zeniv.linux.org.uk> wrote:
> TBH, I'd rather avoid breeding chain includes; sure, mutex.h and wait.h
> are extremely common anyway.  Oh, well....

In my usual coding style, I expect I can include any header and it
will bring its whole dependency chain (which should be as small as
possible, but not smaller).  This seems cleaner to me, because .c
files need to have no insight what a .h file needs (even if the
dependencies are "extremely common").

If the kernel coding style does not consider this useful, we can of
course easily drop that patch.