linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Miklos Szeredi <mszeredi@redhat.com>,
	overlayfs <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 17:12:16 +0200	[thread overview]
Message-ID: <CAJfpegvLcFgFi7aMiJs=xotEMKNNDy7rXdms08Mx3c5G37OJgw@mail.gmail.com> (raw)
In-Reply-To: <20180530143044.GB2717@redhat.com>

On Wed, May 30, 2018 at 4:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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)));

That was meant to be

+     realfile = ovl_open_realfile(file, ovl_inode_realdata(file_inode(file)));

Is that correct?

> 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.

Right.   Unfortunately not detected with automatic testing...

Thanks for spotting!

Miklos

  reply	other threads:[~2018-05-30 15:12 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
2018-05-30 15:12     ` Miklos Szeredi [this message]
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='CAJfpegvLcFgFi7aMiJs=xotEMKNNDy7rXdms08Mx3c5G37OJgw@mail.gmail.com' \
    --to=miklos@szeredi.hu \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=mszeredi@redhat.com \
    --cc=vgoyal@redhat.com \
    --subject='Re: [PATCH 15/28] ovl: Open file with data except for the case of fsync' \
    /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

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).