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

Linus Torvalds
2012-04-27T23:27:41Z

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

On Fri, Apr 27, 2012 at 3:42 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> The obvious way to set the flag would seem to me to also btake O_DIRECT
> on the pipe as meaning this ?

Ok, that really does seem to be pretty natural.

The attached patch implements that, and makes autofs automatically set
the O_DIRECT flag. And now that it is exposed to user-space, here's a
test-program that uses it:

[torvalds@i5 ~]$ cat packet.c
#define _GNU_SOURCE
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

#define REPORT(op,fd,size) do { \
int ret = op(fd, buffer, size); \
fprintf(stderr, #op "(%d)=%d\n", size, ret); \
} while (0)

#define WRITE(x) REPORT(write,fd[1],x)
#define READ(x) REPORT(read,fd[0],x)

int main(void)
{
int fd[2];
static char buffer[1024];

if (pipe2(fd, O_DIRECT)) {
perror("pipe2 O_DIRECT");
exit(1);
}
WRITE(8);
WRITE(12);
WRITE(300);
WRITE(5);
READ(64);
READ(64);
WRITE(7);
READ(64);
READ(64);
READ(64);
return 0;
}

which results in this output:

[torvalds@i5 ~]$ ./a.out
write(8)=8
write(12)=12
write(300)=300
write(5)=5
read(64)=8
read(64)=12
write(7)=7
read(64)=64
read(64)=5
read(64)=7

iow, notice how all the read() calls return individual packets (and
the 300-byte packet was truncated to 64 bytes).

HOWEVER. I haven't actually tested the autofs part. But I suspect it
"should just work".

Thomas, Michael, can you test whether this patch (again: note the "+8"
in autofs_v5_packet_size() purely for debug purposes, so this would be
interesting to test for other people too) makes your setups work?

Linus

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

fs/autofs4/autofs_i.h | 12 ++++++++++++
fs/autofs4/dev-ioctl.c | 2 +-
fs/autofs4/inode.c | 2 +-
fs/autofs4/waitq.c | 14 ++++++--------
fs/pipe.c | 17 +++++++++++------
5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index eb1cc92cd67d..a07885bf0463 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -34,6 +34,7 @@
#include <linux/sched.h>
#include <linux/mount.h>
#include <linux/namei.h>
+#include <linux/pipe_fs_i.h>
#include <asm/current.h>
#include <asm/uaccess.h>

@@ -270,6 +271,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 (!pipe->f_dentry->d_inode->i_pipe)
+ 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 9dacb8586701..6259e7142032 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 d8dc002e9cc3..c525b74deefd 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -292,7 +292,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 9c098db43344..5ce5026200b9 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -97,16 +97,14 @@ static int autofs4_write(struct autofs_sb_info *sbi,
*
* The packets are identical on x86-32 and x86-64, but have different
* alignment. Which means that 'sizeof()' will give different results.
- * Fix it up for the case of running 32-bit user mode on a 64-bit kernel.
+ * However, we packetize the pipe, so just use the bigger size, the
+ * pipe read will discard the rest.
+ *
+ * This adds on 8 bytes of garbage FOR DEBUGGING ONLY!
*/
-static noinline size_t autofs_v5_packet_size(struct autofs_sb_info *sbi)
+static inline size_t autofs_v5_packet_size(struct autofs_sb_info *sbi)
{
- size_t pktsz = sizeof(struct autofs_v5_packet);
-#if defined(CONFIG_X86_64) && defined(CONFIG_COMPAT)
- if (sbi->compat_daemon > 0)
- pktsz -= 4;
-#endif
- return pktsz;
+ return sizeof(struct autofs_v5_packet)+8;
}

static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
diff --git a/fs/pipe.c b/fs/pipe.c
index 25feaa3faac0..c65051eb6893 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -346,6 +346,11 @@ static const struct pipe_buf_operations anon_pipe_buf_ops = {
.get = generic_pipe_buf_get,
};

+static inline int is_packetized(struct file *file)
+{
+ return (file->f_flags & O_DIRECT) != 0;
+}
+
static ssize_t
pipe_read(struct kiocb *iocb, const struct iovec *_iov,
unsigned long nr_segs, loff_t pos)
@@ -407,7 +412,7 @@ redo:
ret += chars;
buf->offset += chars;
buf->len -= chars;
- if (!buf->len) {
+ if (!buf->len || is_packetized(filp)) {
buf->ops = NULL;
ops->release(pipe, buf);
curbuf = (curbuf + 1) & (pipe->buffers - 1);
@@ -416,7 +421,7 @@ redo:
do_wakeup = 1;
}
total_len -= chars;
- if (!total_len)
+ if (!total_len || is_packetized(filp))
break; /* common path: read succeeded */
}
if (bufs) /* More to do? */
@@ -490,7 +495,7 @@ pipe_write(struct kiocb *iocb, const struct iovec *_iov,

/* We try to merge small writes */
chars = total_len & (PAGE_SIZE-1); /* size of the last buffer */
- if (pipe->nrbufs && chars != 0) {
+ if (!is_packetized(filp) && pipe->nrbufs && chars != 0) {
int lastbuf = (pipe->curbuf + pipe->nrbufs - 1) &
(pipe->buffers - 1);
struct pipe_buffer *buf = pipe->bufs + lastbuf;
@@ -1013,7 +1018,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;
@@ -1046,7 +1051,7 @@ struct file *create_read_pipe(struct file *wrf, int flags)
return ERR_PTR(-ENFILE);

path_get(&wrf->f_path);
- f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
+ f->f_flags = O_RDONLY | (flags & (O_NONBLOCK | O_DIRECT));

return f;
}
@@ -1057,7 +1062,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);