On Tue, May 08, 2018 at 08:26:28AM +0300, Amir Goldstein wrote:
> On Mon, May 7, 2018 at 11:59 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> > On Mon, May 07, 2018 at 10:47:28PM +0300, Amir Goldstein wrote:
> >> On Mon, May 7, 2018 at 8:40 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> >> > 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 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@xxxxxxxxxx>
> >> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >>
> >> except one nit
> >>
> >> > ---
> >> > fs/overlayfs/file.c | 49 +++++++++++++++++++++++++++++++++----------------
> >> > 1 file changed, 33 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> >> > index a60734ec89ec..885151e8d0cb 100644
> >> > --- a/fs/overlayfs/file.c
> >> > +++ b/fs/overlayfs/file.c
> >> > @@ -14,22 +14,32 @@
> >> > #include <linux/uio.h>
> >> > #include "overlayfs.h"
> >> >
> >> > -static struct file *ovl_open_realfile(const struct file *file)
> >> > +static struct file *ovl_open_realfile(const struct file *file,
> >> > + bool allow_metacopy)
> >> > {
> >> > struct inode *inode = file_inode(file);
> >> > struct inode *upperinode = ovl_inode_upper(inode);
> >> > - struct inode *realinode = upperinode ?: ovl_inode_lower(inode);
> >> > + struct inode *realinode;
> >> > struct file *realfile;
> >> > + bool upperopen = false;
> >> > const struct cred *old_cred;
> >> >
> >> > + if (upperinode && (allow_metacopy || ovl_has_upperdata(inode))) {
> >> > + realinode = upperinode;
> >> > + upperopen = true;
> >> > + } else {
> >> > + realinode = allow_metacopy ? ovl_inode_lower(inode) :
> >> > + ovl_inode_lowerdata(inode);
> >> > + }
> >> > old_cred = ovl_override_creds(inode->i_sb);
> >> > realfile = path_open(&file->f_path, file->f_flags | O_NOATIME,
> >> > realinode, current_cred(), false);
> >> > revert_creds(old_cred);
> >> >
> >> > pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
> >> > - file, file, upperinode ? 'u' : 'l', file->f_flags,
> >> > - realfile, IS_ERR(realfile) ? 0 : realfile->f_flags);
> >> > + file, file, upperopen ? 'u' : 'l',
> >> > + file->f_flags, realfile,
> >> > + IS_ERR(realfile) ? 0 : realfile->f_flags);
> >> >
> >> > return realfile;
> >> > }
> >> > @@ -72,17 +82,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(const struct file *file, struct fd *real,
> >> > + bool allow_metacopy)
> >> > {
> >> > struct inode *inode = file_inode(file);
> >> > + struct inode *realinode;
> >> >
> >> > real->flags = 0;
> >> > real->file = file->private_data;
> >> >
> >> > + if (allow_metacopy)
> >> > + 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, allow_metacopy);
> >> >
> >> > return PTR_ERR_OR_ZERO(real->file);
> >> > }
> >> > @@ -107,7 +124,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, false);
> >> > if (IS_ERR(realfile))
> >> > return PTR_ERR(realfile);
> >> >
> >> > @@ -184,7 +201,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >> > if (!iov_iter_count(iter))
> >> > return 0;
> >> >
> >> > - ret = ovl_real_fdget(file, &real);
> >> > + ret = ovl_real_fdget(file, &real, false);
> >>
> >>
> >> Instead of changing all those call sites, use a wrapper
> >>
> >> ovl_real_fdget(file, real) => ovl_real_fdget_metacopy(file, real, false)
> >>
> >> and use ovl_real_fdget_metacopy() directly only when you want
> >> the metacopy file.
> >
> > You mean define another helper ovl_real_fdget_metacopy(file, real) and
> > use that when metacopy file is desired/allowed?
> >
> > That's what I had done in previous series. That is define a separate
> > wrapper for metacopy and you had not liked it and that's why I did
> > it this way.
>
> We miss-communicated. I did like it, but I though it was not needed
> because of fsync implementation bug (which I forgot to report to Miklos).
> I was wrong though. I understand now why allow_metacopy is needed
> for fsync.
>
> What you actually need to call from ovl_fsync() is:
> _ovl_real_file(file, real, !datasync);
>
> So you don't actually have a user for ovl_real_meta_file(), but that's
> fine to have the helper anyway.
Ok, here is the updated patch. I have not defined quivalent of
ovl_real_meta_file() as there are no users.
Subject: ovl: Open file with data except for the case of fsync
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 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@xxxxxxxxxx>
---
fs/overlayfs/file.c | 40 +++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
Index: rhvgoyal-linux/fs/overlayfs/file.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/file.c 2018-05-07 16:55:02.562350785 -0400
+++ rhvgoyal-linux/fs/overlayfs/file.c 2018-05-08 08:46:49.994350785 -0400
@@ -14,22 +14,32 @@
#include <linux/uio.h>
#include "overlayfs.h"
-static struct file *ovl_open_realfile(const struct file *file)
+static struct file *ovl_open_realfile(const struct file *file,
+ bool allow_metacopy)
{
struct inode *inode = file_inode(file);
struct inode *upperinode = ovl_inode_upper(inode);
- struct inode *realinode = upperinode ?: ovl_inode_lower(inode);
+ struct inode *realinode;
struct file *realfile;
+ bool upperopen = false;
const struct cred *old_cred;
+ if (upperinode && (allow_metacopy || ovl_has_upperdata(inode))) {
+ realinode = upperinode;
+ upperopen = true;
+ } else {
+ realinode = allow_metacopy ? ovl_inode_lower(inode) :
+ ovl_inode_lowerdata(inode);
+ }
old_cred = ovl_override_creds(inode->i_sb);
realfile = path_open(&file->f_path, file->f_flags | O_NOATIME,
realinode, current_cred(), false);
revert_creds(old_cred);
pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
- file, file, upperinode ? 'u' : 'l', file->f_flags,
- realfile, IS_ERR(realfile) ? 0 : realfile->f_flags);
+ file, file, upperopen ? 'u' : 'l',
+ file->f_flags, realfile,
+ IS_ERR(realfile) ? 0 : realfile->f_flags);
return realfile;
}
@@ -72,17 +82,24 @@ static int ovl_change_flags(struct file
return 0;
}
-static int ovl_real_fdget(const struct file *file, struct fd *real)
+static int _ovl_real_fdget(const struct file *file, struct fd *real,
+ bool allow_metacopy)
{
struct inode *inode = file_inode(file);
+ struct inode *realinode;
real->flags = 0;
real->file = file->private_data;
+ if (allow_metacopy)
+ 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, allow_metacopy);
return PTR_ERR_OR_ZERO(real->file);
}
@@ -94,6 +111,11 @@ static int ovl_real_fdget(const struct f
return 0;
}
+static int ovl_real_fdget(const struct file *file, struct fd *real)
+{
+ return _ovl_real_fdget(file, real, false);
+}
+
static int ovl_open(struct inode *inode, struct file *file)
{
struct dentry *dentry = file_dentry(file);
@@ -107,7 +129,7 @@ static int ovl_open(struct inode *inode,
/* 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, false);
if (IS_ERR(realfile))
return PTR_ERR(realfile);
@@ -244,7 +266,7 @@ static int ovl_fsync(struct file *file,
const struct cred *old_cred;
int ret;
- ret = ovl_real_fdget(file, &real);
+ ret = _ovl_real_fdget(file, &real, !datasync);
if (ret)
return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
