From patchwork Fri Oct 21 09:12:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 9388309 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7331C607D0 for ; Fri, 21 Oct 2016 09:12:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 615BE29F53 for ; Fri, 21 Oct 2016 09:12:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5354329F57; Fri, 21 Oct 2016 09:12:40 +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=-6.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID 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 DDEB929F53 for ; Fri, 21 Oct 2016 09:12:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932193AbcJUJMS (ORCPT ); Fri, 21 Oct 2016 05:12:18 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:34260 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932436AbcJUJMP (ORCPT ); Fri, 21 Oct 2016 05:12:15 -0400 Received: by mail-qk0-f195.google.com with SMTP id n189so6533660qke.1 for ; Fri, 21 Oct 2016 02:12:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=UoDxJT3762g7YTiWz4Yk4cJ8ci0GiCv3eDznZtWmxJE=; b=qFv00CfB1vgcWaMoaVha6YtJRGQiEiKhLa5368C12v4pMPeOEPCLM1Ksse+jU4ZkL/ x3yQnPpwyzkKxkK8Ns3VD1b5izaIFj1a2Ap14nRBSJ/2UHpK1H1LG6L/GKO63sqMnbQU nEcGfvtF6QJPIeo23j/ZJ8d2l+R2YIxjYXhKw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=UoDxJT3762g7YTiWz4Yk4cJ8ci0GiCv3eDznZtWmxJE=; b=aHTI2WWjGxGmg4KDipBwqhkj+qMwr1N077lFfP6Xh2KSiBncbKRl/9ZA6yPS4Zk4uY SpgRRHRKwv5TDIe/rCt9gPe0Jni4E4T/npfIl1nkjWqaMmIMSW9dFrTOgMBDd0Lw2S/8 nRlj6T9W+sJmbpUQH40HEg7RWyBsW45Sq6BPmsKXV6NOoE3nIpuPm2cV+GQlcOQ5AmSk Ck/QxdkZwhb+g4z7X+De6y/t9VbPHtzUghvW2FL3D1HbiBjCRzBbMbizYUPtvI7eJU5P eL4dHI3+OU1Qxbld2mXyJk1CvYlgHvtCMvFFcfu3sXb8pQ8CtQpDYAUt8UzHR/BlPNz1 at+Q== X-Gm-Message-State: ABUngvfC0IIdIeJvIxyNJo7dtBT9yXVRHNkptLTKeA55+MMXdW5otWDeNDhSqh+Sp1HdKQ== X-Received: by 10.194.115.135 with SMTP id jo7mr3222663wjb.21.1477041134391; Fri, 21 Oct 2016 02:12:14 -0700 (PDT) Received: from veci.piliscsaba.szeredi.hu (78-131-56-135.static.hdsnet.hu. [78.131.56.135]) by smtp.gmail.com with ESMTPSA id m194sm3062073wmg.11.2016.10.21.02.12.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Oct 2016 02:12:13 -0700 (PDT) Date: Fri, 21 Oct 2016 11:12:11 +0200 From: Miklos Szeredi To: Vivek Goyal Cc: linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Jeremy Eder , David Howells , Gou Rao , Vinod Jayaraman , Al Viro , Dave Chinner Subject: Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Message-ID: <20161021091211.GI31239@veci.piliscsaba.szeredi.hu> References: <20161012133326.GD31239@veci.piliscsaba.szeredi.hu> <20161020204630.GA1000@redhat.com> <20161020205408.GB1000@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161020205408.GB1000@redhat.com> User-Agent: Mutt/1.7.0 (2016-08-17) 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 On Thu, Oct 20, 2016 at 04:54:08PM -0400, Vivek Goyal wrote: > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote: > > [..] > > > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to) > > > +{ > > > + struct file *file = iocb->ki_filp; > > > + bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry)); > > > + ssize_t ret = -EINVAL; > > > + > > > + if (likely(!isupper)) { > > > + const struct file_operations *fop = ovl_real_fop(file); > > > + > > > + if (likely(fop->read_iter)) > > > + ret = fop->read_iter(iocb, to); > > > + } else { > > > + struct file *upperfile = filp_clone_open(file); > > > + > > > > IIUC, every read of lower file will call filp_clone_open(). Looking at the > > code of filp_clone_open(), I am concerned about the overhead of this call. > > Is it significant? Don't want to be paying too much of penalty for read > > operation on lower files. That would be a common case for containers. > > > > Looks like I read the code in reverse. So if I open a file read-only, > and if it has not been copied up, I will simply call read_iter() on > lower filesystem. But if file has been copied up, then I will call > filp_clone_open() and pay the cost. And this will continue till this > file is closed by caller. > > When file is opened again, by that time it is upper file and we will > install real fop in file (instead of overlay fop). Right. The lockdep issue seems to be real, we can't take i_mutex and s_vfs_rename_mutex while mmap_sem is locked. Fortunately copy up doesn't need mmap_sem, so we can do it while unlocked and retry the mmap. Here's an incremental workaround patch. I don't like adding such workarounds to the VFS/MM but they are really cheap for the non-overlay case and there doesn't appear to be an alternative in this case. Thanks, Miklos --- fs/overlayfs/inode.c | 19 +++++-------------- mm/util.c | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 14 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -419,21 +419,12 @@ static int ovl_mmap(struct file *file, s bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry)); int err; - /* - * Treat MAP_SHARED as hint about future writes to the file (through - * another file descriptor). Caller might not have had such an intent, - * but we hope MAP_PRIVATE will be used in most such cases. - * - * If we don't copy up now and the file is modified, it becomes really - * difficult to change the mapping to match that of the file's content - * later. - */ if (unlikely(isupper || vma->vm_flags & VM_MAYSHARE)) { - if (!isupper) { - err = ovl_copy_up(file->f_path.dentry); - if (err) - goto out; - } + /* + * File should have been copied up by now. See vm_mmap_pgoff(). + */ + if (WARN_ON(!isupper)) + return -EIO; file = filp_clone_open(file); err = PTR_ERR(file); --- a/mm/util.c +++ b/mm/util.c @@ -297,6 +297,28 @@ unsigned long vm_mmap_pgoff(struct file ret = security_mmap_file(file, prot, flag); if (!ret) { + /* + * Special treatment for overlayfs: + * + * Take MAP_SHARED/PROT_READ as hint about future writes to the + * file (through another file descriptor). Caller might not + * have had such an intent, but we hope MAP_PRIVATE will be used + * in most such cases. + * + * If we don't copy up now and the file is modified, it becomes + * really difficult to change the mapping to match that of the + * file's content later. + * + * Copy up needs to be done without mmap_sem since it takes vfs + * locks which would potentially deadlock under mmap_sem. + */ + if ((flag & MAP_SHARED) && !(prot & PROT_WRITE)) { + void *p = d_real(file->f_path.dentry, NULL, O_WRONLY); + + if (IS_ERR(p)) + return PTR_ERR(p); + } + if (down_write_killable(&mm->mmap_sem)) return -EINTR; ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,