All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fuse: Don't drop NOTIFY_REPLY if we promised it
@ 2019-02-12 20:04 Kirill Smelkov
  0 siblings, 0 replies; only message in thread
From: Kirill Smelkov @ 2019-02-12 20:04 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-fsdevel, Kirill Smelkov, Han-Wen Nienhuys,
	Jakob Unterwurzacher, stable

A successful call to NOTIFY_RETRIEVE by filesystem carries promise from
the kernel to send back NOTIFY_REPLY message. However if the filesystem
is not reading requests with fuse_conn->max_pages capacity,
fuse_dev_do_read might see that the "request is too large" and decide to
"reply with an error and restart the read". "Reply with an error" has
underlying assumption that there is a "requester thread" that is waiting
for request completion, which is true for most requests, but is not true
for NOTIFY_REPLY: NOTIFY_RETRIEVE handler completes with OK status right
after it could successfully queue NOTIFY_REPLY message without waiting
for NOTIFY_REPLY completion. This leads to situation when filesystem
requested to retrieve inode data with NOTIFY_RETRIEVE, got err=OK for
that notification request, but NOTIFY_REPLY is not coming back.

More, since there is no "requester thread" to handle the error, the
situation shows itself as /sys/fs/fuse/connections/X/waiting=1 _and_
/dev/fuse read(s) queued. Which is misleading since NOTIFY_REPLY request
was removed from pending queue and abandoned.

One way to fix would be to change NOTIFY_RETRIEVE handler to wait until
queued NOTIFY_REPLY is actually read back to the server and only then
return NOTIFY_RETRIEVE status. However this is change in behaviour and
would require filesystems to have at least 2 threads. In particular a
single-threaded filesystem that was previously successfully using
NOTIFY_RETRIEVE would become stuck after the change. This way of fixing
is thus not acceptable.

However we can fix it another way - by always returning NOTIFY_REPLY
irregardless of its original size - with so much data as provided read
buffer could fit. This aligns with the way NOTIFY_RETRIEVE handler
works, which already unconditionally caps requested retrieve size to
fuse_conn->max_pages. This way it should not hurt NOTIFY_RETRIEVE
semantic if we return less data than was originally requested.

This fix requires another behaviour change however - to be sure that
read buffer has enough capacity to always fit fixed NOTIFY_REPLY part
plus at least some (0 or more) data, we have to precheck the buffer
before dequeuing and handling a request. And if the buffer is very small -
return EINVAL to read in filesystem with semantic that queued read was
invalid from the viewpoint of FUSE protocol. Even though this is also
behaviour change, this should not practically cause problems: 1d3d752b47
(fuse: clean up request size limit checking), which originally removed
such EINVAL return and reworked fuse_dev_do_read to loop and retry, also
added FUSE_MIN_READ_BUFFER=8K to user-visible fuse.h with comment that
"The read buffer is required to be at least 8k ..." Even though
FUSE_MIN_READ_BUFFER is not currently checked anywhere in the kernel,
libfuse always initializes session with bufsize=32·pages and, since its
beginning, (at least from 2005) issues a warning should user modify
fuse_session->bufsize directly to be sure that queued buffers are at
least as large as that sane minimum:

	https://github.com/libfuse/libfuse/blob/fuse-3.3.0-22-g63d53ecc3a/lib/fuse_lowlevel.c#L2869
	https://github.com/libfuse/libfuse/blob/fuse-3.3.0-22-g63d53ecc3a/lib/fuse_lowlevel.c#L1947
	(semantic added in https://github.com/libfuse/libfuse/commit/044da2e9e0)

This way we should be safe to add the check for minimum read buffer size.

I've hit this bug for real with my filesystem that is using
https://github.com/hanwen/go-fuse: there was no NOTIFY_REPLY after
successful NOTIFY_RETRIEVE and the filesystem was stuck waiting,
because FUSE protocol (definition scattered through many places) states
that NOTIFY_REPLY is guaranteed to come after successful NOTIFY_RETRIEVE
(see 2d45ba381a "fuse: add retrieve request"). After inspecting
/sys/fs/fuse/connections/X/waiting and seeing it was 1, I was initially
suspecting that it was user-space who is not issuing /dev/fuse reads and
NOTIFY_REPLY is there but stuck in kernel pending queue. However tracing
what is going on in /dev/fuse exchange and in both kernel and userspace
(see https://lab.nexedi.com/kirr/wendelin.core/blob/13d2d1f8/wcfs/fusetrace)
showed that there are correctly queued /dev/fuse reads still pending
after NOTIFY_RETRIEVE returns and it is the kernel who is not replying back:

	...

	P2 2.215710 /dev/fuse <- qread      wcfs/11399_4_r:

	        syscall.Syscall+48
	        syscall.Read+73
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest.func1+85
	        github.com/hanwen/go-fuse/fuse.handleEINTR+39
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest+355
	        github.com/hanwen/go-fuse/fuse.(*Server).loop+107
	        runtime.goexit+1

	P2 2.215810 /dev/fuse -> read       wcfs/11399_4_r:
	        .56  RELEASE i8 ...             (ret=64)

	P2 2.215859 /dev/fuse <- write      wcfs/11399_5_w:
	        .56 (0) ...

	        syscall.Syscall+48
	        syscall.Write+73
	        github.com/hanwen/go-fuse/fuse.(*Server).systemWrite.func1+76
	        github.com/hanwen/go-fuse/fuse.handleEINTR+39
	        github.com/hanwen/go-fuse/fuse.(*Server).systemWrite+931
	        github.com/hanwen/go-fuse/fuse.(*Server).write+194
	        github.com/hanwen/go-fuse/fuse.(*Server).handleRequest+179
	        github.com/hanwen/go-fuse/fuse.(*Server).loop+399
	        runtime.goexit+1

	P2 2.215871 /dev/fuse -> write_ack  wcfs/11399_5_w (ret=16)

	P2 2.215876 /dev/fuse <- qread      wcfs/11399_5_r:    <-- NOTE

	        syscall.Syscall+48
	        syscall.Read+73
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest.func1+85
	        github.com/hanwen/go-fuse/fuse.handleEINTR+39
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest+355
	        github.com/hanwen/go-fuse/fuse.(*Server).loop+107
	        runtime.goexit+1

	P0 2.221527 /dev/fuse <- qread      wcfs/11401_1_r:    <-- NOTE

	        syscall.Syscall+48
	        syscall.Read+73
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest.func1+85
	        github.com/hanwen/go-fuse/fuse.handleEINTR+39
	        github.com/hanwen/go-fuse/fuse.(*Server).readRequest+355
	        github.com/hanwen/go-fuse/fuse.(*Server).loop+107
	        runtime.goexit+1

	P1 2.239384 /dev/fuse -> read       wcfs/11398_6_r:	# woken read that was queued before "..."
	        .57  READ i5 ...                (ret=80)

	P0 2.239626 /dev/fuse <- write      wcfs/11397_0_w:
	        NOTIFY_RETRIEVE ...

	        syscall.Syscall+48
	        syscall.Write+73
	        github.com/hanwen/go-fuse/fuse.(*Server).systemWrite.func1+76
	        github.com/hanwen/go-fuse/fuse.handleEINTR+39
	        github.com/hanwen/go-fuse/fuse.(*Server).systemWrite+931
	        github.com/hanwen/go-fuse/fuse.(*Server).write+194
	        github.com/hanwen/go-fuse/fuse.(*Server).InodeRetrieveCache+764
	        github.com/hanwen/go-fuse/fuse/nodefs.(*FileSystemConnector).FileRetrieveCache+157
	        main.(*BigFile).invalidateBlk+232
	        main.(*Root).zδhandle1.func1+72
	        golang.org/x/sync/errgroup.(*Group).Go.func1+87
	        runtime.goexit+1

	P0 2.239660 /dev/fuse -> write_ack  wcfs/11397_0_w (ret=48)

	# stuck
	# (full trace: https://lab.nexedi.com/kirr/wendelin.core/commit/96416aaabd)

with queued / served read analysis confirming that two reads were indeed queued
and not served:

	grep -w -e '<- qread\>' y.log |awk {'print $6'} |sort >qread.txt
	grep -w -e '-> read\>'  y.log |awk {'print $6'} |sort >read.txt

	# xdiff qread.txt read.txt
	diff --git a/qread.txt b/read.txt
	index 4ab50d7..fdd2be1 100644
	--- a/qread.txt
	+++ b/read.txt
	@@ -53,7 +53,5 @@ wcfs/11399_1_r:
	 wcfs/11399_2_r:
	 wcfs/11399_3_r:
	 wcfs/11399_4_r:
	-wcfs/11399_5_r:
	 wcfs/11400_0_r:
	 wcfs/11401_0_r:
	-wcfs/11401_1_r:

The bug was hit because go-fuse by default uses 64K for read buffer size

	https://github.com/hanwen/go-fuse/blob/33711add/fuse/server.go#L142

and the kernel presets fuse_conn->max_pages to be 128K (= 32·4K pages).

Go-fuse will be likely fixed to both use bufsize=kernel's and to
correctly handle size > bufsize in InodeRetrieveCache. However we should
also fix the kernel to always deliver NOTIFY_REPLY once NOTIFY_RETRIEVE
was successful, so that FUSE protocol guarantee always holds
irregardless of whether userspace used default or other valid buffer
size setting, and so that filesystems can count not to get stuck waiting
for kernel who promised a reply.

This way this patch is here.

Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
Cc: Han-Wen Nienhuys <hanwen@google.com>
Cc: Jakob Unterwurzacher <jakobunt@gmail.com>
Cc: <stable@vger.kernel.org> # v2.6.36+
---

 ( This is my first patch to fs/fuse, so please forgive me if I missed anything. )

 fs/fuse/dev.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 8a63e52785e9..59bb97f2f491 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -381,6 +381,40 @@ static void queue_request(struct fuse_iqueue *fiq, struct fuse_req *req)
 	kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
 }

+/*
+ * fuse_req_truncate_data truncates data in request that has paged data
+ * (req.in.argpages=1), so that whole request, when serialized, is <= nbytes.
+ *
+ * nbytes must be >= size(request without data).
+ */
+static void fuse_req_truncate_data(struct fuse_req *req, unsigned nbytes) {
+	unsigned size, n;
+
+	BUG_ON(!req->in.argpages);
+	BUG_ON(req->in.numargs < 1);
+
+	/* request size without data */
+	size = sizeof(struct fuse_in_header) +
+		len_args(req->in.numargs - 1, (struct fuse_arg *) req->in.args);
+	BUG_ON(nbytes < size);
+
+	/* truncate paged data */
+	for (n = 0; n < req->num_pages; n++) {
+		struct fuse_page_desc *p = &req->page_descs[n];
+
+		if (size >= nbytes) {
+			p->length = 0;
+		} else {
+			p->length = min_t(unsigned, p->length, nbytes - size);
+		}
+
+		size += p->length;
+	}
+
+	/* update whole request length in the header */
+	req->in.h.len = size;
+}
+
 void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 		       u64 nodeid, u64 nlookup)
 {
@@ -1317,6 +1351,15 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	unsigned reqsize;
 	unsigned int hash;

+	/*
+	 * Require sane minimum read buffer - that has capacity for fixed part
+	 * of any request + some room for data. If the requirement is not
+	 * satisfied return EINVAL to the filesystem without dequeueing /
+	 * aborting any request.
+	 */
+	if (nbytes < FUSE_MIN_READ_BUFFER)
+		return -EINVAL;
+
  restart:
 	spin_lock(&fiq->waitq.lock);
 	err = -EAGAIN;
@@ -1358,12 +1401,27 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,

 	/* If request is too large, reply with an error and restart the read */
 	if (nbytes < reqsize) {
-		req->out.h.error = -EIO;
-		/* SETXATTR is special, since it may contain too large data */
-		if (in->h.opcode == FUSE_SETXATTR)
-			req->out.h.error = -E2BIG;
-		request_end(fc, req);
-		goto restart;
+		switch (in->h.opcode) {
+		default:
+			req->out.h.error = -EIO;
+			/* SETXATTR is special, since it may contain too large data */
+			if (in->h.opcode == FUSE_SETXATTR)
+				req->out.h.error = -E2BIG;
+			request_end(fc, req);
+			goto restart;
+
+		/*
+		 * NOTIFY_REPLY is special: if it was queued we already
+		 * promised to filesystem to deliver it when handling
+		 * NOTIFY_RETRIVE. We know that read buffer has capacity for at
+		 * least some data. Truncate retrieved data to read buffer size
+		 * and deliver it to stay to the promise.
+		 */
+		case FUSE_NOTIFY_REPLY:
+			fuse_req_truncate_data(req, nbytes);
+			reqsize = in->h.len;
+		}
+
 	}
 	spin_lock(&fpq->lock);
 	list_add(&req->list, &fpq->io);
--
2.21.0.rc0.269.g1a574e7a28


^ permalink raw reply related	[flat|nested] only message in thread

only message in thread, other threads:[~2019-02-12 20:19 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12 20:04 [PATCH] fuse: Don't drop NOTIFY_REPLY if we promised it Kirill Smelkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.