From: Vivek Goyal <vgoyal@redhat.com>
To: Miklos Szeredi <mszeredi@redhat.com>
Cc: linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 15/28] ovl: Open file with data except for the case of fsync
Date: Wed, 30 May 2018 10:30:44 -0400 [thread overview]
Message-ID: <20180530143044.GB2717@redhat.com> (raw)
In-Reply-To: <20180529144612.16675-16-mszeredi@redhat.com>
On Tue, May 29, 2018 at 04:45:59PM +0200, Miklos Szeredi wrote:
> From: Vivek Goyal <vgoyal@redhat.com>
>
> ovl_open() should open file which contains data and not open metacopy
> inode. With the introduction of metacopy inodes, with current
> implementaion we will end up opening metacopy inode as well.
>
> But there can be certain circumstances like ovl_fsync() where we want to
> allow opening a metacopy inode instead.
>
> Hence, change ovl_open_realfile() and add _ovl_open_real() and add extra
> parameter which specifies whether to allow opening metacopy inode or not.
> If this parameter is false, we look for data inode and open that.
>
> This should allow covering both the cases.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
> fs/overlayfs/file.c | 39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 266692ce9a9a..c7738ef492c8 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -14,11 +14,20 @@
> #include <linux/uio.h>
> #include "overlayfs.h"
>
> -static struct file *ovl_open_realfile(const struct file *file)
> +static char ovl_whatisit(struct inode *inode, struct inode *realinode)
> +{
> + if (realinode != ovl_inode_upper(inode))
> + return 'l';
> + if (ovl_has_upperdata(inode))
> + return 'u';
> + else
> + return 'm';
> +}
> +
> +static struct file *ovl_open_realfile(const struct file *file,
> + struct inode *realinode)
> {
> struct inode *inode = file_inode(file);
> - struct inode *upperinode = ovl_inode_upper(inode);
> - struct inode *realinode = upperinode ?: ovl_inode_lower(inode);
> struct file *realfile;
> const struct cred *old_cred;
>
> @@ -28,7 +37,7 @@ static struct file *ovl_open_realfile(const struct file *file)
> revert_creds(old_cred);
>
> pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
> - file, file, upperinode ? 'u' : 'l', file->f_flags,
> + file, file, ovl_whatisit(inode, realinode), file->f_flags,
> realfile, IS_ERR(realfile) ? 0 : realfile->f_flags);
>
> return realfile;
> @@ -72,17 +81,24 @@ static int ovl_change_flags(struct file *file, unsigned int flags)
> return 0;
> }
>
> -static int ovl_real_fdget(const struct file *file, struct fd *real)
> +static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
> + bool allow_meta)
> {
> struct inode *inode = file_inode(file);
> + struct inode *realinode;
>
> real->flags = 0;
> real->file = file->private_data;
>
> + if (allow_meta)
> + realinode = ovl_inode_real(inode);
> + else
> + realinode = ovl_inode_realdata(inode);
> +
> /* Has it been copied up since we'd opened it? */
> - if (unlikely(file_inode(real->file) != ovl_inode_real(inode))) {
> + if (unlikely(file_inode(real->file) != realinode)) {
> real->flags = FDPUT_FPUT;
> - real->file = ovl_open_realfile(file);
> + real->file = ovl_open_realfile(file, realinode);
>
> return PTR_ERR_OR_ZERO(real->file);
> }
> @@ -94,6 +110,11 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
> return 0;
> }
>
> +static int ovl_real_fdget(const struct file *file, struct fd *real)
> +{
> + return ovl_real_fdget_meta(file, real, false);
> +}
> +
> static int ovl_open(struct inode *inode, struct file *file)
> {
> struct dentry *dentry = file_dentry(file);
> @@ -107,7 +128,7 @@ static int ovl_open(struct inode *inode, struct file *file)
> /* No longer need these flags, so don't pass them on to underlying fs */
> file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>
> - realfile = ovl_open_realfile(file);
> + realfile = ovl_open_realfile(file, ovl_inode_real(file_inode(file)));
Hi Miklos,
Hmm...so there have been some changes in this patch. My original intention
was that to always open data inode (lower/upper) in ovl_open(). So if upper
inode is a metacopy only, I will open lower inode instead.
But new logic seems to be to always open real inode (that means upper
metacopy inode as well). And that means that later when read happens
on the file we will end up opening lower file, read data and close
lower file.
I am concerned a bit if there are performance implications to this.
This will be hot path for containers.
Thanks
Vivek
next prev parent reply other threads:[~2018-05-30 14:30 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-29 14:45 [PATCH 00/28] overlayfs: Delayed copy up of data Miklos Szeredi
2018-05-29 14:45 ` [PATCH 01/28] ovl: Initialize ovl_inode->redirect in ovl_get_inode() Miklos Szeredi
2018-05-29 14:45 ` [PATCH 02/28] ovl: Move the copy up helpers to copy_up.c Miklos Szeredi
2018-05-29 14:45 ` [PATCH 03/28] ovl: Provide a mount option metacopy=on/off for metadata copyup Miklos Szeredi
2018-05-29 20:44 ` Randy Dunlap
2018-05-30 8:27 ` Miklos Szeredi
2018-05-29 14:45 ` [PATCH 04/28] ovl: During copy up, first copy up metadata and then data Miklos Szeredi
2018-05-29 14:45 ` [PATCH 05/28] ovl: Copy up only metadata during copy up where it makes sense Miklos Szeredi
2018-05-29 14:45 ` [PATCH 06/28] ovl: Add helper ovl_already_copied_up() Miklos Szeredi
2018-05-29 14:45 ` [PATCH 07/28] ovl: A new xattr OVL_XATTR_METACOPY for file on upper Miklos Szeredi
2018-05-29 14:45 ` [PATCH 08/28] ovl: Use out_err instead of out_nomem Miklos Szeredi
2018-05-29 14:45 ` [PATCH 09/28] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry Miklos Szeredi
2018-05-29 14:45 ` [PATCH 10/28] ovl: Copy up meta inode data from lowest data inode Miklos Szeredi
2018-05-29 14:45 ` [PATCH 11/28] ovl: Add helper ovl_dentry_lowerdata() to get lower data dentry Miklos Szeredi
2018-05-29 14:45 ` [PATCH 12/28] ovl: Fix ovl_getattr() to get number of blocks from lower Miklos Szeredi
2018-05-29 14:45 ` [PATCH 13/28] ovl: Store lower data inode in ovl_inode Miklos Szeredi
2018-05-29 14:45 ` [PATCH 14/28] ovl: Add helper ovl_inode_realdata() Miklos Szeredi
2018-05-29 14:45 ` [PATCH 15/28] ovl: Open file with data except for the case of fsync Miklos Szeredi
2018-05-30 14:30 ` Vivek Goyal [this message]
2018-05-30 15:12 ` Miklos Szeredi
2018-05-30 15:48 ` Vivek Goyal
2018-05-29 14:46 ` [PATCH 16/28] ovl: Do not expose metacopy only dentry from d_real() Miklos Szeredi
2018-05-30 21:05 ` Vivek Goyal
2018-05-31 4:30 ` Amir Goldstein
2018-05-29 14:46 ` [PATCH 17/28] ovl: Move some dir related ovl_lookup_single() code in else block Miklos Szeredi
2018-05-29 14:46 ` [PATCH 18/28] ovl: Check redirects for metacopy files Miklos Szeredi
2018-05-29 14:46 ` [PATCH 19/28] ovl: Treat metacopy dentries as type OVL_PATH_MERGE Miklos Szeredi
2018-05-29 14:46 ` [PATCH 20/28] ovl: Add an inode flag OVL_CONST_INO Miklos Szeredi
2018-05-29 14:46 ` [PATCH 21/28] ovl: Do not set dentry type ORIGIN for broken hardlinks Miklos Szeredi
2018-05-29 14:46 ` [PATCH 22/28] ovl: Set redirect on metacopy files upon rename Miklos Szeredi
2018-05-29 14:46 ` [PATCH 23/28] ovl: Set redirect on upper inode when it is linked Miklos Szeredi
2018-05-29 14:46 ` [PATCH 24/28] ovl: Check redirect on index as well Miklos Szeredi
2018-05-29 14:46 ` [PATCH 25/28] ovl: Disbale metacopy for MAP_SHARED mmap() Miklos Szeredi
2018-05-29 14:46 ` [PATCH 26/28] ovl: Do not do metadata only copy-up for truncate operation Miklos Szeredi
2018-05-29 14:46 ` [PATCH 27/28] ovl: Do not do metacopy only for ioctl modifying file attr Miklos Szeredi
2018-05-29 14:46 ` [PATCH 28/28] ovl: Enable metadata only feature Miklos Szeredi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180530143044.GB2717@redhat.com \
--to=vgoyal@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=mszeredi@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).