[PATCH 1/4] pipe: reduce padding in struct pipe_inode_info

[PATCH 1/4] pipe: reduce padding in struct pipe_inode_info

[Max Kellermann]

This has no effect on 64 bit because there are 10 32-bit integers
surrounding the two bools, but on 32 bit architectures, this reduces
the struct size by 4 bytes by merging the two bools into one word.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 include/linux/pipe_fs_i.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 608a9eb86bff..598a411d7da2 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -62,9 +62,6 @@ struct pipe_inode_info {
 	unsigned int tail;
 	unsigned int max_usage;
 	unsigned int ring_size;
-#ifdef CONFIG_WATCH_QUEUE
-	bool note_loss;
-#endif
 	unsigned int nr_accounted;
 	unsigned int readers;
 	unsigned int writers;
@@ -72,6 +69,9 @@ struct pipe_inode_info {
 	unsigned int r_counter;
 	unsigned int w_counter;
 	bool poll_usage;
+#ifdef CONFIG_WATCH_QUEUE
+	bool note_loss;
+#endif
 	struct page *tmp_page;
 	struct fasync_struct *fasync_readers;
 	struct fasync_struct *fasync_writers;
-- 
2.39.2

[PATCH 2/4] fs/pipe: move check to pipe_has_watch_queue()

[Max Kellermann]

This declutters the code by reducing the number of #ifdefs and makes
the watch_queue checks simpler.  This has no runtime effect; the
machine code is identical.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 fs/pipe.c                 | 12 +++---------
 include/linux/pipe_fs_i.h | 13 +++++++++++++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 6c1a9b1db907..6ecaccb48738 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -437,12 +437,10 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 	}
 
-#ifdef CONFIG_WATCH_QUEUE
-	if (pipe->watch_queue) {
+	if (pipe_has_watch_queue(pipe)) {
 		ret = -EXDEV;
 		goto out;
 	}
-#endif
 
 	/*
 	 * If it wasn't empty we try to merge new data into
@@ -1325,10 +1323,8 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned int arg)
 	unsigned int nr_slots, size;
 	long ret = 0;
 
-#ifdef CONFIG_WATCH_QUEUE
-	if (pipe->watch_queue)
+	if (pipe_has_watch_queue(pipe))
 		return -EBUSY;
-#endif
 
 	size = round_pipe_size(arg);
 	nr_slots = size >> PAGE_SHIFT;
@@ -1380,10 +1376,8 @@ struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice)
 
 	if (file->f_op != &pipefifo_fops || !pipe)
 		return NULL;
-#ifdef CONFIG_WATCH_QUEUE
-	if (for_splice && pipe->watch_queue)
+	if (for_splice && pipe_has_watch_queue(pipe))
 		return NULL;
-#endif
 	return pipe;
 }
 
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 598a411d7da2..699507c52b72 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -124,6 +124,19 @@ struct pipe_buf_operations {
 	bool (*get)(struct pipe_inode_info *, struct pipe_buffer *);
 };
 
+/**
+ * pipe_has_watch_queue - Check whether the pipe is a watch_queue,
+ * i.e. it was created with O_NOTIFICATION_PIPE
+ */
+static inline bool pipe_has_watch_queue(const struct pipe_inode_info *pipe)
+{
+#ifdef CONFIG_WATCH_QUEUE
+	return pipe->watch_queue != NULL;
+#else
+	return false;
+#endif
+}
+
 /**
  * pipe_empty - Return true if the pipe is empty
  * @head: The pipe ring head pointer
-- 
2.39.2

[PATCH 3/4] fs/pipe: remove unnecessary spinlock from pipe_write()

[Max Kellermann]

This reverts commit 8df441294dd3 ("pipe: Check for ring full inside of
the spinlock in pipe_write()") which was obsoleted by commit
c73be61cede ("pipe: Add general notification queue support") because
now pipe_write() fails early with -EXDEV if there is a watch_queue.

Without a watch_queue, no notifications can be posted to the pipe and
mutex protection is enough, as can be seen in splice_pipe_to_pipe()
which does not use the spinlock either.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 fs/pipe.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 6ecaccb48738..939def02c18c 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -505,16 +505,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
 			 * it, either the reader will consume it or it'll still
 			 * be there for the next write.
 			 */
-			spin_lock_irq(&pipe->rd_wait.lock);
-
-			head = pipe->head;
-			if (pipe_full(head, pipe->tail, pipe->max_usage)) {
-				spin_unlock_irq(&pipe->rd_wait.lock);
-				continue;
-			}
-
 			pipe->head = head + 1;
-			spin_unlock_irq(&pipe->rd_wait.lock);
 
 			/* Insert it into the buffer array */
 			buf = &pipe->bufs[head & mask];
-- 
2.39.2

[PATCH 4/4] fs/pipe: use spinlock in pipe_read() only if there is a watch_queue

[Max Kellermann]

If there is no watch_queue, holding the pipe mutex is enough to
prevent concurrent writes, and we can avoid the spinlock.

O_NOTIFICATION_QUEUE is an exotic and rarely used feature, and of all
the pipes that exist at any given time, only very few actually have a
watch_queue, therefore it appears worthwile to optimize the common
case.

This patch does not optimize pipe_resize_ring() where the spinlocks
could be avoided as well; that does not seem like a worthwile
optimization because this function is not called often.

Related commits:

- commit 8df441294dd3 ("pipe: Check for ring full inside of the
  spinlock in pipe_write()")
- commit b667b8673443 ("pipe: Advance tail pointer inside of wait
  spinlock in pipe_read()")
- commit 189b0ddc2451 ("pipe: Fix missing lock in pipe_resize_ring()")

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 fs/pipe.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 939def02c18c..da557eff9560 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -322,14 +322,34 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 
 			if (!buf->len) {
 				pipe_buf_release(pipe, buf);
-				spin_lock_irq(&pipe->rd_wait.lock);
+
+				if (pipe_has_watch_queue(pipe)) {
+					/* if the pipe has a
+					 * watch_queue, we need
+					 * additional protection by
+					 * the spinlock because
+					 * notifications get posted
+					 * with only this spinlock, no
+					 * mutex
+					 */
+
+					spin_lock_irq(&pipe->rd_wait.lock);
 #ifdef CONFIG_WATCH_QUEUE
-				if (buf->flags & PIPE_BUF_FLAG_LOSS)
-					pipe->note_loss = true;
+					if (buf->flags & PIPE_BUF_FLAG_LOSS)
+						pipe->note_loss = true;
 #endif
-				tail++;
-				pipe->tail = tail;
-				spin_unlock_irq(&pipe->rd_wait.lock);
+					tail++;
+					pipe->tail = tail;
+					spin_unlock_irq(&pipe->rd_wait.lock);
+				} else {
+					/* without a watch_queue, we
+					 * can simply increment the
+					 * tail without the spinlock -
+					 * the mutex is enough
+					 */
+
+					pipe->tail = ++tail;
+				}
 			}
 			total_len -= chars;
 			if (!total_len)
-- 
2.39.2

Re: [PATCH 1/4] pipe: reduce padding in struct pipe_inode_info

[Christian Brauner]

On Thu, 21 Sep 2023 09:57:52 +0200, Max Kellermann wrote:
> This has no effect on 64 bit because there are 10 32-bit integers
> surrounding the two bools, but on 32 bit architectures, this reduces
> the struct size by 4 bytes by merging the two bools into one word.
> 
> 

I've massaged the last commit a bit and moved that bit into a helper
instead of indentorama in pipe_write(). keyutils watchqueue tests and
LTP pipe and watch queue tests pass without regressions.

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc 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.misc

[1/4] pipe: reduce padding in struct pipe_inode_info
      https://git.kernel.org/vfs/vfs/c/5ba6d9b6d526
[2/4] fs/pipe: move check to pipe_has_watch_queue()
      https://git.kernel.org/vfs/vfs/c/7084dde72592
[3/4] fs/pipe: remove unnecessary spinlock from pipe_write()
      https://git.kernel.org/vfs/vfs/c/b9e8c77cad52
[4/4] fs/pipe: use spinlock in pipe_read() only if there is a watch_queue
      https://git.kernel.org/vfs/vfs/c/4c280825d39c