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@xxxxxxxxxx> Cc: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> Cc: Jakob Unterwurzacher <jakobunt@xxxxxxxxx> Cc: <stable@xxxxxxxxxxxxxxx> # 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
