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

Commit Message

Oleg Nesterov Feb. 5, 2025, 3:33 p.m. UTC

So that fifos and anonymous pipes could have different f_op methods.
Preparation to simplify the next patch.

Signed-off-by: Oleg Nesterov <[e-mail]>
---
 fs/internal.h |  1 +
 fs/pipe.c     | 23 ++++++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

Comments

Linus Torvalds Feb. 5, 2025, 4:06 p.m. UTC | #1

On Wed, 5 Feb 2025 at 07:34, Oleg Nesterov <[e-mail]> 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

Oleg Nesterov Feb. 5, 2025, 4:16 p.m. UTC | #2

On 02/05, Linus Torvalds wrote:
>
> On Wed, 5 Feb 2025 at 07:34, Oleg Nesterov <[e-mail]> 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.

Patch

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;