overlayfs: Delayed copy up of data

By default metadata only copy up is disabled. Provide a mount option so that users can choose one way or other.

Also provide a kernel config and module option to enable/disable metacopy feature.

Like index feature, we verify on mount that upper root is not being reused with a different lower root. This hopes to get the configuration right and detect the copied layers use case. But this does only so much as we don't verify all the lowers. So it is possible that a lower is missing and later data copy up fails.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/Kconfig     |  8 ++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 40 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 3 deletions(-)

fs/overlayfs/Kconfig +8 -0

index cbfc196e5dc5..17a0b17ad14c 100644
@@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
          outcomes.  However, mounting the same overlay with an old kernel
          read-write and then mounting it again with a new kernel, will have
          unexpected results.
+
+config OVERLAY_FS_METACOPY
+       bool "Overlayfs: turn on metadata only copy up feature by default"
+       depends on OVERLAY_FS
+       help
+         If this config option is enabled then overlay filesystems will
+         copy up only metadata where appropriate and data copy up will
+         happen when a file is opended for WRITE operation.

fs/overlayfs/ovl_entry.h +1 -0

index 93eb6a044dd2..2720ed21ad53 100644
@@ -15,6 +15,7 @@ struct ovl_config {
        bool default_permissions;
        bool redirect_dir;
        bool index;
+       bool metacopy;
 };
 
 struct ovl_layer {

fs/overlayfs/super.c +28 -3

index ee41bde87b14..d8b6b4d80d78 100644
@@ -46,6 +46,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
                dput(oe->lowerstack[i].dentry);
 }
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+                "Default to on or off for the metadata only copy up feature");
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
        struct ovl_entry *oe = dentry->d_fsdata;

@@ -319,6 +324,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
        if (ofs->config.index != ovl_index_def)
                seq_printf(m, ",index=%s",
                           ofs->config.index ? "on" : "off");
+       if (ofs->config.metacopy != ovl_metacopy_def)
+               seq_printf(m, ",metacopy=%s",
+                          ofs->config.metacopy ? "on" : "off");
        return 0;
 }
 

@@ -352,6 +360,8 @@ enum {
        OPT_REDIRECT_DIR_OFF,
        OPT_INDEX_ON,
        OPT_INDEX_OFF,
+       OPT_METACOPY_ON,
+       OPT_METACOPY_OFF,
        OPT_ERR,
 };
 

@@ -364,6 +374,8 @@ static const match_table_t ovl_tokens = {
        {OPT_REDIRECT_DIR_OFF,          "redirect_dir=off"},
        {OPT_INDEX_ON,                  "index=on"},
        {OPT_INDEX_OFF,                 "index=off"},
+       {OPT_METACOPY_ON,               "metacopy=on"},
+       {OPT_METACOPY_OFF,              "metacopy=off"},
        {OPT_ERR,                       NULL}
 };
 

@@ -444,6 +456,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
                        config->index = false;
                        break;
 
+               case OPT_METACOPY_ON:
+                       config->metacopy = true;
+                       break;
+
+               case OPT_METACOPY_OFF:
+                       config->metacopy = false;
+                       break;
+
                default:
                        pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
                        return -EINVAL;

@@ -657,9 +677,11 @@ static int ovl_lower_dir(const char *name, struct path *path,
         * The inodes index feature needs to encode and decode file
         * handles, so it requires that all layers support them.
         */
-       if (ofs->config.index && !ovl_can_decode_fh(path->dentry->d_sb)) {
+       if ((ofs->config.index || ofs->config.metacopy) &&
+            !ovl_can_decode_fh(path->dentry->d_sb)) {
+               pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off and metacopy=off.\n", name);
                ofs->config.index = false;
-               pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
+               ofs->config.metacopy = false;
        }
 
        return 0;

@@ -923,7 +945,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
                ofs->noxattr = true;
                ofs->config.redirect_dir = false;
                ofs->config.index = false;
-               pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
+               ofs->config.metacopy = false;
+               pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off, metacopy=off and no opaque dir.\n");
        } else {
                vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
        }

@@ -1164,6 +1187,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.redirect_dir = ovl_redirect_dir_def;
        ofs->config.index = ovl_index_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

@@ -1209,6 +1233,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
                ofs->same_sb = NULL;
 
+       if (!(ovl_force_readonly(ofs)) && ofs->config.metacopy) {

Quoted 12 lines

fs/overlayfs/super.c +1 -0

@@ -1164,6 +1187,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.redirect_dir = ovl_redirect_dir_def;
        ofs->config.index = ovl_index_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

@@ -1209,6 +1233,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
                ofs->same_sb = NULL;
 

fs/overlayfs/super.c static int ovl_fill_super(struct super_block *sb, void *data, int silent)

Quoted 11 lines

fs/overlayfs/super.c +10 -0

+       if (!(ovl_force_readonly(ofs)) && ofs->config.metacopy) {
+               /* Verify lower root is upper root origin */
+               err = ovl_verify_origin(upperpath.dentry,
+                                       oe->lowerstack[0].dentry, false, true);
+               if (err) {
+                       pr_err("overlayfs: failed to verify upper root origin\n");
+                        goto out_free_oe;
+               }
+       }
+
 

Fs can have upper but no workdir and you still need to verify lower If metacopy is enabled

On Fri, Nov 17, 2017 at 01:23:58PM +0200, Amir Goldstein wrote:

[..]

Quoted 12 lines

Quoted 12 lines

fs/overlayfs/super.c +1 -0

@@ -1164,6 +1187,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.redirect_dir = ovl_redirect_dir_def;
        ofs->config.index = ovl_index_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

@@ -1209,6 +1233,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
                ofs->same_sb = NULL;
 

Quoted 14 lines

fs/overlayfs/super.c static int ovl_fill_super(struct super_block *sb, void *data, int silent)

Quoted 11 lines

fs/overlayfs/super.c +10 -0

+       if (!(ovl_force_readonly(ofs)) && ofs->config.metacopy) {
+               /* Verify lower root is upper root origin */
+               err = ovl_verify_origin(upperpath.dentry,
+                                       oe->lowerstack[0].dentry, false, true);
+               if (err) {
+                       pr_err("overlayfs: failed to verify upper root origin\n");
+                        goto out_free_oe;
+               }
+       }
+
 

Fs can have upper but no workdir and you still need to verify lower If metacopy is enabled

Hmm..., trying to understand this. This probably is more involved.

So first use case is that if metacopy is being enabled, verify lower root is upper root origin. (Even if it is read-only fs).

if (ofs->upper_mnt && ofs->config.metacopy)
ovl_verify_origin().

But this does not cover the case of same fs being remounted with metacopy=off. In that case we will not do metacopy only copy ups but existing metacopy inodes will still require that lower does not change.

Will it make sense to set OVL_METACOPY xattr on upper when metacopy=on is done first time on mount. And later in subsequent mounts, if METACOPY is set on upper, make sure to verify origin and make sure lower supports file handles etc.

Quoted 30 lines · Vivek Goyal

On Fri, Nov 17, 2017 at 01:23:58PM +0200, Amir Goldstein wrote:

[..]

Quoted 12 lines

Quoted 12 lines

fs/overlayfs/super.c +1 -0

@@ -1164,6 +1187,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.redirect_dir = ovl_redirect_dir_def;
        ofs->config.index = ovl_index_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

@@ -1209,6 +1233,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
                ofs->same_sb = NULL;
 

Quoted 14 lines

fs/overlayfs/super.c static int ovl_fill_super(struct super_block *sb, void *data, int silent)

Quoted 11 lines

fs/overlayfs/super.c +10 -0

+       if (!(ovl_force_readonly(ofs)) && ofs->config.metacopy) {
+               /* Verify lower root is upper root origin */
+               err = ovl_verify_origin(upperpath.dentry,
+                                       oe->lowerstack[0].dentry, false, true);
+               if (err) {
+                       pr_err("overlayfs: failed to verify upper root origin\n");
+                        goto out_free_oe;
+               }
+       }
+
 

Fs can have upper but no workdir and you still need to verify lower If metacopy is enabled

Quoted 7 lines

Hmm..., trying to understand this. This probably is more involved.

So first use case is that if metacopy is being enabled, verify lower root is upper root origin. (Even if it is read-only fs).

if (ofs->upper_mnt && ofs->config.metacopy)
ovl_verify_origin().

Yes, this is needed in case workdir/work dir creation fails and fs is mounted read-only. You should not allow it with metacopy on unverified lower.

Quoted 10 lines

But this does not cover the case of same fs being remounted with metacopy=off. In that case we will not do metacopy only copy ups but existing metacopy inodes will still require that lower does not change.

Will it make sense to set OVL_METACOPY xattr on upper when metacopy=on is done first time on mount. And later in subsequent mounts, if METACOPY is set on upper, make sure to verify origin and make sure lower supports file handles etc.

This is an related to what we discussed at length on the ovl-features patches.

Yes, we SHOULD mark that the metacopy feature was enabled, at least on first meta_copy_up().

No, we should not do that by marking root upper with metacopy xattr. this would be very confusing and inconsistent.
We can either set trusted.overlay.features on upper root as Miklos suggested or incompat_feature/ directory as I suggested, but we need to decide on a consistent scheme we can use for other features as well.

I guess metacopy must have this infrastructure in place, because as you correctly noted, we should not mount an overlay with metacopied inodes without verifying lower and file handle support.

Right now two copy up related functions are part of inode.c. Amir suggested it might be better to move these to copy_up.c. Hence this patch does that.

There will one more related function which will come in later patch.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c   | 30 ------------------------------
 2 files changed, 30 insertions(+), 30 deletions(-)

fs/overlayfs/copy_up.c +20 -0

index 303794bb9127..f84ba12c9b05 100644
@@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
        return err;
 }
 
+static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
+{
+       if (ovl_dentry_upper(dentry) &&
+           ovl_dentry_has_upper_alias(dentry))
+               return false;
+
+       if (special_file(d_inode(dentry)->i_mode))
+               return false;
+
+       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+               return false;
+
+       return true;
+}
+
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
+{
+       int err = 0;
+
+       if (ovl_open_need_copy_up(dentry, file_flags)) {

Quoted 2 lines · Vivek Goyal

Right now two copy up related functions are part of inode.c. Amir suggested it might be better to move these to copy_up.c. Hence this patch does that.

IMO it is better to phrase simply "Move the copy up helpers to copy_up.c."

Quoted 41 lines

There will one more related function which will come in later patch.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c   | 30 ------------------------------
 2 files changed, 30 insertions(+), 30 deletions(-)

fs/overlayfs/copy_up.c +25 -0

index 303794bb9127..f84ba12c9b05 100644
@@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
        return err;
 }
 
+static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
+{
+       if (ovl_dentry_upper(dentry) &&
+           ovl_dentry_has_upper_alias(dentry))
+               return false;
+
+       if (special_file(d_inode(dentry)->i_mode))
+               return false;
+
+       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+               return false;
+
+       return true;
+}
+
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
+{
+       int err = 0;
+
+       if (ovl_open_need_copy_up(dentry, file_flags)) {
+               err = ovl_want_write(dentry);
+               if (!err) {
+                       err = ovl_copy_up_flags(dentry, file_flags);
+                       ovl_drop_write(dentry);
+               }

fs/overlayfs/copy_up.c int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat) · line 264

Quoted 5 lines

fs/overlayfs/copy_up.c +5 -0

+       }
+
+       return err;
+}
+

I would put there at the bottom ,below ovl_copy_up_flags, where they belong logically (this file is sorted bottom up.

And also need to move the declaration of ovl_open_maybe_copy_up() in overlayfs.h to the respective location.

Quoted 47 lines · Amir Goldstein

Quoted 2 lines · Vivek Goyal

Right now two copy up related functions are part of inode.c. Amir suggested it might be better to move these to copy_up.c. Hence this patch does that.

IMO it is better to phrase simply "Move the copy up helpers to copy_up.c."

Quoted 41 lines

There will one more related function which will come in later patch.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c   | 30 ------------------------------
 2 files changed, 30 insertions(+), 30 deletions(-)

fs/overlayfs/copy_up.c +25 -0

index 303794bb9127..f84ba12c9b05 100644
@@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
        return err;
 }
 
+static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
+{
+       if (ovl_dentry_upper(dentry) &&
+           ovl_dentry_has_upper_alias(dentry))
+               return false;
+
+       if (special_file(d_inode(dentry)->i_mode))
+               return false;
+
+       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+               return false;
+
+       return true;
+}
+
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
+{
+       int err = 0;
+
+       if (ovl_open_need_copy_up(dentry, file_flags)) {
+               err = ovl_want_write(dentry);
+               if (!err) {
+                       err = ovl_copy_up_flags(dentry, file_flags);
+                       ovl_drop_write(dentry);
+               }

Quoted 8 lines

fs/overlayfs/copy_up.c int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat) · line 264

Quoted 5 lines

fs/overlayfs/copy_up.c +5 -0

+       }
+
+       return err;
+}
+

I would put there at the bottom ,below ovl_copy_up_flags, where they belong logically (this file is sorted bottom up.

Ok, I will keep these functions below ovl_copy_up_flag().

But ovl_copy_up_meta_inode_data() is called by ovl_copy_up_one(). So will keep that above ovl_copy_up_one().

If it makes sense to copy up only metadata during copy up, do it. This is done for regular files which are not opened for WRITE and have origin being saved.

Right now ->metacopy is set to 0 always. Last patch in the series will remove the hard coded statement and enable metacopy feature.

Signed-off-by Vivek Goyal <[email protected]>

Now we will have the capability to have upper inodes which might be only metadata copy up and data is still on lower inode. So add a new xattr OVL_XATTR_METACOPY to distinguish between two cases.

Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata only and and data will be copied up later from lower origin.
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_UPPERDATA which reflects whether ovl inode has data or not (as opposed to metadata only copy up).

If a file is copied up metadata only and later when same file is opened for WRITE, then data copy up takes place. We copy up data, remove METACOPY xattr and then set the UPPERDATA flag in ovl_entry->flags. While all these operations happen with oi->lock held, read side of oi->flags can be lockless. That is another thread on another cpu can check if UPPERDATA flag is set or not.

So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if another cpu sees UPPERDATA flag set, then it should be guaranteed that effects of data copy up and remove xattr operations are also visible.

For example.

CPU1 CPU2
ovl_d_real() acquire(oi->lock)
ovl_open_maybe_copy_up() ovl_copy_up_data()
open_open_need_copy_up() vfs_removexattr()
ovl_already_copied_up()

    ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if CPU1 perceives the effects of setting UPPERDATA flag but not the effects of preceeding operations (ex. upper that is not fully copied up), it will be a problem.

Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation and smp_rmb() on UPPERDATA flag test operation.

May be some other lock or barrier is already covering it. But I am not sure what that is and is it obvious enough that we will not break it in future.

So hence trying to be safe here and introducing barriers explicitly for UPPERDATA flag/bit.

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

ovl_d_real()
ovl_open_maybe_copy_up()
open_open_need_copy_up()
ovl_already_copied_up()
ovl_dentry_needs_data_copy_up()
ovl_test_flag(OVL_UPPERDATA)
<-- OVL_UPPERDATA is not visible yet. copy up file.

I have not introduced any barrier to take care of this. There is already a check ovl_should_check_upperdata() to make sure lower dentry is there otherwise return false (even if OVL_UPPERDATA is not visible yet).

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c   | 55 +++++++++++++++++++++++++++++++---
 fs/overlayfs/dir.c       |  1 +
 fs/overlayfs/overlayfs.h | 10 +++++--
 fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 134 insertions(+), 10 deletions(-)

fs/overlayfs/copy_up.c +51 -4

index b6fa5830c4c6..3f59b98340f7 100644
@@ -195,6 +195,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 = {

@@ -234,7 +244,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
-       if (ovl_already_copied_up(dentry))
+       if (ovl_already_copied_up(dentry, flags))
                return false;
 
        if (special_file(d_inode(dentry)->i_mode))

@@ -486,6 +496,28 @@ 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)
+{
+       struct path upperpath;
+       int err;
+
+       ovl_path_upper(c->dentry, &upperpath);
+       if (WARN_ON(upperpath.dentry == NULL))
+               return -EIO;
+
+       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_set_upperdata(c->dentry);
+       return err;
+}
+
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
        int err;

@@ -519,8 +551,19 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
                        return err;
        }
 
+       if (c->metacopy) {
+               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);
+       if (c->metacopy)
+               err = ovl_set_size(temp, &c->stat);
+       if (!err)
+               err = ovl_set_attr(temp, &c->stat);
        inode_unlock(temp->d_inode);
 
        return err;

@@ -552,6 +595,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
        if (err)
                goto out_cleanup;
 
+       if (!c->metacopy)
+               ovl_set_upperdata(c->dentry);
        inode = d_inode(c->dentry);
        ovl_inode_update(inode, newdentry);
        if (S_ISDIR(inode->i_mode))

@@ -674,7 +719,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)

@@ -684,6 +729,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_locked(dentry, flags))
+                       err = ovl_copy_up_meta_inode_data(&ctx);
                ovl_copy_up_end(dentry);
        }
        do_delayed_call(&done);

@@ -700,7 +747,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
                struct dentry *next;
                struct dentry *parent;
 
-               if (ovl_already_copied_up(dentry))
+               if (ovl_already_copied_up(dentry, flags))
                        break;
 
                next = dget(dentry);

fs/overlayfs/dir.c +0 -0

index e13921824c70..fb3cd73c3693 100644

fs/overlayfs/dir.c +0 -0

                ovl_inode_update(inode, newdentry);
                ovl_copyattr(newdentry->d_inode, inode);
        } else {

fs/overlayfs/overlayfs.h +8 -2

index 7f9a79b5a2b5..d4890e59b881 100644
@@ -27,6 +27,7 @@ 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 {
        /* Pure upper dir that may contain non pure upper entries */

@@ -34,6 +35,7 @@ enum ovl_flag {
        /* Non-merge dir that may contain whiteout entries */
        OVL_WHITEOUTS,
        OVL_INDEX,
+       OVL_UPPERDATA,
 };
 
 /*

@@ -215,6 +217,10 @@ 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_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags);
+bool ovl_has_upperdata(struct dentry *dentry);
+void ovl_set_upperdata(struct dentry *dentry);
 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);

@@ -225,9 +231,9 @@ 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_already_copied_up(struct dentry *dentry);
+bool ovl_already_copied_up(struct dentry *dentry, int flags);
 bool ovl_check_origin_xattr(struct dentry *dentry);
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,

fs/overlayfs/util.c +4 -0

index 572bf46e9f2e..73578bfe9f4b 100644
@@ -231,6 +231,64 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
        oe->has_upper = true;
 }
 
+static bool ovl_should_check_upperdata(struct dentry *dentry) {
+       if (!S_ISREG(d_inode(dentry)->i_mode))
+               return false;
+

fs/overlayfs/util.c +8 -0

+        * if setting of OVL_UPPERDATA is visible, then effects of writes
+        * before that are visible too.
+        */
+       smp_rmb();
+       return true;
+}
+
+void ovl_set_upperdata(struct dentry *dentry) {

Quoted 38 lines · Vivek Goyal

Now we will have the capability to have upper inodes which might be only metadata copy up and data is still on lower inode. So add a new xattr OVL_XATTR_METACOPY to distinguish between two cases.

Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata only and and data will be copied up later from lower origin.
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_UPPERDATA which reflects whether ovl inode has data or not (as opposed to metadata only copy up).

If a file is copied up metadata only and later when same file is opened for WRITE, then data copy up takes place. We copy up data, remove METACOPY xattr and then set the UPPERDATA flag in ovl_entry->flags. While all these operations happen with oi->lock held, read side of oi->flags can be lockless. That is another thread on another cpu can check if UPPERDATA flag is set or not.

So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if another cpu sees UPPERDATA flag set, then it should be guaranteed that effects of data copy up and remove xattr operations are also visible.

For example.

CPU1 CPU2
ovl_d_real() acquire(oi->lock)
ovl_open_maybe_copy_up() ovl_copy_up_data()
open_open_need_copy_up() vfs_removexattr()
ovl_already_copied_up()

    ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if CPU1 perceives the effects of setting UPPERDATA flag but not the effects of preceeding operations (ex. upper that is not fully copied up), it will be a problem.

Quoted 9 lines

Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation and smp_rmb() on UPPERDATA flag test operation.

May be some other lock or barrier is already covering it. But I am not sure what that is and is it obvious enough that we will not break it in future.

So hence trying to be safe here and introducing barriers explicitly for UPPERDATA flag/bit.

Vivek,

I like this version a lot more, but IMO it could still be even simpler.

Quoted 7 lines

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.

Creating new upper is expensive anyway, so I don't think we should care about an unneeded smp_wmb() and probably Miklos will know to tell why it is not needed anyway.

It is very easy to make sure that the OVL_UPPERDATA is always set for the pure upper and non regular file cases and then we have no need for ovl_should_check_upperdata().
Simple is better.

There is just one more thing you need to do before killing ovl_should_check_upperdata() - set the OVL_UPPERDATA on the root inode in ovl_fill_super().

fs/overlayfs/dir.c static void ovl_instantiate(struct dentry *dentry, struct inode *inode, · line 161

Quoted 9 lines

fs/overlayfs/dir.c +1 -0

index e13921824c70..fb3cd73c3693 100644
@@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
        ovl_dentry_version_inc(dentry->d_parent, false);
        ovl_dentry_set_upper_alias(dentry);
        if (!hardlink) {
+               ovl_set_flag(OVL_UPPERDATA, inode);

Call ovl_set_upperdata() instead?

fs/overlayfs/dir.c static void ovl_instantiate(struct dentry *dentry, struct inode *inode,

Quoted 4 lines

fs/overlayfs/dir.c +4 -0

+bool ovl_has_upperdata(struct dentry *dentry) {
+       if (!ovl_should_check_upperdata(dentry))
+               return true;
+

IMO we don't need this check, but you may leave it as if (WARN_ON(!ovl_should_check_upperdata(dentry) after ovl_test_flag() to be on the safe side.

fs/overlayfs/dir.c

Quoted 6 lines

fs/overlayfs/dir.c +4 -0

+       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
+               return false;
+       /*
+        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure

To be consistent, you should update the comment to say pairs with... in ovl_set_upperdata(), although it may be useful to leave the information that the main user of ovl_set_upperdata() is ovl_copy_up_meta_inode_data().

Quoted 13 lines

fs/overlayfs/dir.c +13 -0

+        * if setting of OVL_UPPERDATA is visible, then effects of writes
+        * before that are visible too.
+        */
+       smp_rmb();
+       return true;
+}
+
+void ovl_set_upperdata(struct dentry *dentry) {
+       /*
+        * Pairs with smp_rmb() in ovl_has_upperdata(). Make sure
+        * if OVL_UPPERDATA flag is visible, then effects of write operations
+        * before it are visible as well.
+        */

fs/overlayfs/dir.c

Quoted 5 lines

fs/overlayfs/dir.c +5 -0

+       smp_wmb();
+       ovl_set_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+static inline bool open_for_write(int flags)

Need ovl_ namespace prefix
and the name sounds like an action (i.e. a request to open for write)

fs/overlayfs/dir.c

Quoted 3 lines

fs/overlayfs/dir.c +2 -0

+{
+       if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))

What I meant in previous review is that you make a static inline helper in overlayfs.h to be used by this and ovl_open_need_copy_up(). Your helper is missing the O_TRUNC test, i.e.

return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));

It may sound like we don't *need* to copy up data on O_TRUNC, but in fact, if we don't call copy up functions, we won't be cleaning the metacopy xattr/flag on truncate.

The thing is that the result of O_TRUNC|O_RDONLY is not even well defined by man page ("... Otherwise, the effect of O_TRUNC is unspecified"), but we shouldn't change the way overlayfs treats this strange combination.

Quoted 51 lines · Amir Goldstein

Quoted 38 lines · Vivek Goyal

Now we will have the capability to have upper inodes which might be only metadata copy up and data is still on lower inode. So add a new xattr OVL_XATTR_METACOPY to distinguish between two cases.

Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata only and and data will be copied up later from lower origin.
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_UPPERDATA which reflects whether ovl inode has data or not (as opposed to metadata only copy up).

If a file is copied up metadata only and later when same file is opened for WRITE, then data copy up takes place. We copy up data, remove METACOPY xattr and then set the UPPERDATA flag in ovl_entry->flags. While all these operations happen with oi->lock held, read side of oi->flags can be lockless. That is another thread on another cpu can check if UPPERDATA flag is set or not.

So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if another cpu sees UPPERDATA flag set, then it should be guaranteed that effects of data copy up and remove xattr operations are also visible.

For example.

CPU1 CPU2
ovl_d_real() acquire(oi->lock)
ovl_open_maybe_copy_up() ovl_copy_up_data()
open_open_need_copy_up() vfs_removexattr()
ovl_already_copied_up()

    ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if CPU1 perceives the effects of setting UPPERDATA flag but not the effects of preceeding operations (ex. upper that is not fully copied up), it will be a problem.

Quoted 9 lines

Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation and smp_rmb() on UPPERDATA flag test operation.

May be some other lock or barrier is already covering it. But I am not sure what that is and is it obvious enough that we will not break it in future.

So hence trying to be safe here and introducing barriers explicitly for UPPERDATA flag/bit.

Vivek,

[..]

Quoted 10 lines

I like this version a lot more, but IMO it could still be even simpler.

Quoted 7 lines

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

Quoted 2 lines

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.

Just using ovl_set_upperdata() will not do away with ordering dependency right? I mean, setting OVL_UPPERDATA in file creation path has different ordering requirement (same is the case of >has_upper). And I wanted to highlight that ordering requirement in changelogs.

I can get rid of it. But this seems such a subtle requirement, I think its a good idea to talk about it explicitly.

Quoted 3 lines

Creating new upper is expensive anyway, so I don't think we should care about an unneeded smp_wmb() and probably Miklos will know to tell why it is not needed anyway.

Ok, I can call ovl_set_upperdata() in creation path and not worry about extra unneeded smp_wmb().

Quoted 1 line

Quoted 4 lines

It is very easy to make sure that the OVL_UPPERDATA is always set for the pure upper and non regular file cases and then we have no need for ovl_should_check_upperdata().
Simple is better.

It avoids lots of smp_rmb() calls on files which should not have OVL_UPPERDATA. Now I don't know what is more expensive. smp_rmb() or calling ovl_should_check_upperdata().

Also, it calls ovl_dentry_lower() and that covers the possible race with setting of OVL_UPPERDATA on file creation and setting of oi->__upperdentry.

So if I remove this check, in-theory we open that race.

Quoted 4 lines

There is just one more thing you need to do before killing ovl_should_check_upperdata() - set the OVL_UPPERDATA on the root inode in ovl_fill_super().

That's a good point. Will do.

[..]

Quoted 11 lines

fs/overlayfs/dir.c static void ovl_instantiate(struct dentry *dentry, struct inode *inode, · line 161

Quoted 9 lines

fs/overlayfs/dir.c +1 -0

index e13921824c70..fb3cd73c3693 100644
@@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
        ovl_dentry_version_inc(dentry->d_parent, false);
        ovl_dentry_set_upper_alias(dentry);
        if (!hardlink) {
+               ovl_set_flag(OVL_UPPERDATA, inode);

Call ovl_set_upperdata() instead?

Will do.

[..]

Quoted 9 lines

fs/overlayfs/dir.c static void ovl_instantiate(struct dentry *dentry, struct inode *inode,

Quoted 4 lines

fs/overlayfs/dir.c +4 -0

+bool ovl_has_upperdata(struct dentry *dentry) {
+       if (!ovl_should_check_upperdata(dentry))
+               return true;
+

IMO we don't need this check, but you may leave it as if (WARN_ON(!ovl_should_check_upperdata(dentry) after ovl_test_flag() to be on the safe side.

I primarily kept it as it avoided smp_rmb() for directories and non-regular files.

Also not sure how can I use it as WARN_ON(). Now OVL_UPPERDATA is set on all files/dir. So we will hit WARN_ON() all the time?

Quoted 11 lines

fs/overlayfs/dir.c

Quoted 6 lines

fs/overlayfs/dir.c +4 -0

+       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
+               return false;
+       /*
+        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure

To be consistent, you should update the comment to say pairs with... in ovl_set_upperdata(), although it may be useful to leave the information that the main user of ovl_set_upperdata() is ovl_copy_up_meta_inode_data().

Ok.

Quoted 13 lines

Quoted 13 lines

fs/overlayfs/dir.c +13 -0

+        * if setting of OVL_UPPERDATA is visible, then effects of writes
+        * before that are visible too.
+        */
+       smp_rmb();
+       return true;
+}
+
+void ovl_set_upperdata(struct dentry *dentry) {
+       /*
+        * Pairs with smp_rmb() in ovl_has_upperdata(). Make sure
+        * if OVL_UPPERDATA flag is visible, then effects of write operations
+        * before it are visible as well.
+        */

Quoted 8 lines

fs/overlayfs/dir.c

Quoted 5 lines

fs/overlayfs/dir.c +5 -0

+       smp_wmb();
+       ovl_set_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+static inline bool open_for_write(int flags)

Need ovl_ namespace prefix
and the name sounds like an action (i.e. a request to open for write)

ovl_opened_for_write_or_trunc()?

Quoted 16 lines

Quoted 10 lines

Quoted 7 lines

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.

Just using ovl_set_upperdata() will not do away with ordering dependency right? I mean, setting OVL_UPPERDATA in file creation path has different ordering requirement (same is the case of >has_upper). And I wanted to highlight that ordering requirement in changelogs.

Quoted 3 lines

I can get rid of it. But this seems such a subtle requirement, I think its a good idea to talk about it explicitly.

I am not following. you only need to ovl_set_upperdata() before ovl_inode_update(), just like in regular copy up. Am I missing something subtle?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

+static inline bool open_for_write(int flags)

Need ovl_ namespace prefix
and the name sounds like an action (i.e. a request to open for write)

ovl_opened_for_write_or_trunc()?

OK. maybe ovl_open_flags_need_copy_up() ?

On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:

[..]

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

Quoted 7 lines

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.

Just using ovl_set_upperdata() will not do away with ordering dependency right? I mean, setting OVL_UPPERDATA in file creation path has different ordering requirement (same is the case of >has_upper). And I wanted to highlight that ordering requirement in changelogs.

Quoted 7 lines

Quoted 3 lines

I can get rid of it. But this seems such a subtle requirement, I think its a good idea to talk about it explicitly.

I am not following. you only need to ovl_set_upperdata() before ovl_inode_update(), just like in regular copy up. Am I missing something subtle?

Hi Amir,

Right. This is ordering dependency between setting of OVL_UPPERDATA and oi->__upperdentry.

So as per barrier documentation file, read/write barrier should look as follows.

        CPU1                            CPU2
     set OVL_UPPERDATA                  smp_rmb()
     smp_wmb()                          Load oi->__upperdentry
     set oi->__upperdentry

So we need to place smp_wmb() after setting OVL_UPPERDATA and place smp_rmb() just before load of oi->__upperdentry.

Instead of smp_rmb() we are relying on checking for ovl_dentry_lower(). That is even if OVL_UPPERDATA is not visible on CPU2, but oi->__upperdentry is visible, then we don't try to copy up a file which does not have a lower.

So ovl_dentry_lower() is preventing the race which *in-theory* can trigger trying to copy up file which does not have lower.

Does this make sense?

Quoted 48 lines · Vivek Goyal

On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:

[..]

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

Quoted 7 lines

Note, we now set OVL_UPPERDATA on all kind of dentires and check OVL_UPPERDATA only where it makes sense. If a file is created on upper, we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real() path again. This time ordering dependency is w.r.t oe->__upperdata. We need to make surr OVL_UPPERDATA is visible before oe->__upperdata is visible. Otherwise following code path might try to copy up an upper only file.

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.

Just using ovl_set_upperdata() will not do away with ordering dependency right? I mean, setting OVL_UPPERDATA in file creation path has different ordering requirement (same is the case of >has_upper). And I wanted to highlight that ordering requirement in changelogs.

Quoted 7 lines

Quoted 3 lines

I can get rid of it. But this seems such a subtle requirement, I think its a good idea to talk about it explicitly.

I am not following. you only need to ovl_set_upperdata() before ovl_inode_update(), just like in regular copy up. Am I missing something subtle?

Hi Amir,

Right. This is ordering dependency between setting of OVL_UPPERDATA and oi->__upperdentry.

So as per barrier documentation file, read/write barrier should look as follows.

        CPU1                            CPU2
     set OVL_UPPERDATA                  smp_rmb()
     smp_wmb()                          Load oi->__upperdentry
     set oi->__upperdentry

So we need to place smp_wmb() after setting OVL_UPPERDATA and place smp_rmb() just before load of oi->__upperdentry.

Instead of smp_rmb() we are relying on checking for ovl_dentry_lower(). That is even if OVL_UPPERDATA is not visible on CPU2, but oi->__upperdentry is visible, then we don't try to copy up a file which does not have a lower.

Quoted 5 lines

So ovl_dentry_lower() is preventing the race which *in-theory* can trigger trying to copy up file which does not have lower.

Does this make sense?

Maybe. Brain overflow.
To put it shortly, if there is a problem with setting OVL_UPPERDATA on new upper then there is a problem with setting upper alias as well (as you pointed out). I don't think that is really the case, but I am not the one to explain why. It just does not make sense that d_instantiate doesn't make sure that inode fields are consistent before attaching the inode to dentry.
I think the barriers hide in raw_write_seqcount_begin()/raw_seqcount_begin() for the lockless dentry cache lookup, but I'm not an expert.

Anyway, I find the commit message text from "If a file is created on upper,..." very confusing.
I suggest that if you are not sure about this leave a "XXX" comment in the code path of creating new upper and setting OVL_UPPERDATA flag or "TODO" comment if you thing there is something to be done.

It is easy to later apply a patch to remove the XXX comment after explaining it or remove TODO comment after fixing it.

A confusing text remains in git log forever and get git history researchers off track.

Amir.

If an inode has been copied up metadata only, then we need to query the number of blocks from lower and fill up the stat->st_blocks.

We need to be careful about races where we are doing stat on one cpu and data copy up is taking place on other cpu. We want to return stat->st_blocks either from lower or stable upper and not something in between. Hence, ovl_has_upperdata() is called first to figure out whether block reporting will take place from lower or upper.

Signed-off-by Vivek Goyal <[email protected]>

During lookup, check for presence of OVL_XATTR_METACOPY and if not present, set OVL_UPPERDATA bit in flags. This is only done for upper inode with a corresponding lower dentry and file needs to be a regular file. Basically any file which is eligible for metadata only copy up.

Signed-off-by Vivek Goyal <[email protected]>

d_real() can make a upper metacopy dentry/inode visible to the vfs layer. This is something new and vfs layer does not know that this inode contains only metadata and not data. And this could break things.

So to be safe, do not expose metacopy only dentry/inode to vfs using d_real().

For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper dentry inode is metacopy only and does not have data, return lower dentry.

If d_real() is called with flag D_REAL_UPPER, return upper dentry only if it has data (flag OVL_UPPERDATA is set).

Similiarly, if d_real(inode=X) is called, a warning is emitted if returned dentry/inode does not have OVL_UPPERDATA set. This should not happen as we never made this metacopy inode visible to vfs so nobody should be calling overlayfs back with inode=metacopy_inode.

I scanned the code and I don't think it breaks any of the existing code. There are two users of D_REAL_UPPER. may_write_real() and update_ovl_inode_times().

may_write_real(), will get an NULL dentry if upper inode is metacopy only and it will return -EPERM. Effectively, we are disallowing modifications to metacopy only inode from this interface. Though there is opportunity to improve it. (Allow chattr on metacopy inodes).

update_ovl_inode_times() gets inode mtime and ctime from real inode. It should not be broken for metacopy inode as well for following reasons.

- For any metadata operations (setattr, acl etc), overlay always calls ovl_copyattr() and updates ovl inode mtime and ctime. So there is no need to update mtime and ctime int his case. Its already updated.

- For metadata inode, mtime should be same as lower and not change. (data can't be modified on metadata inode without copyup).

- For file writes, ctime and mtime will be updated. But in that case first data will be copied up and this will not be a metadata inode anymore. And furthr call to d_real(D_REAL_UPPER) will return upper inode and new mtime and ctime will be obtainable.

So atime updates should work just fine for metacopy inodes.

Signed-off-by Vivek Goyal <[email protected]>

If file is metacopy only, it is possible that lower is encrypted while other is not. In that case, report file as encrypted (despite the fact that only data is encrypted while metadata is not).

Similarly if lower is compressed, report that in stat for a metacopy only file.

Signed-off-by Vivek Goyal <[email protected]>

fs/overlayfs/inode.c +18 -2

+static void ovl_stat_fix_attributes(struct kstat *ustat, struct kstat *lstat) {
+       unsigned int attr_mask, attr;
+
+       attr_mask = lstat->attributes_mask & OVL_STATX_ATTR_MASK;
+       if (!attr_mask)
+               return;
+
+       attr = lstat->attributes & attr_mask;
+       if (!attr)
+               return;
+
+       ustat->attributes_mask |= attr_mask;
+       ustat->attributes |= attr;
+}
+
 int ovl_getattr(const struct path *path, struct kstat *stat,
                u32 request_mask, unsigned int flags)
 {

@@ -123,8 +139,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
                        else
                                stat->dev = ovl_get_pseudo_dev(dentry);
 
-                       if (metacopy)
+                       if (metacopy) {
                                stat->blocks = lowerstat.blocks;
+                               ovl_stat_fix_attributes(stat, &lowerstat);
+                       }
                }
                if (samefs) {
                        /*
--

2.13.6