From patchwork Fri Oct 21 20:13:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vivek Goyal X-Patchwork-Id: 9390053 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 66CAA60780 for ; Fri, 21 Oct 2016 20:13:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 590012A296 for ; Fri, 21 Oct 2016 20:13:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4D07A2A29A; Fri, 21 Oct 2016 20:13:55 +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.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable 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 DF3DA2A296 for ; Fri, 21 Oct 2016 20:13:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936177AbcJUUNi (ORCPT ); Fri, 21 Oct 2016 16:13:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57348 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755471AbcJUUNh (ORCPT ); Fri, 21 Oct 2016 16:13:37 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9964131B315; Fri, 21 Oct 2016 20:13:36 +0000 (UTC) Received: from horse.redhat.com (dhcp-25-205.bos.redhat.com [10.18.25.205]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9LKDZ9q017397; Fri, 21 Oct 2016 16:13:35 -0400 Received: by horse.redhat.com (Postfix, from userid 10451) id 0CBCF2043FF; Fri, 21 Oct 2016 16:13:35 -0400 (EDT) Date: Fri, 21 Oct 2016 16:13:35 -0400 From: Vivek Goyal To: Amir Goldstein Cc: Miklos Szeredi , linux-unionfs@vger.kernel.org, linux-fsdevel , linux-kernel , Jeremy Eder , David Howells , Ratna Bolla , Gou Rao , Vinod Jayaraman , Al Viro , Dave Chinner Subject: Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Message-ID: <20161021201335.GB20129@redhat.com> 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: User-Agent: Mutt/1.7.0 (2016-08-17) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 21 Oct 2016 20:13:37 +0000 (UTC) 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 Fri, Oct 21, 2016 at 11:53:41AM +0300, Amir Goldstein wrote: > On Thu, Oct 20, 2016 at 11:54 PM, 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. > > > > I wonder if that cost could be reduced by calling replace_fd() or > some variant of it to install the cloned file onto the rofd after the > first access?? Hmm.., Interesting. Will something like following work? This applies on top of Miklos's patch. It seems to work for me. It might be completely broken/racy though. Somebody who understands this code well, will have to have a look. --- fs/file.c | 41 +++++++++++++++++++++++++++++++++++++++++ fs/overlayfs/inode.c | 1 + 2 files changed, 42 insertions(+) -- 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 Index: rhvgoyal-linux/fs/overlayfs/inode.c =================================================================== --- rhvgoyal-linux.orig/fs/overlayfs/inode.c 2016-10-21 15:43:05.391488406 -0400 +++ rhvgoyal-linux/fs/overlayfs/inode.c 2016-10-21 16:07:57.409420795 -0400 @@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc if (IS_ERR(upperfile)) { ret = PTR_ERR(upperfile); } else { + replace_file(file, upperfile); ret = vfs_iter_read(upperfile, to, &iocb->ki_pos); fput(upperfile); } Index: rhvgoyal-linux/fs/file.c =================================================================== --- rhvgoyal-linux.orig/fs/file.c 2016-10-21 15:43:05.391488406 -0400 +++ rhvgoyal-linux/fs/file.c 2016-10-21 16:08:18.168420795 -0400 @@ -864,6 +864,47 @@ Ebusy: return -EBUSY; } + +int replace_file(struct file *old_file, struct file *new_file) +{ +#define MAX_TO_FREE 8 + int n, idx = 0; + struct files_struct *files = current->files; + struct fdtable *fdt; + struct file *to_free[MAX_TO_FREE]; + bool retry = false; + +try_again: + spin_lock(&files->file_lock); + for (n = 0, fdt = files_fdtable(files); n < fdt->max_fds; n++) { + struct file *file; + file = rcu_dereference_check_fdtable(files, fdt->fd[n]); + if (!file) + continue; + if (file == old_file) { + get_file(new_file); + rcu_assign_pointer(fdt->fd[n], new_file); + to_free[idx++] = file; + if (idx >= MAX_TO_FREE) { + retry = true; + break; + } + } + } + spin_unlock(&files->file_lock); + while (idx) { + filp_close(to_free[--idx], files); + } + + if (retry) { + retry = false; + idx = 0; + goto try_again; + } + return 0; +} +EXPORT_SYMBOL(replace_file); + int replace_fd(unsigned fd, struct file *file, unsigned flags) { int err;