This patchset extends FUSE such that it supports more file operations
and is consisted of the following seven patches.

  0001-FUSE-add-include-protectors.patch
  0002-FUSE-pass-nonblock-flag-to-client.patch
  0003-FUSE-implement-nonseekable-open.patch
  0004-FUSE-implement-direct-lseek-support.patch
  0005-FUSE-implement-ioctl-support.patch
  0006-FUSE-implement-unsolicited-notification.patch
  0007-FUSE-implement-poll-support.patch

The added features will be used primarily for CUSE but can be used by
any FUSE client.  Accompanying libfuse updates will be posted
separately.

This patchset is on top of

  2.6.27-rc4 (b8e6c91c74e9f0279b7c51048779b3d62da60b88)
+ [1] 9p-use-single-poller patchset
+ [2] wait-kill-is_sync_wait
+ [3] poll-allow-f_op_poll-to-sleep

The above three patches allow f_op->poll() to sleep and 0007 depends
on it.

This patchset is available in the following git tree.

 http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=extend-fuse
 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git extend-fuse

and contains the following changes.

 fs/fuse/dev.c        |   49 +++++
 fs/fuse/file.c       |  446 +++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h     |   31 +++
 fs/fuse/inode.c      |    5 
 include/linux/fuse.h |   82 +++++++++
 5 files changed, 599 insertions(+), 14 deletions(-)

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/726098
[2] http://article.gmane.org/gmane.linux.kernel/726176
[3] http://article.gmane.org/gmane.linux.kernel/726178
--

[ message continues ]

Pass O_NONBLOCK to client on reads and writes using FUSE_READ_NONBLOCK
and FUSE_WRITE_NONBLOCK respectively.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/file.c       |   10 +++++++---
 include/linux/fuse.h |    3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 2bada6b..d405865 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -386,14 +386,15 @@ static size_t fuse_send_read(struct fuse_req *req, struct file *file,
 			     fl_owner_t owner)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	struct fuse_read_in *inarg = &req->misc.read.in;
 
 	fuse_read_fill(req, file, inode, pos, count, FUSE_READ);
 	if (owner != NULL) {
-		struct fuse_read_in *inarg = &req->misc.read.in;
-
 		inarg->read_flags |= FUSE_READ_LOCKOWNER;
 		inarg->lock_owner = fuse_lock_owner_id(fc, owner);
 	}
+	if (file->f_flags & O_NONBLOCK)
+		inarg->read_flags |= FUSE_READ_NONBLOCK;
 	request_send(fc, req);
 	return req->out.args[0].size;
 }
@@ -628,12 +629,15 @@ static size_t fuse_send_write(struct fuse_req *req, struct file *file,
 			      fl_owner_t owner)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	struct fuse_write_in *inarg = &req->misc.write.in;
+
 	fuse_write_fill(req, file, file->private_data, inode, pos, count, 0);
 	if (owner != NULL) {
-		struct fuse_write_in *inarg = &req->misc.write.in;
 		inarg->write_flags |= FUSE_WRITE_LOCKOWNER;
 		inarg->lock_owner = fuse_lock_owner_id(fc, owner);
 	}
+	if (file->f_flags & O_NONBLOCK)
+		inarg->write_flags |= FUSE_WRITE_NONBLOCK;
 	request_send(fc, req);
 	return req->misc.write.out.size;
 }
diff --git a/include/linux/fuse.h b/include/linux/fuse.h
index 776ab72..724ca19 100644
--- a/include/linux/fuse.h
+++ b/include/linux/fuse.h
@@ -137,14 +137,17 @@ struct fuse_file_lock {
  *
  * FUSE_WRITE_CACHE: delayed write from page cache, file handle is guessed
  * FUSE_WRITE_LOCKOWNER: lock_owner field is valid
+ * FUSE_WRITE_NONBLOCK: perform non-blocking ...
[ message continues ]

Let the client request nonseekable open using FOPEN_NONSEEKABLE and
call nonseekable_open() on the file if requested.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/file.c       |    2 ++
 include/linux/fuse.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index d405865..9c44f9c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -101,6 +101,8 @@ void fuse_finish_open(struct inode *inode, struct file *file,
 		file->f_op = &fuse_direct_io_file_operations;
 	if (!(outarg->open_flags & FOPEN_KEEP_CACHE))
 		invalidate_inode_pages2(inode->i_mapping);
+	if (outarg->open_flags & FOPEN_NONSEEKABLE)
+		nonseekable_open(inode, file);
 	ff->fh = outarg->fh;
 	file->private_data = fuse_file_get(ff);
 }
diff --git a/include/linux/fuse.h b/include/linux/fuse.h
index 724ca19..431666e 100644
--- a/include/linux/fuse.h
+++ b/include/linux/fuse.h
@@ -101,9 +101,11 @@ struct fuse_file_lock {
  *
  * FOPEN_DIRECT_IO: bypass page cache for this open file
  * FOPEN_KEEP_CACHE: don't invalidate the data cache on open
+ * FOPEN_NONSEEKABLE: the file is not seekable
  */
 #define FOPEN_DIRECT_IO		(1 << 0)
 #define FOPEN_KEEP_CACHE	(1 << 1)
+#define FOPEN_NONSEEKABLE	(1 << 2)
 
 /**
  * INIT request/reply flags
-- 
1.5.4.5

--

[ message continues ]

Add include protectors to include/linux/fuse.h and fs/fuse/fuse_i.h.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/fuse_i.h     |    5 +++++
 include/linux/fuse.h |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 3a87607..e2b3b72 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -6,6 +6,9 @@
   See the file COPYING.
 */
 
+#ifndef __FS_FUSE_I_H
+#define __FS_FUSE_I_H
+
 #include <linux/fuse.h>
 #include <linux/fs.h>
 #include <linux/mount.h>
@@ -655,3 +658,5 @@ void fuse_set_nowrite(struct inode *inode);
 void fuse_release_nowrite(struct inode *inode);
 
 u64 fuse_get_attr_version(struct fuse_conn *fc);
+
+#endif /* __FS_FUSE_I_H */
diff --git a/include/linux/fuse.h b/include/linux/fuse.h
index 265635d..776ab72 100644
--- a/include/linux/fuse.h
+++ b/include/linux/fuse.h
@@ -19,6 +19,9 @@
  *  - add file flags field to fuse_read_in and fuse_write_in
  */
 
+#ifndef __LINUX_FUSE_H
+#define __LINUX_FUSE_H
+
 #include <asm/types.h>
 #include <linux/major.h>
 
@@ -409,3 +412,5 @@ struct fuse_dirent {
 #define FUSE_DIRENT_ALIGN(x) (((x) + sizeof(__u64) - 1) & ~(sizeof(__u64) - 1))
 #define FUSE_DIRENT_SIZE(d) \
 	FUSE_DIRENT_ALIGN(FUSE_NAME_OFFSET + (d)->namelen)
+
+#endif /* __LINUX_FUSE_H */
-- 
1.5.4.5

--

[ message continues ]

Allow clients to implement private lseek.  The feature is negotiated
using FUSE_DIRECT_LSEEK flag during INIT.  If the client doesn't
request direct lseek, the original implicit lseek is used.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/file.c       |   52 ++++++++++++++++++++++++++++++++++++++++++-------
 fs/fuse/fuse_i.h     |    3 ++
 fs/fuse/inode.c      |    4 ++-
 include/linux/fuse.h |   14 +++++++++++++
 4 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 9c44f9c..fa27edb 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1448,18 +1448,53 @@ static sector_t fuse_bmap(struct address_space *mapping, sector_t block)
 
 static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin)
 {
-	loff_t retval;
+	loff_t retval = -EINVAL;
 	struct inode *inode = file->f_path.dentry->d_inode;
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (is_bad_inode(inode))
+		return -EIO;
 
 	mutex_lock(&inode->i_mutex);
-	switch (origin) {
-	case SEEK_END:
-		offset += i_size_read(inode);
-		break;
-	case SEEK_CUR:
-		offset += file->f_pos;
+
+	if (fc->direct_lseek) {
+		struct fuse_file *ff = file->private_data;
+		struct fuse_lseek_in inarg = { .fh = ff->fh, .pos = file->f_pos,
+					   .offset = offset, .origin = origin };
+		struct fuse_lseek_out outarg;
+		struct fuse_req *req;
+		int err;
+
+		req = fuse_get_req(fc);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+
+		req->in.h.opcode = FUSE_LSEEK;
+		req->in.h.nodeid = get_node_id(inode);
+		req->in.numargs = 1;
+		req->in.args[0].size = sizeof(inarg);
+		req->in.args[0].value = &inarg;
+		req->out.numargs = 1;
+		req->out.args[0].size = sizeof(outarg);
+		req->out.args[0].value = &outarg;
+		request_send(fc, req);
+		err = req->out.h.error;
+		fuse_put_request(fc, req);
+
+		if (err)
+			return err;
+
+		offset = outarg.pos;
+	} else {
+		switch (origin) {
+		case SEEK_END:
+			offset += ...
[ message continues ]

Clients always used to write only in response to read requests.  To
implement poll efficiently, clients should be able to issue
unsolicited notifications.  This patch implements basic notification
support.

Zero fuse_out_header.unique is now accepted and considered unsolicited
notification and the error field contains notification code.  This
patch doesn't implement any actual notification.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/dev.c        |   34 ++++++++++++++++++++++++++++++++--
 include/linux/fuse.h |    4 ++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 87250b6..2fb65a5 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -813,6 +813,21 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, const struct iovec *iov,
 	return err;
 }
 
+static int fuse_handle_notify(struct fuse_conn *fc, enum fuse_notify_code code,
+			      unsigned int size, struct fuse_copy_state *cs)
+{
+	int err;
+
+	switch (code) {
+	default:
+		err = -EINVAL;
+		break;
+	}
+
+	fuse_copy_finish(cs);
+	return err;
+}
+
 /* Look up request on processing list by unique ID */
 static struct fuse_req *request_find(struct fuse_conn *fc, u64 unique)
 {
@@ -876,9 +891,24 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, const struct iovec *iov,
 	err = fuse_copy_one(&cs, &oh, sizeof(oh));
 	if (err)
 		goto err_finish;
+
+	if (oh.len != nbytes)
+		goto err_finish;
+
+	/*
+	 * Zero oh.unique indicates unsolicited notification message
+	 * and error contains notification code.
+	 */
+	if (!oh.unique) {
+		err = fuse_handle_notify(fc, oh.error, nbytes - sizeof(oh),
+					 &cs);
+		if (err)
+			return err;
+		return nbytes;
+	}
+
 	err = -EINVAL;
-	if (!oh.unique || oh.error <= -1000 || oh.error > 0 ||
-	    oh.len != nbytes)
+	if (oh.error <= -1000 || oh.error > 0)
 		goto err_finish;
 
 	spin_lock(&fc->lock);
diff --git a/include/linux/fuse.h b/include/linux/fuse.h
index 5a396d3..0044590 100644
--- ...
[ message continues ]

Implement poll support.  Polled files are indexed using fh in a RB
tree rooted at fuse_conn->polled_files.  All pollable files should
have unique fh as that's how notifications are matched to files.  If
duplicate fhs are detected, FUSE spits out warning message.  Poll will
malfunction but otherwise it will work fine.

Client should send FUSE_NOTIFY_POLL notification once after processing
FUSE_POLL which has FUSE_POLL_SCHEDULE_NOTIFY set.  Sending
notification unconditionally after the latest poll or everytime file
content might have changed is inefficient but won't cause malfunction.

fuse_file_poll() can sleep and requires patches from the following
thread which allows f_op->poll() to sleep.

  http://thread.gmane.org/gmane.linux.kernel/726176

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/dev.c        |   15 +++++
 fs/fuse/file.c       |  141 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/fuse/fuse_i.h     |   23 ++++++++
 fs/fuse/inode.c      |    1 +
 include/linux/fuse.h |   24 +++++++++
 5 files changed, 204 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 2fb65a5..1c422f9 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -819,11 +819,26 @@ static int fuse_handle_notify(struct fuse_conn *fc, enum fuse_notify_code code,
 	int err;
 
 	switch (code) {
+	case FUSE_NOTIFY_POLL: {
+		struct fuse_notify_poll_wakeup_out outarg;
+
+		err = -EINVAL;
+		if (size != sizeof(outarg))
+			goto out;
+
+		err = fuse_copy_one(cs, &outarg, sizeof(outarg));
+		if (err)
+			goto out;
+
+		err = fuse_notify_poll_wakeup(fc, &outarg);
+		break;
+	}
 	default:
 		err = -EINVAL;
 		break;
 	}
 
+ out:
 	fuse_copy_finish(cs);
 	return err;
 }
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 75a9b53..5d704e3 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -59,6 +59,8 @@ struct fuse_file *fuse_file_alloc(void)
 			INIT_LIST_HEAD(&ff->write_entry);
 			atomic_set(&ff->count, 0);
 ...
[ message continues ]

ioctl support is tricky to implement because only the ioctl
implementation itself knows which memory regions need to be read
and/or written.  To support this, fuse client can request retry of
ioctl specifying memory regions to read and write.  Deep copying
(nested pointers) can be implemented by retrying multiple times
resolving one depth of dereference at a time.

Plese read the comment on top of fs/fuse/file.c::fuse_file_do_ioctl()
for more information.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 fs/fuse/file.c       |  241 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fuse.h |   30 ++++++
 2 files changed, 271 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index fa27edb..75a9b53 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1507,6 +1507,243 @@ static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin)
 	return retval;
 }
 
+static int fuse_ioctl_copy_user(struct page **pages, struct iovec *iov,
+			unsigned int nr_segs, size_t bytes, bool to_user)
+{
+	struct iov_iter ii;
+	int page_idx = 0;
+
+	if (!bytes)
+		return 0;
+
+	iov_iter_init(&ii, iov, nr_segs, bytes, 0);
+
+	while (iov_iter_count(&ii)) {
+		struct page *page = pages[page_idx++];
+		size_t todo = min_t(size_t, PAGE_SIZE, iov_iter_count(&ii));
+		void *kaddr, *map;
+
+		kaddr = map = kmap(page);
+
+		while (todo) {
+			char __user *uaddr = ii.iov->iov_base + ii.iov_offset;
+			size_t iov_len = ii.iov->iov_len - ii.iov_offset;
+			size_t copy = min(todo, iov_len);
+			size_t left;
+
+			if (!to_user)
+				left = copy_from_user(kaddr, uaddr, copy);
+			else
+				left = copy_to_user(uaddr, kaddr, copy);
+
+			if (unlikely(left))
+				return -EFAULT;
+
+			iov_iter_advance(&ii, copy);
+			todo -= copy;
+			kaddr += copy;
+		}
+
+		kunmap(map);
+	}
+
+	return 0;
+}
+
+/*
+ * For ioctls, there is no generic way to determine how much memory
+ * needs to be read and/or written.  Furthermore, ioctls are ...
[ message continues ]

Why do we need ioctls?  For CUSE?  In that case, would we need to copy
the memory from userspace, into the kernel, and then back out into
userspace again?  Can't we just have a "pass-through" type fixed ioctl
instead?

thanks,

greg k-h
--

[ message continues ]

Can you elaborate a bit?  How the fixed ioctl would know how much to
copy in and out and from where?

Thanks.

-- 
tejun
--

[ message continues ]

If you're worried about the double copying due to performance reasons,
what we can do is implementing userspace to userspace copying.  I just
took easier path of copying twice as u-u copy can't be done using the
existing FUSE mechanics.

-- 
tejun
--

[ message continues ]

How about not copying anything in the kernel, just passing the virtual
address to the filesystem?  It can get/modify the data of the calling
task by mapping /proc/pid/mem.  Ugly, yes, but ioctls are inherently
ugly.

Miklos
--

[ message continues ]

Hmmm... I was trying to stay within similar operation mechanics as other
ops.  Directly accessing the caller's memory has performance benefits
but that benefit can also be used by reads and writes.  So, if we're
gonna do direct memory access, maybe doing it in more generic way is a
better idea?

Thanks.

-- 
tejun
--

[ message continues ]

On the contrary: playing VM games is going to be slow.  I think this
approach is best suited for generic ioctl support because it
simplifies the kernel part.  I'd hate to add all that complexity to
the kernel if not absolutely necessary.

Miklos
--

[ message continues ]

Well, it's only 240 lines with good amount of comments and iovec copying
function.  The ioctl itself isn't too complex.  I'm a bit skeptical
about direct access.  It can easily introduce security vulnerabilities
as there really is no way to hold a pid.

Thanks.

-- 
tejun
--

[ message continues ]

I don't understand.  No new vulnerabilities are introduced, since it
would just use existing infrastructure.

Why is it better if the kernel does the copying of memory regions
instructed by the userspace filesystem, than if the userspace
filesystem does that copying itself?  I feel they are totally
equivalent, except that the latter needs more complexity in the
kernel.

Miklos
--

[ message continues ]

I'm no security expert but it feels pretty dangerous to me.  First of
all, there are cases where the calling process can exit before the
userland FUSE is finished with an operation, so it might not be always
possible for the FUSE client to tell the PID it got is the correct one.

Another thing is that as it currently stands, the kernel side FUSE
implementation forms a nice safety net taking responsibility of most
security concerns and insulating the mistakes the client may make.
Letting userland client to access and possibly modify the caller's
memory directly weakens that insulation.

Pushing memory access to userland feels a bit too risky to me.  There
seem to be too many loose components in security sensitive path and I
have a nagging feeling that someone will come up with something we can't
think of at the moment.

Thanks.

-- 
tejun
--

[ message continues ]

OK, I grant this one.  But then it's easy to protect against by
getting a ref on the task (or just the task ID, I don't know if that's

The same stupid mistakes can be done by giving the wrong instructions
to the kernel about what to modify, thus thrashing the calling

I don't see the difference.  You have to be careful either way, it's
not possible to do ioctls safely as the rest of fuse unfortunately.
This obviously also means, that it's impossible to run the filesystem
as an unprivileged user, as it has to have access to the whole address
space of the calling process either way (or ioctls have to be
restricted somehow).

Miklos
--

[ message continues ]

Yeah, ioctl, by design, has that potential.  Whether it's implemented in
kernel or userland, it's gonna access arbitrary memory regions from deep
down the implementation, and it can corrupt things, but by giving the
responsibility to move data to kernel part of FUSE, we can at least
guarantee that it's only gonna ruin the calling address space even when
it screws up and when that happens it will be easy to track down by

I'm not worried about the client accessing wrong memory regions or even
corrupting it.  It's pointless to try to protect against that.  From the
calling process's POV, it runs the same risk whether it calls an
in-kernel ioctl or a CUSE one and FUSE already has sufficient protection
against allowing unprivileged FS implementation to serve other users.

What I'm worried about is the possibility of CUSE client being able to
break out of that privilege protection which is currently ensured by the
kernel.  Also, what about containers?  How would it work then?

Thanks.

-- 
tejun
--

[ message continues ]

Yes and no.  Fuse allows this protection to be relaxed
(-oallow_other), because it does provide quite good privilege
separation.  This ioctl thing breaks that, so we should disable ioctls
with 'allow_other' or require the filesystem to be privileged.  But
the latter is not easy because mount(2) is always privileged, we don't
know if the process calling fusermount was privileged or not.

So, your current patch actually _introduces_ a security vulnerability

What do you call client?  If you mean the app using the char dev, then

Dunno.  Isn't there some transformation of pids going on, so that the
global namespace can access pids in all containers but under a
different alias?  I do hope somethinig like this works, otherwise it's
not only fuse that will break.

Miklos
--

[ message continues ]

Ah.. right.  allow_other.  Yeah, restricting ioctl implementation only

I first used 'server' for userland [FC]USE server but then I noticed
there were places in FUSE they were referred as clients so now I use
'client' for those and call the app using the FUSE fs the 'caller'.
What are the established terms?

Anyways, doing it directly from the server (or is it client) opens up a
lot of new possibilities to screw up and I'd really much prefer staying
in similar ballpark with other operations.  Maybe we can restrict it to
two stages (query size & transfer) and linear consecutive ranges but
then again adding retry doesn't contribute too much to the complexity.
Oh.. and BTW, the in-ioctl length coding is not used universally, so it

I'm not sure either.  Any idea who we should be asking about it?

Thanks.

-- 
tejun
--

[ message continues ]

Umm

 - userspace filesystem
 - filesystem daemon
 - filesystem process
 - server


I know it's not universal, some horrors I've seen in the old wireless
interfaces.  The question is: do we want to support such "extended"

Serge Hallyn and Eric Biederman.

Miklos
--

[ message continues ]

OSS ioctls are all pretty simple and I think they all use the proper
encoding.  For the question, my answer would be yes (naturally).  It
will suck later when implementing some other device only to find out
that there's this one ioctl that needs to dereference a pointer but
there's no supported way to do it but everything else works.

I don't think the performance or the complexity of specific ioctl
implementation is of the determining importance as long as it can be
made to work with minimal impact on the rest of the whole thing, so

Okay, cc'd both.  Hello, Eric Biederman, Serge Hallyn.  For
implementing ioctl in FUSE, it's suggested that to access the address
space of the caller directly from the FUSE server using its pid via
/proc/pid/mem (or task/tid/mem).  It's most likely that the calling
process's tid will be used.  As I don't know much about the
containers, I'm not sure how such approach will play out when combined
with containers.  Can you enlighten us a bit?  W/o containers, it will
look like the following.


          FUSE ----------------
           ^                   |
           |                   |        kernel
  ------ ioctl ----------- /dev/fuse ------------
           |                   |       userland
           |                   v
    ---------------      -------------
   | caller        |    | FUSE server |---> reads and writes
   | with tid CTID |    |             |    /proc/PID/task/TID/mem
    ---------------      -------------

The FUSE server gets task->pid.  IIUC, if the FUSE server is not in a
container, task->pid should work fine whether the caller is in
container or not, right?  And if the FUSE server is in a container,
it's hell lot more complex and FUSE may have to map task->pid to what
FUSE server would know if possible?

Thanks.

-- 
tejun
--

[ message continues ]

Implementation wise it is not too bad.

           FUSE ----------------
pid = get_pid(task_tid(current))
            ^                   |
            |                   |        kernel
                          pid_vnr(pid)
   ------ ioctl ----------- /dev/fuse ------------
            |                   |       userland
            |                   v
     ---------------      -------------
    | caller        |    | FUSE server |---> reads and writes
    | with tid CTID |    |             |    /proc/PID/task/TID/mem
     ---------------      -------------

However it is a largely an insane idea.
- Write is not implemented for /proc/PID/task/TID/mem
- It would be better if the kernel handed you back a file descriptor to the
  other process memory rather than you having to generate one.
- To access /proc/PID/task/TID/mem you need to have CAP_PTRACE.
- This seems to allow for random ioctls.  With the compat_ioctl thing we have
  largely stomped on that idea.  So you should only need to deal with well
  defined ioctls.  At which point why do you need to directly access the memory
  of another process.

So why not just only support well defined ioctls and serialize them in the kernel
and allow the receiving process to deserialize them?

That would allow all of this to happen with a non-privileged server which
makes the functionality much more useful.

Given the pain it is to maintain ioctls I would be very surprised if we wanted
to open up that pandoras box even wider by allowing arbitrary user space
processes to support random ioctls.  How would you do 32/64bit support
and the like?

Eric
--

[ message continues ]

I think that is not too much of a problem: Just like the file operations in
the kernel have two callbacks (ioctl and compat_ioctl), you need to provide
both operations from user space if there is a difference between them, or
at least a stub otherwise.

	Arnd <><
--

[ message continues ]

Hello, Eric.



Yeah, that would at least ensure we're not meddling with the wrong

What do you mean by "compat_ioctl" thing?  We still allow random


When !allow_others, there's no reason to prevent one user's FUSE
server to corruption the user's processes.  Everyone has the innate

32/64bit is not really much problem tho.  The kernel just flags the
request as-such and let the userland deal with it.

Miklos, I think Eric's reply pretty much blew the direct access idea
out of water, which leaves us with three options.

1. Support only the proper ioctls.  This would be the simplest but
   someone might run into its limitations in the future.  Given that
   the most probable use cases for CUSE are replacing in-kernel legacy
   code or faking some proprietary interfaces which are likely to be
   somewhat weird, the chance isn't too low IMHO.

2. Do what hpa suggested, that is a simple language to describe the
   needed data regions.  To me, it seems like an overkill.  If there's
   already such infrastructure in kernel, I would be happy to
   piggyback on it but separate serialization language for FUSE ioctl
   support seems extreme.

3. And my favorite (obviously), keep the current implementation.  Ugly
   it may look but given the horror of the problem at hand I think it
   actually hits a pretty good balance between interface peculiarness
   and complexity that a more elegant solution would require.  It's
   only ~250 lines of code which can support any ioctl.  Complex ones
   will have to go through a few duplicate copy operations but I don't
   really think those are gonna hurt anyone when the whole thing is
   being relayed to userland.

So, what do you think?

Thanks.

-- 
tejun
--

[ message continues ]

Ahh, thanks.  I'll need to fix this up then, regardless of any ioctl
issues, so that the tid supplied to the userspace filesystem actually



Yes, but access to the other process's address space requires some
sort of privilege anyway.  It would not do to have an unprivileged
process peek at arbitrary addresses in the other process's memory, and

I'd like the idea of limiting to well behaved ioctls, but Tejun

There's still a security issue, because we cannot verify *if* a
particular ioctl is indeed well behaved: only the application and the
driver knows that, and the application cannot tell us (ioctl interface
is broken, broken, broken), and we don't trust the server.

Thanks,
Miklos
--

[ message continues ]

Hello,


I'm not dead against it.  I'm just a bit more inclined to my
implementation (naturally), which means if you're dead against the
current implementation, supporting only the proper ones definitely is
an option, but comparing the pros and cons, I'm not quite convinced
yet.

Thanks.

-- 
tejun
--

[ message continues ]

I really think that if an ioctl is passing through the kernel we
should know how to parse and understand it's options.  Otherwise
we won't have the option of doing backwards compatibility when something
changes, like we can with the 32->64bit ioctls.

That seems to imply that you need a stub in the kernel to handle 
really weird ioctls.

The upside is that because you know what the inputs and outputs are
and where the inputs and output are you can support that ioctl well
into the future, and you can do it with an unprivileged file
system server.

Eric
--

[ message continues ]

There's no reason 32->64bit can't be handled in userland?  What's the

Well, kernel stub kind of beats a lot of benefits of FUSE - no
specific kernel dependencies, easy development and distribution,
etc...

Thanks.

-- 
tejun
--

[ message continues ]

Maintenance.  What happens if I go 128bit, if I have some processes
that are big endian and some that are little endian.  Or if I have
some processes that are running a completely different instruction
set with a completely different ABI than other processes.  Or
perhaps different perhaps the processes is in a different network
namespace than your filesystem and so it's arguments refer
to something different entirely.  Is it a userspace bug if userspace
does not anticipate how the kernel will change in the future?

If we don't look at ioctl as a set of system calls that should
be put into an appropriate format for a filesystem we have
a maintenance problem.

If we don't have an interface clean enough we can push data
out to a server on a remote machine have it processes the
arguments and send the data back.  We actually have failed

Of course FUSE has specific kernel dependencies.  It depends
on the implementation of fusefs in the kernel to talk to it.
The reason you don't need a specific kernel today is that
the kernel dependencies are well defined.  You are talking
about using a very poorly defined interface to talk to the
filesystem.  At which point it would be better to open
a separate channel and talk to the filsystem directly.

Being able to add a kernel system call (ioctl) with no review is a
total maintenance disaster.  It is impossible to maintain because
there is not a process to even discover what is going on.

We have to have a kernel stub to support other system calls
and I don't see why individual ioctls should be different.

If you want to support forwards compatibility reserving
some ioctl numbers and saying these numbers will always
be parsed this way.  Which would allow you to write
a common stub that can be implemented before the ioctls
are implemented.

If you really don't want new kernel dependencies you can hook up to
the process via ptrace and intercept the ioctls before they even get
to the kernel.  If you can open /proc/<pid>/mem you have the ...
[ message continues ]

What I don't see is how anything becomes different whether that part
is implemented in kernel or not.  For ioctls, only the implementing
code knows what the data structure should look like that's why we
can't handle 64/32 bit problems generically and has ->compat_ioctl
hook.  To me, splitting it to two pieces looks like more maintenance

I don't think that's a possible goal in generic manner.  We can't even
do that for read/writes.  At least not at FUSE's level.  We need
higher abstraction for that.  e.g. /dev/oss via CUSE, ioctl specified
machine endian 16bit format.  Supporting that in the way you described
would require a lot of logic in the kernel and I don't really see any
benefit over, say, forward the sound via pulseaudio after getting it
to userland.  I don't think the problem can or should be solved at

The difference is that FUSE servers depend on FUSE and each server
doesn't require separate new pieces to get working and ioctl is poorly

How about adding whole FSes w/o a review?  That's the whole point of
FUSE.  Seriously, there are infinite number of ways to break the
regular defined filesystem semantics using FUSE.  FUSE should provide
mechanism and protects against certain fundamental things.  It can't

ioctls is most likely to be used to emulate legacy or proprietary
devices.  If you're worried about filesystems abusing it, maybe the
solution is to only allow CUSE to use ioctls but I really don't see
what the problem is here.  If the userland FS server wants to shoot

Aieee.. The same goes for the whole FUSE.

Thanks.

-- 
tejun
--

[ message continues ]

One thing to bear in mind is the possibility of 32-bit client programs 
running on a 64-bit platform, so your user-space driver will need be be 
aware of the flavour of the client, and parse the ioctl arguments (and 
in particular, pointers) appropriately.

That's true of a kernel implementation too, of course.

--
Roger



--

[ message continues ]

Yeap, for 32bit client on 64bit machine, FUSE_IOCTL_COMPAT will be set.
 For OSS emulation, it doesn't make any difference tho.  It's the same
for both word sizes.

Thanks.

-- 
tejun
--

[ message continues ]

Agreed entirely. If and only if there is a future problem with
performance someone can introduce a method for the client to pre-describe
certain ioctls in terms of number/length/read/write.

Alan
--

[ message continues ]

If you're worried about the double copying due to performance reasons,
what we can do is implementing userspace to userspace copying.  I just
took easier path of copying twice as u-u copy can't be done using the
existing FUSE mechanics, but frankly, at this point, I think that would
be an premature optimization.

-- 
tejun
--

[ message continues ]

No, I'm not worried about the performance, just that this should be
simple as we can just pass "arg" to userspace without touching it as it
just came from userspace, right?

Oh wait, there are process space issues at play here that I'm totally
forgetting about, right?

thanks,

greg k-h
--

[ message continues ]

For simple ones, that's how it's gonna be handled.  Those copying in and
out kicks in when the @arg is pointer or worse data structure which can

Thanks.

-- 
tejun
--

[ message continues ]

Okay, I'm going to say it... wouldn't it be better to have some kind of 
data structure description language which the userspace can register 
with the kernel to linearize the data on a per-ioctl basis (kind of like 
rpcgen)?

	-hpa
--

[ message continues ]

Ah.... funky.  If this retry thing is too repulsive, I guess the best
alternative would be directly accessing caller's memory as Miklos suggested.

Thanks.

-- 
tejun
--

[ message continues ]

Be careful -- there are some serious dragons there in the presence of 
multiple threads.

	-hpa
--

[ message continues ]

OK, it should map /proc/pid/task/tid/mem.  Or rather
/proc/tid/task/tid/mem, as the pid (tgid) of the caller is not
currently passed to the filesystem.

Miklos
--

[ message continues ]

Uhm, no.  You can still have it change underneath you as long as you 
have any thread of execution with access to the same memory.

This is *hard* to get right, and we screw this up in the kernel with 
painful regularity.  The throught of having user-space processes, which 
don't have access to the kernel locking primitives and functions like 
copy_from_user() dealing with this stuff scares me crazy.

That is why I'm suggesting using an in-kernel linearizer.

	-hpa
--

[ message continues ]

Lots of complexity, ugh...  Even Tejun's current scheme is better IMO.

Miklos

--

[ message continues ]

Memory changing underneath you.  It can be dealt with by very careful 

And then you get *no* privilege separation, for one thing, so why even 
bother doing it in userspace?

	-hpa
--

[ message continues ]

That's just handwaving.  Apps don't normally change memory under
system call arguments.  Or if they do the only thing we ever guarantee
is that the thing won't blow up in a big fireball.

I don't see how getting the data from userspace is different from

And with ioctls (at least if the filesystem supplies the linearizer
instructions) you simply _cannot_ get proper privilege separation.
Generic ioctl support will always be a privileged thing.

Alternatively we can restrict ioctls.  Most ioctls conform to some
convention for encoding the format (size/in/out) in the command, no?

Miklos
--

[ message continues ]

Guessing that CUSE stands for "Character Devices in Userspace"?

Is there a description of this?  It's for sound device emulation,
right?

What is direct-lseek for?  It doesn't sound like a feature needed by
char devices.

Thanks,
Miklos
--

[ message continues ]

I did it for OSS emulation but it can be used for anything.  Hmm... I'm
still in the process of pushing out patches, so please wait just a bit.
 The libfuse changes include simple examples so that should make things

Sound device just sets nonseekable.  The direct lseek bit is for API
completeness as there are chardevs which have special semantics
regarding lseek.

Thanks.

-- 
tejun
--

[ message continues ]

Hello, Miklos.

I'm about ready to send the next round of this patchset w/ mmap support,
which I'm sure gonna stir up some discussion. :-) Several things I wanna
hear your comments about.

1. I'll try to incorporate all other comments here but regarding ioctl I
don't think we've reached any better solution, so I'm sticking with the
original one.

2. You told me that the version branching in the userland library wasn't
necessary.  Can you explain to me when FUSE version bumping is necessary?

3. Any other things on you mind?

Thanks.

-- 
tejun
--

[ message continues ]

Hi Tejun,


Great.  Just yesterday evening I was looking through your patches to
see which ones I can submit for 2.6.28, and I've already applied

  0001-FUSE-add-include-protectors.patch
  0003-FUSE-implement-nonseekable-open.patch

Comments about the others:

  0002-FUSE-pass-nonblock-flag-to-client.patch

this is not needed, f_flags are already passed to userspace for read
and write.

  0004-FUSE-implement-direct-lseek-support.patch

this is trickier to get the interface right I think.  If we want to
allow filesystems to implement a custom lseek, then we also want them
to keep track of the file position, which means we must differentiate
between a write(2) and a pwrite(2) and similarly for reads.  AFAICS
this isn't needed for CUSE so we can leave this to later.

  0005-FUSE-implement-ioctl-support.patch

See below.

  0006-FUSE-implement-unsolicited-notification.patch
  0007-FUSE-implement-poll-support.patch

This would be nice, but...  I don't really like the fact that it uses
the file handle.  Could we have a separate "poll handle" that is

I still got qualms about this ioctl thing.  One is the security
aspect, but that could be dealt with.  The other is that I really
really don't want people to start implementing new custom ioctls for
their filesystems, as I think that way lies madness.  We could limit
ioctls to CUSE and that would be fine with me.  Or for non-CUSE users
we could enforce the "standard" format where the type and length is
encoded in the command number.

I don't have any problems with the iterative way you implemented
ioctls.  We just need some additional restrictions to the current

The version number has to be bumped anyway.  But if you are only
adding new functions to the end of fuse_operations and
fuse_lowlevel_ops, then the interface can handle that, without needing

One other thing I was thinking about is that do we really need
emulated char devices to be char devices?  What I mean is, what would
happen if instead of a char device ...
[ message continues ]

Is this enough to support the most useful/requested file system ioctls, 
like FIEMAP?

Thanks,
	   Szaka

-- 
NTFS-3G:  http://ntfs-3g.org


--

[ message continues ]

Looking at FIEMAP patches...

No, FIEMAP doesn't seem fit in that category.  But FIEMAP has a
specific handler, so it would not get as far as the generic ioctl
handler anyway.  If we need FIEMAP support in fuse, that will need to
be handled independently of the generic ioctls (just like FIBMAP).

Miklos
--

[ message continues ]

Hello, Miklos.  Sorry about the extra long delay.  I was buried alive
under bugs and regressions with SLE11 release date nearing and all.



Hmmm... I'll try to find out whether I can use f_flags.  There was

Read/write already passes @offset, so the only thing required is an
extra flag there.  I mainly wanted a way for a CUSE server to veto lseek
with proper error and still think it's better to have this as we don't
really know what wacky users are out there.  What do you think about an


For now, I'll limit ioctl to CUSE.  Hmmm... Yeah, limiting ioctl to



For most it would work, I suppose, but there are all sorts of wonky
users out in the wild (and quite a few that we don't have source access
to) and different configurations, so I think it's better to appear as
proper character device if it is a character device.  It will also help
udev and other desktop thingies deal with devices implemented in userland.

Thanks.

-- 
tejun
--

[ message continues ]

Note that mounting a char device on /dev/dsp is exactly the same as
mounting a regular file on /dev/dsp - the only difference is what you
set i_mode to.

--

[ message continues ]

A difference is how the device is located.  With proper character device
emulation, any char device node on any filesystem would work.  Not sure
how relevant that would be tho.

Thanks.

-- 
tejun
--

[ message continues ]

Yeah, I think it would be a bit hacky to lie about being a device with
a specific major/minor number, when in fact we just set those in the
stat fields, and the real char device would return ENODEV or implement
something else.

So I'd prefer the CUSE approach if we want the file to actually look
like a device.

Miklos
--

[ message continues ]

You're talking about fusd, here.
http://www.circlemud.org/~jelson/software/fusd/

Mike
--

[ message continues ]

Support for this was missing from libfuse, but now I fixed that in the

OK, but that's gonna involve a fair bit of API churn, and I'm not sure
it's worth it at this stage.  If this is not needed for the OSS
emulation, I think we should postpone it.

Thanks,
Miklos
--

[ message continues ]

Alright.

Thanks.

-- 
tejun
--

[ message continues ]

I've been thinking about this a bit more.  What do you think about
putting the following restrictions?

1. FUSE server can only support well-formed ioctls.  At the kernel side,
the interfaces remains the same for both FUSE and CUSE but libfuse only
exports well-formed ioctl API.

2. ioctl can only be used by FUSE server running as root (would this be
necessary?  I'm not sure.  To me it seems all the necessary protections
are already there).

Thanks.

-- 
tejun
--

[ message continues ]

Not with '-oallow_other'.  Consider the case that the caller invoked a
non well formed ioctl, but since there's no way to know this we
allowed the fuse server to tinker with the caller's address space
_as if_ the ioctl was well formed.

So we should always make sure that the server has enough privilege to
read/write the caller's memory, i.e. it can ptrace the caller.

At this point we could allow any ioctls, not just well formed ones.
But I don't want that for a different reason: if the possibility is
there people will find new "innovative" uses for it and just get
themselves into a big mess.

Miklos
--

[ message continues ]

Hello,



I don't really mind people doing strange things in userland as long as
it's safe but you're the maintainer.  It's a bit strange to export the
feature only for CUSE, so I'm a little bit hesitant.  I wanna make it
useful for both.  So, at the kernel level, only well formed for FUSE and
everything goes for CUSE.  Does that sound good enough?

-- 
tejun
--

[ message continues ]

You are starting from the fact that ioctl is a good API.  It's not,

With additional restrictions for ptraceability yes.  But if you just
restrict it to CUSE at first, that's fine by me as well :)

Thanks,
Miklos
--

[ message continues ]

Heh heh, I don't think ioctl is a good API but it's fun to watch people

It actually wasn't a big change.  Now only well formed ioctls are
allowed for FUSE server and kernel will dispatch the correct amount of
input data and feed back the correct amount of output data without any
retry.  With proper massaging of FUSE ioctl API, ioctl, at least for
FUSE, will be very straight forward.

Thanks.

-- 
tejun
--

[ message continues ]

Hello, Miklos.


Eh... I replied too early for this.  I'm now trying to convert it to its
own handle but there is a rather serious problem.  It's usually much
easier to have the entity to be waken up registered before calling
->poll so that ->poll can use the same notification path from ->poll ans
for later.

However, if we allocate poll handle from ->poll and tell it to kernel
via reply, it creates two problem.  1. the entity which is to be waken
up can't be registered prior to calling ->poll as there's nothing to
identify it, 2. the interval from reply write and in-kernel polled
entity registration must be made atomic so that no notification can come
through inbetween.  #1 means that ->poll can't call the same
notification path from ->poll itself and #2 means that there needs to be
special provision from dev.c::fuse_dev_write() to
file.c::fuse_file_poll() so that atomicity can be guaranteed.  Both of
which can be done but I'm not really sure whether using a separate
handle would be a good idea even with the involved cost.

Why do you think using separate poll handle would be better?  And do you
still think the overhead is justifiable?

Thanks.

-- 
tejun
--

[ message continues ]

Because it would be a change in the semantics of the file handle.
Previously it was just an opaque cookie that the kernel stored for the
filesystem, not making any assumptions about it (like uniqueness).

OK, we can say that if the filesystems wants to implement poll, it has
to make the file handle unique.  Also now the filesystem (or
something) has to deal with races between poll notification and
reuse of the file handle (release/open).

With a new poll handle we'd have more room to properly deal with these
without overloading the file handle with extra requirements.

How about this: the poll handle is allocated by the kernel, not by the
filesystem.  This guarantees uniqueness, so the filesystem cannot get
this wrong.  Releasing the poll handle is still tricky, there could be
various races... only the userspace filesystem knows if it has no
outstanding notificiatons on a poll handle, so the release has to come
after all outstanding notifications have been ack'ed.  Something like
this:

(userspace <- kernel)

<- POLL-request(pollhandle) (alloc handle)
-> POLL-reply
...
-> POLL-notification(pollhandle)
<- POLL-ack
...
<- POLL_RELEASE(pollhandle)
-> POLL_RELEASE-reply (free handle)

Thanks,
Miklos
--

[ message continues ]

Hello,


Hmm... yeah, allocating handle from kernel should work fine, but I
wouldn't worry about race here.  We can just use 64 bit and guarantee
that any handle won't be reused ever.

Thanks.

-- 
tejun
--

[ message continues ]

Right, that sounds perfect.

Thanks,
Miklos
--

[ message continues ]

Btw, the protocol could be simplified even more by getting rid of all
acknowledgements:

<- POLL-request(pollhandle) (alloc handle)
...
-> POLL-notification(pollhandle)
...
<- POLL_RELEASE(pollhandle)

So normally ->poll() wouldn't have to sleep at all.  If the POLL
request fails for some reason, userspace just sends a notification
with the respective error.

Miklos
--

[ message continues ]

Hi,


We don't need POLL_RELEASE but we still need POLL-reply (to request) to
send revents.  We can put that into notification too.  Hmmm... Yeah,
that could be simpler for FUSE servers.  I'll venture that way.

Thanks.

-- 
tejun
--

[ message continues ]

What happens on timeout?  Do we just let userspace continue polling

Then in fact the notification could just become the reply:

<- POLL-request (sent with request_send_nowait())
...
-> POLL-reply  (calls req->end())

So there won't even be a need to implement notification (we'll need
that for other things in the future) simplifying things even further.
Even if we want to cancel the request because of a timeout, that could
be done with the existing INTERRUPT request.

Miklos
--

[ message continues ]

poll/select/epoll can poll on massive number of files.  I don't think
it's wise to have that many outstanding requests.  FUSE currently uses
linear list to match replies to requests and libfuse will consume one
thread per each poll if implemented like other requests.  It can be made
asynchronous from libfuse tho.

I kind of like the original implementation tho.  The f_ops->poll
interface is designed to be used like ->poll returning events if
available immediately and queue for later notification as necessary.
Notification is asynchronous and can be spurious (this actually comes
pretty handy for low level implementation).  When notified, upper layer
queries the same way using ->poll.  This is quite convenient for low
level implementation as the actual logic of poll can live in ->poll
proper while notifications can be scattered around places where events
can occur.

Thanks.

-- 
tejun
--

[ message continues ]

Yes, that kind of interface is nice for f_ops->poll, and for libfuse.

But for the kernel interface it's inefficient.  A wake up event is 3
context switches instead of one.  And that's inherent in the interface
itself for no good reason.

Also there's again the question of userspace filesystem messing with
the caller: your original implementation allows the userspace
filesystem to block f_ops->poll() forever, which really isn't what
poll/select is about.

So I'd still argue for the simple POLL-request/POLL-notify protocol on
the kernel API, and possibly have the async notification similar to
the kernel interface on the library API.

Implementation wise I don't care all that much, but I'd actually
prefer if it was implemented using the traditional request/reply thing
and optimized (possibly later) to find requests in a more efficient
way than searching the linear list, which would benefit not just poll
but all requests.

Miklos
--

[ message continues ]

Hello,


Event notification performance problem is usually in its scalability
not in each notification.  It's nice to optimize that too but I don't
think it weighs too much especially for FUSE.  Doing it request/reply

That would simply be a broken poll implementation just as O_NONBLOCK

Given that the number of in-flight requests are not too high, I think
linear search is fine for now but switching it to b-tree shouldn't be
difficult.

So, pros for req/reply approach.

* Less context switch per event notification.

* No need for separate async notification mechanism.

Cons.

* More interface impedence matching from libfuse.

* Higher overhead when poll/select finishes.  Either all outstanding
  requests need to be cancelled using INTERRUPT whenever poll/select
  returns or kernel needs to keep persistent list of outstanding polls
  so that later poll/select can reuse them.  The problem here is that
  kernel doesn't know when or whether they'll be re-used.  We can put
  in LRU-based heuristics but it's getting too complex.  Note that
  it's different from userland server keeping track.  The same problem
  exists with userland based tracking but for many servers it would be
  just a bit in existing structure and we can be much more lax on
  userland.  ie. actual storage backed files usually don't need
  notification at all as data is always available, so the amount of
  overhead is limited in most cases but we can't assume things like
  that for the kernel.

Overall, I think being lazy about cancellation and let userland notify
asynchronously would be better performance and simplicity wise.  What
do you think?

Thanks.

-- 
tejun
--

[ message continues ]

Why not just link the outstanding poll requests into a list anchored

Lazy cancellation (no cancellation, esentially) sounds good.  But that
works fine with the simplified protocol.

Think of it this way, this is what a poll event would look like with
your scheme:

1) -> POLL-notification
2) <- POLL-req
3) -> POLL-reply (revents)

Notice, how 1) and 2) don't carry _any_ information (the notification
can be spurious, the events in the POLL request is just repeated from
the original request).  All the info is in 3), so I really don't see
any reason why the above would be better than just omitting the first
two steps.

Miklos
--

[ message continues ]

Hello,



Alrighty then.  I'll convert it.

Thanks.

-- 
tejun
--

[ message continues ]

Hello, Miklos.

I tried to implement poll as you suggested but it doesn't work because
poll actually is synchronous.  Please consider the following scenario.
A file system implements a file which supports poll and other file
operations and there's a single threaded client which does the
following.

1. open the file
2. do polling (timeout == 0) poll on the fd
3-1. if POLLIN, consume data and goto #2
3-2. if ! POLLIN, do a ioctl (or whatever) on the fd and goto #2

For a client with single stream of syscalls (single threaded), it's
generally guaranteed that the attempt to consume data is successful
after POLLIN unless the fd dies inbetween.  I don't think this is
something guaranteed in POSIX but for most in-kernel poll
implementations, this holds and I've seen good amount of code
depending on it.

To satisfy the above assumption, if ->poll is always asynchronous,
FUSE has to cache revents from previous ->poll attempts and clear it
when something which could have consumed data has occurred.
Unfortunately, in the above case, FUSE has no idea what constitutes
"consume data" but, double unfortunately, it can't take big hammer
approach (clearing on any access) either, because intervening non-data
consuming call like 3-2 above would mean that poll() will never
succeed.

Because data availability should be determined atomically && only the
filesystem knows when or how data availability changes, revents return
from ->poll() must be synchronous.

We can still use req -> reply approach where there's a flag telling
the FUSE server whether the request is synchronous or not but at that
point it seems just obfuscating to me.

So, ->poll() needs to be the combination of synchronous data
availability check + asynchronous notification which can be spurious
to implement the required semantics and I think the original interface
was much more natural for such functionality.

What do you think?

Thanks.

-- 
tejun
--

[ message continues ]

Hi Tejun,



OK, lets do it with the original interface.  There's still room for
optimization, though, because the _normal_ operation of poll() is
absolutely asynchronous.  I think the 'flags' field in poll_in should
be adequate to make the interface extensible in the future.

Thanks,
Miklos
--

[ message continues ]

Hmm... I'm not sure I understand what you're saying but if you're
talking about optimizing async case where notification occurs while the
poller is sleeping, I don't think it really matters.  That could be
common but they're not performance sensitive path.  As select/poll users
become busy, ->poll operation becomes more and more synchronous.

If the client is using better interface like epoll, sending revents via
notification could help a bit but again the problem is that the ->poll
vfs interface is not ready for that.  e.g. How do you tell whether
->poll is for epoll polling after the notification or an asynchronous
poll(2) being called after a read(2) after the notification arrived.
There simply isn't enough information to determine when the cached
revents (no matter how short the period of caching is) can be used or
not.  One thing we can do is to invalidate the received revents value on
every file operation and then reverting to synchronous call only when
cached revents is not available, but I don't really see good
justifications for such over complexity.

Thanks.

-- 
tejun
--

[ message continues ]

If poll does have to sleep (however short), then it's alwasy more
synchronous than necessary.  It introduces an extra latency of two
context switches per poll.  Yes, this means the performance hit will

It's an optimization, and not a very complex one at that.  But yes, we
should leave that till later, when the simple interface prooved itself
working.

Miklos
--

[ message continues ]

Hello,


If we're talking about poll(2) and select(2), in many cases, they don't
sleep at all as the load nears its limit due to the cost for scanning
idle fds.  Those apps usually reach 100% before hitting their bandwidth
limits (no sleep at all) and until they hit the bandwidth limit only the
overhead of scanning idle fds decreases.

What I was trying to say was that for poll(2) and select(2), the
optimization would be a bit empty as they're not gonna help any with
increasing load.  Optimization would still be helpful but cost/benefit

Yeap, we can cache @revents per file and clear it on any operation on
the file but as you said let's leave it for the day when it actually
comes biting.

Thanks.

-- 
tejun
--

[ message continues ]