From: Vivek Goyal <vgoyal@redhat.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: overlayfs <linux-unionfs@vger.kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>
Subject: Re: [PATCH 06/11] ovl: Set xattr OVL_XATTR_METACOPY on upper file
Date: Wed, 18 Oct 2017 09:30:52 -0400 [thread overview]
Message-ID: <20171018133052.GD3445@redhat.com> (raw)
In-Reply-To: <CAOQ4uxiEU2i9LbEkFR1sk0UaSt54qYe-4adzS+FFrukAtJ0khQ@mail.gmail.com>
On Wed, Oct 18, 2017 at 07:57:16AM +0300, Amir Goldstein wrote:
> On Wed, Oct 18, 2017 at 12:05 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Presence of OVL_XATTR_METACOPY reflects that file has been copied up
> > with metadata only and data needs to be copied up later from lower.
> > So this xattr is set when a metadata copy takes place and cleared when
> > data copy takes place.
> >
> > We also use a bit in ovl_inode->flags to cache OVL_METACOPY which reflects
> > whether ovl inode has only metadata copied up.
> >
> > ovl_set_size() code has been taken from Amir's patch.
>
> I waiver this attribution.
> In git log history context, it is just noise that adds no information.
> Please remove.
Ok, will do.
>
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/overlayfs/copy_up.c | 59 ++++++++++++++++++++++++++++++++++++++++++------
> > fs/overlayfs/inode.c | 3 ++-
> > fs/overlayfs/overlayfs.h | 5 +++-
> > fs/overlayfs/util.c | 21 +++++++++++++++--
> > 4 files changed, 77 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index cf0b36518a3a..99b7be4ff4fc 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -196,6 +196,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> > return error;
> > }
> >
> > +static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
> > +{
> > + struct iattr attr = {
> > + .ia_valid = ATTR_SIZE,
> > + .ia_size = stat->size,
> > + };
> > +
> > + return notify_change(upperdentry, &attr, NULL);
> > +}
> > +
> > static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
> > {
> > struct iattr attr = {
> > @@ -439,6 +449,27 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
> > goto out;
> > }
> >
> > +/* Copy up data of an inode which was copied up metadata only in the past. */
> > +static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>
> I like the name :)
Glad to hear that. :-)
>
> > +{
> > + struct path upperpath;
> > + int err;
> > +
> > + ovl_path_upper(c->dentry, &upperpath);
> > + BUG_ON(upperpath.dentry == NULL);
>
> I know this is copied from ovl_copy_up_inode() and Miklos uses BUG_ON often,
> but kernel coding style prefers to use
>
> if (WARN_ON())
> return -EIO;
Ok, I will change it and use above instead.
>
> > +
> > + err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> > + if (err)
> > + return err;
> > +
> > + err= vfs_removexattr(upperpath.dentry, OVL_XATTR_METACOPY);
> > + if (err)
> > + return err;
> > +
> > + ovl_clear_flag(OVL_METACOPY, d_inode(c->dentry));
> > + return err;
> > +}
> > +
> > static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > {
> > int err;
> > @@ -476,13 +507,21 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > return err;
> > }
> >
> > + if (c->metadata_only) {
> > + err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
> > + NULL, 0, -EOPNOTSUPP);
> > + if (err)
> > + return err;
> > + }
> > +
> > inode_lock(temp->d_inode);
> > - err = ovl_set_attr(temp, &c->stat);
> > - inode_unlock(temp->d_inode);
> > - if (err)
> > - return err;
> > + if (c->metadata_only)
> > + err = ovl_set_size(temp, &c->stat);
> >
> > - return 0;
> > + if (!err)
> > + err = ovl_set_attr(temp, &c->stat);
> > + inode_unlock(temp->d_inode);
> > + return err;
> > }
> >
> > static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
> > @@ -510,6 +549,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
> > if (err)
> > goto out_cleanup;
> >
> > + if (c->metadata_only)
> > + ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
> > ovl_inode_update(d_inode(c->dentry), newdentry);
> > out:
> > dput(temp);
> > @@ -637,7 +678,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > }
> > ovl_do_check_copy_up(ctx.lowerpath.dentry);
> >
> > - err = ovl_copy_up_start(dentry);
> > + err = ovl_copy_up_start(dentry, flags);
> > /* err < 0: interrupted, err > 0: raced with another copy-up */
> > if (unlikely(err)) {
> > if (err > 0)
> > @@ -647,6 +688,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> > err = ovl_do_copy_up(&ctx);
> > if (!err && !ovl_dentry_has_upper_alias(dentry))
> > err = ovl_link_up(&ctx);
> > + if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
> > + err = ovl_copy_up_meta_inode_data(&ctx);
> > ovl_copy_up_end(dentry);
> > }
> > do_delayed_call(&done);
> > @@ -677,8 +720,10 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
> > * with rename.
> > */
> > if (ovl_dentry_upper(dentry) &&
> > - ovl_dentry_has_upper_alias(dentry))
> > + ovl_dentry_has_upper_alias(dentry) &&
> > + !ovl_dentry_needs_data_copy_up(dentry, flags)) {
> > break;
> > + }
> >
> > next = dget(dentry);
> > /* find the topmost dentry not yet copied up */
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 321511ed8c42..1b4b42c45ed5 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -320,7 +320,8 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
> > static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
> > {
> > if (ovl_dentry_upper(dentry) &&
> > - ovl_dentry_has_upper_alias(dentry))
> > + ovl_dentry_has_upper_alias(dentry) &&
> > + !ovl_dentry_needs_data_copy_up(dentry, flags))
> > return false;
> >
> > if (special_file(d_inode(dentry)->i_mode))
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index d9a0edd4e57e..dcec9456c654 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -26,10 +26,12 @@ enum ovl_path_type {
> > #define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
> > #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
> > #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
> > +#define OVL_XATTR_METACOPY OVL_XATTR_PREFIX "metacopy"
> >
> > enum ovl_flag {
> > OVL_IMPURE,
> > OVL_INDEX,
> > + OVL_METACOPY,
> > };
> >
> > /*
> > @@ -211,6 +213,7 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
> > void ovl_dentry_set_opaque(struct dentry *dentry);
> > bool ovl_dentry_has_upper_alias(struct dentry *dentry);
> > void ovl_dentry_set_upper_alias(struct dentry *dentry);
> > +bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags);
> > bool ovl_redirect_dir(struct super_block *sb);
> > const char *ovl_dentry_get_redirect(struct dentry *dentry);
> > void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
> > @@ -221,7 +224,7 @@ void ovl_dentry_version_inc(struct dentry *dentry, bool impurity);
> > u64 ovl_dentry_version_get(struct dentry *dentry);
> > bool ovl_is_whiteout(struct dentry *dentry);
> > struct file *ovl_path_open(struct path *path, int flags);
> > -int ovl_copy_up_start(struct dentry *dentry);
> > +int ovl_copy_up_start(struct dentry *dentry, int flags);
> > void ovl_copy_up_end(struct dentry *dentry);
> > bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
> > int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index a4ce1c9944f0..91c542d1a39d 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -227,6 +227,22 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
> > oe->has_upper = true;
> > }
> >
> > +bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
>
> Maybe start off with
>
> if (likely(!ovl_test_flag(OVL_METACOPY, d_inode(dentry))
> return false;
This is lockless access and last two patches are introducing two smp_rmb()
around this access. So it might turn out to be more expensive to check
this first.
But if we managed to remove those barriers, then it makes sense to check
this first.
Vivek
>
> > + if (!S_ISREG(d_inode(dentry)->i_mode))
> > + return false;
> > +
> > + if (!flags)
> > + return false;
> > +
> > + if (!(OPEN_FMODE(flags) & FMODE_WRITE))
> > + return false;
> > +
> > + if (!ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > bool ovl_redirect_dir(struct super_block *sb)
> > {
> > struct ovl_fs *ofs = sb->s_fs_info;
> > @@ -310,13 +326,14 @@ struct file *ovl_path_open(struct path *path, int flags)
> > return dentry_open(path, flags | O_NOATIME, current_cred());
> > }
> >
> > -int ovl_copy_up_start(struct dentry *dentry)
> > +int ovl_copy_up_start(struct dentry *dentry, int flags)
> > {
> > struct ovl_inode *oi = OVL_I(d_inode(dentry));
> > int err;
> >
> > err = mutex_lock_interruptible(&oi->lock);
> > - if (!err && ovl_dentry_has_upper_alias(dentry)) {
> > + if (!err && ovl_dentry_has_upper_alias(dentry) &&
> > + !ovl_dentry_needs_data_copy_up(dentry, flags)) {
> > err = 1; /* Already copied up */
> > mutex_unlock(&oi->lock);
> > }
> > --
> > 2.13.5
> >
next prev parent reply other threads:[~2017-10-18 13:30 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-17 21:05 [RFC PATCH 00/11][V4] overlayfs: Delayed copy up of data Vivek Goyal
2017-10-17 21:05 ` [PATCH 01/11] ovl: Create origin xattr on copy up for all files Vivek Goyal
2017-10-18 4:09 ` Amir Goldstein
2017-10-18 12:55 ` Vivek Goyal
2017-10-18 13:56 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 02/11] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check Vivek Goyal
2017-10-18 4:11 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 03/11] ovl: During copy up, first copy up metadata and then data Vivek Goyal
2017-10-18 4:13 ` Amir Goldstein
2017-10-18 4:39 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 04/11] ovl: Provide a mount option metacopy=on/off for metadata copyup Vivek Goyal
2017-10-18 4:31 ` Amir Goldstein
2017-10-18 13:03 ` Vivek Goyal
2017-10-18 14:09 ` Amir Goldstein
2017-10-18 14:26 ` Vivek Goyal
2017-10-18 14:38 ` Amir Goldstein
2017-10-18 14:10 ` Vivek Goyal
2017-10-18 14:26 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 05/11] ovl: Copy up only metadata during copy up where it makes sense Vivek Goyal
2017-10-18 4:46 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 06/11] ovl: Set xattr OVL_XATTR_METACOPY on upper file Vivek Goyal
2017-10-18 4:57 ` Amir Goldstein
2017-10-18 13:30 ` Vivek Goyal [this message]
2017-10-17 21:05 ` [PATCH 07/11] ovl: Fix ovl_getattr() to get number of blocks from lower Vivek Goyal
2017-10-18 5:01 ` Amir Goldstein
2017-10-18 13:39 ` Vivek Goyal
2017-10-17 21:05 ` [PATCH 08/11] ovl: Set OVL_METACOPY flag during ovl_lookup() Vivek Goyal
2017-10-18 5:06 ` Amir Goldstein
2017-10-18 13:53 ` Vivek Goyal
2017-10-17 21:05 ` [PATCH 09/11] ovl: Return lower dentry if only metadata copy up took place Vivek Goyal
2017-10-18 5:07 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 10/11] ovl: Introduce read/write barriers around metacopy flag update Vivek Goyal
2017-10-18 5:19 ` Amir Goldstein
2017-10-18 15:32 ` Vivek Goyal
2017-10-18 16:05 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 11/11] ovl: Put barriers to order oi->__upperdentry and OVL_METACOPY update Vivek Goyal
2017-10-18 5:40 ` Amir Goldstein
2017-10-19 13:00 ` Vivek Goyal
2017-10-19 13:21 ` Amir Goldstein
2017-10-19 14:58 ` Vivek Goyal
2017-10-19 15:08 ` Amir Goldstein
2017-10-19 15:22 ` Vivek Goyal
2017-10-19 15:39 ` Amir Goldstein
2017-10-19 15:59 ` Vivek Goyal
2017-10-19 16:33 ` Amir Goldstein
2017-10-19 20:33 ` Vivek Goyal
2017-10-20 4:09 ` Amir Goldstein
2017-10-20 15:41 ` Vivek Goyal
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=20171018133052.GD3445@redhat.com \
--to=vgoyal@redhat.com \
--cc=amir73il@gmail.com \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
/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).