From patchwork Tue Feb 12 20:04:23 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kirill Smelkov X-Patchwork-Id: 10808653 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 779FA14E1 for ; Tue, 12 Feb 2019 20:19:32 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 639E22C12D for ; Tue, 12 Feb 2019 20:19:32 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 555522C13A; Tue, 12 Feb 2019 20:19:32 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.6 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,URIBL_GREY autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 307872C12D for ; Tue, 12 Feb 2019 20:19:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730990AbfBLUTa (ORCPT ); Tue, 12 Feb 2019 15:19:30 -0500 Received: from mail177-6.suw61.mandrillapp.com ([198.2.177.6]:20184 "EHLO mail177-6.suw61.mandrillapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727885AbfBLUT3 (ORCPT ); Tue, 12 Feb 2019 15:19:29 -0500 X-Greylist: delayed 903 seconds by postgrey-1.27 at vger.kernel.org; Tue, 12 Feb 2019 15:19:26 EST DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; s=mandrill; d=nexedi.com; h=From:Subject:To:Cc:Message-Id:Date:MIME-Version:Content-Type:Content-Transfer-Encoding; i=kirr@nexedi.com; bh=GV+GF05l8bIPfkKomToqU/NAezs8exlgtePQznkUxuQ=; b=Iv9hosU5+Co+KzAy5VEEmRQoSvQMNIvFMvonliYOfNJVc59nS6Ycnziu0Z1XQgmRG5Otx4QpO03v ZpQZumf2XmAhAySLJA+bM+KCUEjuCBI+lpQz2h1if75aBC9jEESMnhcryR2o09rgZ2CETQvsNlWR vWkbLf51ENPHZH4d3o0= Received: from pmta06.mandrill.prod.suw01.rsglab.com (127.0.0.1) by mail177-6.suw61.mandrillapp.com id hccl4s22rtke for ; Tue, 12 Feb 2019 20:04:23 +0000 (envelope-from ) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mandrillapp.com; i=@mandrillapp.com; q=dns/txt; s=mandrill; t=1550001863; h=From : Subject : To : Cc : Message-Id : Date : MIME-Version : Content-Type : Content-Transfer-Encoding : From : Subject : Date : X-Mandrill-User : List-Unsubscribe; bh=GV+GF05l8bIPfkKomToqU/NAezs8exlgtePQznkUxuQ=; b=Wg8chtR/dVPdLTiVjunDCAS65vrx79XsLvfBc8IreOdkmJ4vmMjNxNGVx5hpbqUgdw45zY +zZFsLcZWwpiUtifX8iYCvsvX5rYSE7YQ/RSL7a89oaWp+H/pAm2wAtF0Xnw1fGX8m9p6EdF 9bEwI6GKJF4YHQMxgG0uFp+htDnPc= From: Kirill Smelkov Subject: [PATCH] fuse: Don't drop NOTIFY_REPLY if we promised it Received: from [87.98.221.171] by mandrillapp.com id 36f16ef4dee54233ae0fbdc97d1acff2; Tue, 12 Feb 2019 20:04:23 +0000 X-Mailer: git-send-email 2.21.0.rc0.269.g1a574e7a28 To: Miklos Szeredi Cc: , Kirill Smelkov , Han-Wen Nienhuys , Jakob Unterwurzacher , Message-Id: <20190212200412.6573-1-kirr@nexedi.com> X-Report-Abuse: Please forward a copy of this message, including all headers, to abuse@mandrill.com X-Report-Abuse: You can also report abuse here: http://mandrillapp.com/contact/abuse?id=31050260.36f16ef4dee54233ae0fbdc97d1acff2 X-Mandrill-User: md_31050260 Date: Tue, 12 Feb 2019 20:04:23 +0000 MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Cc: Han-Wen Nienhuys Cc: Jakob Unterwurzacher Cc: # 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);