Re: [PATCH v2] Introduce a version6 of autofs interface, to fix design error.

Linus Torvalds
2012-04-29T19:09:39Z

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On Sun, Apr 29, 2012 at 11:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But I think I have another approach. We can make the *writer* be the
> only one that cares about the packetized nature, and if it's a packet
> write, it will set a PIPE_BUF_PACKET flag in the pipe buffer. The
> reader can react to that. I think that actually has the potential to
> make the code a bit prettier too..

Patch attached.

This keeps Alan's idea of using O_DIRECT, but only makes it matter for
the writer - because now the "packet" thing is a per-pipe-buffer state
(we already had per-pipe-buffer flags, so this is not anything new).
So when you *write* using a O_DIRECT pipe, it will create packetized
buffers, and the reader just sees that directly.

So this should "just work", and doesn't need any extra flags in the
pipe_inode_info. And the user space interface remains the same: you
can create these packet pipes with "pipe2(fd, O_DIRECT)" if you want
to, and my test-program gives exactly the same output.

Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 4757 bytes --]

fs/autofs4/autofs_i.h | 11 +++++++++++
fs/autofs4/dev-ioctl.c | 2 +-
fs/autofs4/inode.c | 2 +-
fs/autofs4/waitq.c | 2 +-
fs/pipe.c | 31 +++++++++++++++++++++++++++++--
include/linux/pipe_fs_i.h | 1 +
6 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index d8d8e7ba6a1e..908e18455413 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -269,6 +269,17 @@ int autofs4_fill_super(struct super_block *, void *, int);
struct autofs_info *autofs4_new_ino(struct autofs_sb_info *);
void autofs4_clean_ino(struct autofs_info *);

+static inline int autofs_prepare_pipe(struct file *pipe)
+{
+ if (!pipe->f_op || !pipe->f_op->write)
+ return -EINVAL;
+ if (!S_ISFIFO(pipe->f_dentry->d_inode->i_mode))
+ return -EINVAL;
+ /* We want a packet pipe */
+ pipe->f_flags |= O_DIRECT;
+ return 0;
+}
+
/* Queue management functions */

int autofs4_wait(struct autofs_sb_info *,struct dentry *, enum autofs_notify);
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 3dfd615afb6b..aa9103f8f01b 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -376,7 +376,7 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
err = -EBADF;
goto out;
}
- if (!pipe->f_op || !pipe->f_op->write) {
+ if (autofs_prepare_pipe(pipe) < 0) {
err = -EPIPE;
fput(pipe);
goto out;
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 14c7bc02349e..6e488ebe7784 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -290,7 +290,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
printk("autofs: could not open pipe file descriptor\n");
goto fail_dput;
}
- if (!pipe->f_op || !pipe->f_op->write)
+ if (autofs_prepare_pipe(pipe) < 0)
goto fail_fput;
sbi->pipe = pipe;
sbi->pipefd = pipefd;
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index da8876d38a7b..f624cd0ff84d 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -91,7 +91,7 @@ static int autofs4_write(struct autofs_sb_info *sbi,

return (bytes > 0);
}
-
+
static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
struct autofs_wait_queue *wq,
int type)
diff --git a/fs/pipe.c b/fs/pipe.c
index 25feaa3faac0..fec5e4ad071a 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -346,6 +346,16 @@ static const struct pipe_buf_operations anon_pipe_buf_ops = {
.get = generic_pipe_buf_get,
};

+static const struct pipe_buf_operations packet_pipe_buf_ops = {
+ .can_merge = 0,
+ .map = generic_pipe_buf_map,
+ .unmap = generic_pipe_buf_unmap,
+ .confirm = generic_pipe_buf_confirm,
+ .release = anon_pipe_buf_release,
+ .steal = generic_pipe_buf_steal,
+ .get = generic_pipe_buf_get,
+};
+
static ssize_t
pipe_read(struct kiocb *iocb, const struct iovec *_iov,
unsigned long nr_segs, loff_t pos)
@@ -407,6 +417,13 @@ redo:
ret += chars;
buf->offset += chars;
buf->len -= chars;
+
+ /* Was it a packet buffer? Clean up and exit */
+ if (buf->flags & PIPE_BUF_FLAG_PACKET) {
+ total_len = chars;
+ buf->len = 0;
+ }
+
if (!buf->len) {
buf->ops = NULL;
ops->release(pipe, buf);
@@ -459,6 +476,11 @@ redo:
return ret;
}

+static inline int is_packetized(struct file *file)
+{
+ return (file->f_flags & O_DIRECT) != 0;
+}
+
static ssize_t
pipe_write(struct kiocb *iocb, const struct iovec *_iov,
unsigned long nr_segs, loff_t ppos)
@@ -593,6 +615,11 @@ redo2:
buf->ops = &anon_pipe_buf_ops;
buf->offset = 0;
buf->len = chars;
+ buf->flags = 0;
+ if (is_packetized(filp)) {
+ buf->ops = &packet_pipe_buf_ops;
+ buf->flags = PIPE_BUF_FLAG_PACKET;
+ }
pipe->nrbufs = ++bufs;
pipe->tmp_page = NULL;

@@ -1013,7 +1040,7 @@ struct file *create_write_pipe(int flags)
goto err_dentry;
f->f_mapping = inode->i_mapping;

- f->f_flags = O_WRONLY | (flags & O_NONBLOCK);
+ f->f_flags = O_WRONLY | (flags & (O_NONBLOCK | O_DIRECT));
f->f_version = 0;

return f;
@@ -1057,7 +1084,7 @@ int do_pipe_flags(int *fd, int flags)
int error;
int fdw, fdr;

- if (flags & ~(O_CLOEXEC | O_NONBLOCK))
+ if (flags & ~(O_CLOEXEC | O_NONBLOCK | O_DIRECT))
return -EINVAL;

fw = create_write_pipe(flags);
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 6d626ff0cfd0..e1ac1ce16fb0 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -6,6 +6,7 @@
#define PIPE_BUF_FLAG_LRU 0x01 /* page is on the LRU */
#define PIPE_BUF_FLAG_ATOMIC 0x02 /* was atomically mapped */
#define PIPE_BUF_FLAG_GIFT 0x04 /* page is a gift */
+#define PIPE_BUF_FLAG_PACKET 0x08 /* read() as a packet */

/**
* struct pipe_buffer - a linux kernel pipe buffer