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 metadata copyup is conditional on index=on. If index=off and user specifies metacopy=on, it goes back to metacopy=off and a warning is printed.

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

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

---
 fs/overlayfs/Kconfig     |  9 +++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

fs/overlayfs/Kconfig +9 -0

index cbfc196e5dc5..94d4c61719c8 100644
@@ -43,3 +43,12 @@ 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
+       depends on OVERLAY_FS_INDEX
+       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 25d9b5adcd42..6806f0b0fbc2 100644
@@ -15,6 +15,7 @@ struct ovl_config {
        bool default_permissions;
        bool redirect_dir;
        bool index;
+       bool metacopy;
 };
 
 /* private information held for overlayfs's superblock */

fs/overlayfs/super.c +32 -1

index 092d150643c1..6f4c32e49298 100644
@@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
                 "Default to on or off for the inodes index feature");
 
+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;

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

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

@@ -348,6 +358,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}
 };
 

@@ -428,6 +440,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;

@@ -847,6 +867,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ufs->config.redirect_dir = ovl_redirect_dir_def;
        ufs->config.index = ovl_index_def;
+       if (ovl_metacopy_def && !ovl_index_def) {
+               pr_warn("overlayfs: metadata copy up can not be enabled by default as index feature is not enabled by default.\n");
+               ovl_metacopy_def = false;
+       }
+       ufs->config.metacopy = ovl_metacopy_def;
+
        err = ovl_parse_opt((char *) data, &ufs->config);
        if (err)
                goto out_free_config;

@@ -1091,6 +1117,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        if (!ufs->indexdir)
                ufs->config.index = false;
 
+       /* As of now metacopy=on is dependent on index=on */
+       if (ufs->config.metacopy && !ufs->config.index) {
+               pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
+               ufs->config.metacopy = false;
+       }
+
        if (remote)
                sb->s_d_op = &ovl_reval_dentry_operations;
        else
--

2.13.5

Quoted 7 lines · Vivek Goyal

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

Also metadata copyup is conditional on index=on. If index=off and user specifies metacopy=on, it goes back to metacopy=off and a warning is printed.

So this commit does not explain why the features are dependent And when I think about it again, they should not be dependent.

I had several arguments for having metacopy rely on index, but You shot down the idea to index all regular files for performance Of index cleanup on mount.

Now the only remaining reason is not setting same origin for Broken upper hardlinks when index is off, the solution should Be to not metacopy lower hardlinks when index is off. IMO it's a minor trade-off for keeping the features independent.

Quoted 128 lines

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

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

---
 fs/overlayfs/Kconfig     |  9 +++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

fs/overlayfs/Kconfig +9 -0

index cbfc196e5dc5..94d4c61719c8 100644
@@ -43,3 +43,12 @@ 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
+       depends on OVERLAY_FS_INDEX
+       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 25d9b5adcd42..6806f0b0fbc2 100644
@@ -15,6 +15,7 @@ struct ovl_config {
        bool default_permissions;
        bool redirect_dir;
        bool index;
+       bool metacopy;
 };
 
 /* private information held for overlayfs's superblock */

fs/overlayfs/super.c +32 -1

index 092d150643c1..6f4c32e49298 100644
@@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
                 "Default to on or off for the inodes index feature");
 
+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;

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

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

@@ -348,6 +358,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}
 };
 

@@ -428,6 +440,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;

@@ -847,6 +867,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ufs->config.redirect_dir = ovl_redirect_dir_def;
        ufs->config.index = ovl_index_def;
+       if (ovl_metacopy_def && !ovl_index_def) {
+               pr_warn("overlayfs: metadata copy up can not be enabled by default as index feature is not enabled by default.\n");
+               ovl_metacopy_def = false;
+       }
+       ufs->config.metacopy = ovl_metacopy_def;
+
        err = ovl_parse_opt((char *) data, &ufs->config);
        if (err)
                goto out_free_config;

@@ -1091,6 +1117,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        if (!ufs->indexdir)
                ufs->config.index = false;
 
+       /* As of now metacopy=on is dependent on index=on */
+       if (ufs->config.metacopy && !ufs->config.index) {
+               pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
+               ufs->config.metacopy = false;
+       }
+
        if (remote)
                sb->s_d_op = &ovl_reval_dentry_operations;
        else
--

2.13.5

Quoted 12 lines · Amir Goldstein

Quoted 7 lines · Vivek Goyal

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

Also metadata copyup is conditional on index=on. If index=off and user specifies metacopy=on, it goes back to metacopy=off and a warning is printed.

So this commit does not explain why the features are dependent And when I think about it again, they should not be dependent.

Quoted 8 lines

I had several arguments for having metacopy rely on index, but You shot down the idea to index all regular files for performance Of index cleanup on mount.

Now the only remaining reason is not setting same origin for Broken upper hardlinks when index is off, the solution should Be to not metacopy lower hardlinks when index is off. IMO it's a minor trade-off for keeping the features independent.

Hi Amir,

Hey, you were the one who pushed me hard to make it dependent on index. :-) I was more than happy to keep it as independent functionality.

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

Quoted 129 lines

Quoted 129 lines

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

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

---
 fs/overlayfs/Kconfig     |  9 +++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

fs/overlayfs/Kconfig +9 -0

index cbfc196e5dc5..94d4c61719c8 100644
@@ -43,3 +43,12 @@ 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
+       depends on OVERLAY_FS_INDEX
+       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 25d9b5adcd42..6806f0b0fbc2 100644
@@ -15,6 +15,7 @@ struct ovl_config {
        bool default_permissions;
        bool redirect_dir;
        bool index;
+       bool metacopy;
 };
 
 /* private information held for overlayfs's superblock */

fs/overlayfs/super.c +32 -1

index 092d150643c1..6f4c32e49298 100644
@@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
                 "Default to on or off for the inodes index feature");
 
+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;

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

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

@@ -348,6 +358,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}
 };
 

@@ -428,6 +440,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;

@@ -847,6 +867,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ufs->config.redirect_dir = ovl_redirect_dir_def;
        ufs->config.index = ovl_index_def;
+       if (ovl_metacopy_def && !ovl_index_def) {
+               pr_warn("overlayfs: metadata copy up can not be enabled by default as index feature is not enabled by default.\n");
+               ovl_metacopy_def = false;
+       }
+       ufs->config.metacopy = ovl_metacopy_def;
+
        err = ovl_parse_opt((char *) data, &ufs->config);
        if (err)
                goto out_free_config;

@@ -1091,6 +1117,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
        if (!ufs->indexdir)
                ufs->config.index = false;
 
+       /* As of now metacopy=on is dependent on index=on */
+       if (ufs->config.metacopy && !ufs->config.index) {
+               pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
+               ufs->config.metacopy = false;
+       }
+
        if (remote)
                sb->s_d_op = &ovl_reval_dentry_operations;
        else
--

2.13.5

Quoted 5 lines

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

Quoted 4 lines

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

It is not broken now because we intentionally do NOT set origin when copying up lower hardlinks and index is disabled.
You must not break this rule, so instead you can avoid metacopy for lower hardlinks when index is disabled.

On Wed, Oct 11, 2017 at 07:29:38PM +0300, Amir Goldstein wrote:

[..]

Quoted 5 lines

Quoted 5 lines

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

Quoted 8 lines

Quoted 4 lines

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

It is not broken now because we intentionally do NOT set origin when copying up lower hardlinks and index is disabled.
You must not break this rule, so instead you can avoid metacopy for lower hardlinks when index is disabled.

Hi Amir,

Ok, I am open to change it so that if index=off, lower hardlinks are not copied up metadata only.

But please help me understand that what *additional* thing breaks if origin is set when index=off.

Say lower has a file foo.txt and another hard link foo-link.txt. (say index=off, metacopy=off).

Say, I open foo-link.txt for WRITE, and it gets copied up. Now there is no link between foo-link.txt and foo.txt and if a user opens foo.txt for READ, it does not see updates to foo-link.txt. So this is the broken behavior of hardlinks with index=off, as of today.

Now I go back to original state and this time do same operation with (indxex=off, metacopy=on). Then "chown foo-link.txt" will only copy metadata and set origin to lower file. Say I also chown "foo.txt", that will do the same thing. Now both foo.txt and foo-link.txt will have ORIGIN pointing to same lower inode. If any of the file is opened for WRITE, data will be copied up from same origin.

So setting same ORIGIN on two upper files, does not seem to break anything additional, AFAICS. What am I missing.

Quoted 40 lines · Vivek Goyal

On Wed, Oct 11, 2017 at 07:29:38PM +0300, Amir Goldstein wrote:

[..]

Quoted 5 lines

Quoted 5 lines

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

Quoted 8 lines

Quoted 4 lines

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

It is not broken now because we intentionally do NOT set origin when copying up lower hardlinks and index is disabled.
You must not break this rule, so instead you can avoid metacopy for lower hardlinks when index is disabled.

Hi Amir,

Ok, I am open to change it so that if index=off, lower hardlinks are not copied up metadata only.

But please help me understand that what *additional* thing breaks if origin is set when index=off.

Say lower has a file foo.txt and another hard link foo-link.txt. (say index=off, metacopy=off).

Say, I open foo-link.txt for WRITE, and it gets copied up. Now there is no link between foo-link.txt and foo.txt and if a user opens foo.txt for READ, it does not see updates to foo-link.txt. So this is the broken behavior of hardlinks with index=off, as of today.

Now I go back to original state and this time do same operation with (indxex=off, metacopy=on). Then "chown foo-link.txt" will only copy metadata and set origin to lower file. Say I also chown "foo.txt", that will do the same thing. Now both foo.txt and foo-link.txt will have ORIGIN pointing to same lower inode. If any of the file is opened for WRITE, data will be copied up from same origin.

Quoted 3 lines

So setting same ORIGIN on two upper files, does not seem to break anything additional, AFAICS. What am I missing.

Sorry for brevity of my response earlier. I was hopping on a plane. The problem is a bit convoluted to explain, but here goes.

ORIGIN was introduced to implement constant st_ino across copy up. stat(2) of an overlay file with ORIGIN (both layer in same fs) returns st_ino of origin inode.

When copy up breaks lower hardlink to *different* upper inodes, then those broken aliases MUST NOT return the same st_ino, because they are different files with different data.

Only when index is enabled and copy up does not break lower hardlink, it is legal to set ORIGIN for every upper alias, because all upper aliases can and should return same st_ino (the origin st_ino) from stat(2).

So that is the simplest way to explain why you MUST NOT set ORIGIN on copy up of lower hardlink when index is disabled and therefore, you cannot use ORIGIN for metacopy.

As I wrote in some email, you could set METAORIGIN xattr in this case just for the use of metacopy, but I rather not have this special case.

Beyond the problem stated above with st_ino of broken hardlinks, if broken hardlinks have the same ORIGIN, this also breaks indexing assumptions and can lead to EIO on lookup, but no need to get into that.

Quoted 48 lines · Amir Goldstein

Quoted 40 lines · Vivek Goyal

On Wed, Oct 11, 2017 at 07:29:38PM +0300, Amir Goldstein wrote:

[..]

Quoted 5 lines

Quoted 5 lines

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

Quoted 8 lines

Quoted 4 lines

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

It is not broken now because we intentionally do NOT set origin when copying up lower hardlinks and index is disabled.
You must not break this rule, so instead you can avoid metacopy for lower hardlinks when index is disabled.

Hi Amir,

Ok, I am open to change it so that if index=off, lower hardlinks are not copied up metadata only.

But please help me understand that what *additional* thing breaks if origin is set when index=off.

Say lower has a file foo.txt and another hard link foo-link.txt. (say index=off, metacopy=off).

Say, I open foo-link.txt for WRITE, and it gets copied up. Now there is no link between foo-link.txt and foo.txt and if a user opens foo.txt for READ, it does not see updates to foo-link.txt. So this is the broken behavior of hardlinks with index=off, as of today.

Now I go back to original state and this time do same operation with (indxex=off, metacopy=on). Then "chown foo-link.txt" will only copy metadata and set origin to lower file. Say I also chown "foo.txt", that will do the same thing. Now both foo.txt and foo-link.txt will have ORIGIN pointing to same lower inode. If any of the file is opened for WRITE, data will be copied up from same origin.

Quoted 3 lines

So setting same ORIGIN on two upper files, does not seem to break anything additional, AFAICS. What am I missing.

Sorry for brevity of my response earlier. I was hopping on a plane. The problem is a bit convoluted to explain, but here goes.

Quoted 7 lines

ORIGIN was introduced to implement constant st_ino across copy up. stat(2) of an overlay file with ORIGIN (both layer in same fs) returns st_ino of origin inode.

When copy up breaks lower hardlink to *different* upper inodes, then those broken aliases MUST NOT return the same st_ino, because they are different files with different data.

Hi Amir,

I thought current code already takes care of this situation.

ovl_getattr() {
/*
* Lower hardlinks may be broken on copy up to different
* upper files, so we cannot use the lower origin st_ino
* for those different files, even for the same fs case.
* With inodes index enabled, it is safe to use st_ino
* of an indexed hardlinked origin. The index validates
* that the upper hardlink is not broken.
*/
if (is_dir || lowerstat.nlink == 1 ||
ovl_test_flag(OVL_INDEX, d_inode(dentry)))
stat->ino = lowerstat.ino;
}

So if lower had nlink > 1, then its inode number will not be used if indexing was not enabled. If that's the case, two upper pointing to same origin should not be a problem?

I even tested it. Wrote a patch to force setting origin even on hard links with index=off.

Before copy up, both the files (testfile.txt and link-file.txt) had same inode numbers. The moment file is copied up it uses its real inode number on upper (and not the one retrieved from lower). IOW, origin inode number is discarded if index=off. If that's the case then setting ORIGIN with broken hard link should be just fine. It can still be used for metadata copyup while it can't be used with index=off.

In fact this sounds better to me. Make use of ORIGIN information only if it is safe. With broken hardlinks it is not safe to make use of this information so ignore ORIGIN. But it is still ok to make use of this information from metadata copyup point of view.

Quoted 10 lines

Only when index is enabled and copy up does not break lower hardlink, it is legal to set ORIGIN for every upper alias, because all upper aliases can and should return same st_ino (the origin st_ino) from stat(2).

So that is the simplest way to explain why you MUST NOT set ORIGIN on copy up of lower hardlink when index is disabled and therefore, you cannot use ORIGIN for metacopy.

Quoted 6 lines

As I wrote in some email, you could set METAORIGIN xattr in this case just for the use of metacopy, but I rather not have this special case.

Beyond the problem stated above with st_ino of broken hardlinks, if broken hardlinks have the same ORIGIN, this also breaks indexing assumptions and can lead to EIO on lookup, but no need to get into that.

I guess you are referring to ovl_lookup_index().

 () {
        index = lookup_one_len_unlocked(name.name, ofs->indexdir, name.len);
        if (d_is_negative(index)) {
                if (upper && d_inode(origin)->i_nlink > 1) {
                        pr_warn_ratelimited("overlayfs: hard link with origin but no index (ino=%lu).\n",
                                            d_inode(origin)->i_ino);
                        goto fail;
                }

        }
}

So this is failing if we find a dentry which has origin but no index. And this will hit if a overlay was mounted with index=off, hard link copied up and then remounted with index=on. In that case it will return -EIO.

My question is, that does this have to be an error. If we are supporting this use case, where index can be turned on later, then can we just warn and continue? Or set OVL_XATTR_INDEX on upper to indicate that this upper should have an index.

I mean ORIGIN started with the exclusive purpose of inode number stability. But this is sort of infrastructure which keeps track of ORIGIN of copy up source and can be used for metadata copy up as well. So indexing should not put restrictions on what files ORIGIN can be set on. Instead both metacopy and index should not make use of ORIGIN where they this things are broken for them.

Thoughts?

Quoted 90 lines · Vivek Goyal

Quoted 48 lines · Amir Goldstein

Quoted 40 lines · Vivek Goyal

On Wed, Oct 11, 2017 at 07:29:38PM +0300, Amir Goldstein wrote:

[..]

Quoted 5 lines

Quoted 5 lines

Anyway, I did not understand what's the problem with borken upper hardlinks when index=off. If two upper hardlinks (broken), can point to same ORIGIN on lower, they will just copy up same data.

Quoted 8 lines

Quoted 4 lines

IOW, it does not seem to be more broken then what it is now just becase of metadata copy up or because of both upper pointing to same ORIGIN? What am I missing.

It is not broken now because we intentionally do NOT set origin when copying up lower hardlinks and index is disabled.
You must not break this rule, so instead you can avoid metacopy for lower hardlinks when index is disabled.

Hi Amir,

Ok, I am open to change it so that if index=off, lower hardlinks are not copied up metadata only.

But please help me understand that what *additional* thing breaks if origin is set when index=off.

Say lower has a file foo.txt and another hard link foo-link.txt. (say index=off, metacopy=off).

Say, I open foo-link.txt for WRITE, and it gets copied up. Now there is no link between foo-link.txt and foo.txt and if a user opens foo.txt for READ, it does not see updates to foo-link.txt. So this is the broken behavior of hardlinks with index=off, as of today.

Now I go back to original state and this time do same operation with (indxex=off, metacopy=on). Then "chown foo-link.txt" will only copy metadata and set origin to lower file. Say I also chown "foo.txt", that will do the same thing. Now both foo.txt and foo-link.txt will have ORIGIN pointing to same lower inode. If any of the file is opened for WRITE, data will be copied up from same origin.

Quoted 3 lines

So setting same ORIGIN on two upper files, does not seem to break anything additional, AFAICS. What am I missing.

Sorry for brevity of my response earlier. I was hopping on a plane. The problem is a bit convoluted to explain, but here goes.

Quoted 7 lines

ORIGIN was introduced to implement constant st_ino across copy up. stat(2) of an overlay file with ORIGIN (both layer in same fs) returns st_ino of origin inode.

When copy up breaks lower hardlink to *different* upper inodes, then those broken aliases MUST NOT return the same st_ino, because they are different files with different data.

Hi Amir,

I thought current code already takes care of this situation.

ovl_getattr() {
/*
* Lower hardlinks may be broken on copy up to different
* upper files, so we cannot use the lower origin st_ino
* for those different files, even for the same fs case.
* With inodes index enabled, it is safe to use st_ino
* of an indexed hardlinked origin. The index validates
* that the upper hardlink is not broken.
*/
if (is_dir || lowerstat.nlink == 1 ||
ovl_test_flag(OVL_INDEX, d_inode(dentry)))
stat->ino = lowerstat.ino;
}

So if lower had nlink > 1, then its inode number will not be used if indexing was not enabled. If that's the case, two upper pointing to same origin should not be a problem?

I even tested it. Wrote a patch to force setting origin even on hard links with index=off.

Before copy up, both the files (testfile.txt and link-file.txt) had same inode numbers. The moment file is copied up it uses its real inode number on upper (and not the one retrieved from lower). IOW, origin inode number is discarded if index=off. If that's the case then setting ORIGIN with broken hard link should be just fine. It can still be used for metadata copyup while it can't be used with index=off.

Quoted 5 lines

In fact this sounds better to me. Make use of ORIGIN information only if it is safe. With broken hardlinks it is not safe to make use of this information so ignore ORIGIN. But it is still ok to make use of this information from metadata copyup point of view.

Yes, all this is true. I was trying to avoid the full explanation regrading index inconsistency, but I ended up giving an incomplete story.

Quoted 40 lines

Quoted 10 lines

Only when index is enabled and copy up does not break lower hardlink, it is legal to set ORIGIN for every upper alias, because all upper aliases can and should return same st_ino (the origin st_ino) from stat(2).

So that is the simplest way to explain why you MUST NOT set ORIGIN on copy up of lower hardlink when index is disabled and therefore, you cannot use ORIGIN for metacopy.

Quoted 6 lines

As I wrote in some email, you could set METAORIGIN xattr in this case just for the use of metacopy, but I rather not have this special case.

Beyond the problem stated above with st_ino of broken hardlinks, if broken hardlinks have the same ORIGIN, this also breaks indexing assumptions and can lead to EIO on lookup, but no need to get into that.

I guess you are referring to ovl_lookup_index().

 () {
        index = lookup_one_len_unlocked(name.name, ofs->indexdir, name.len);
        if (d_is_negative(index)) {
                if (upper && d_inode(origin)->i_nlink > 1) {
                        pr_warn_ratelimited("overlayfs: hard link with origin but no index (ino=%lu).\n",
                                            d_inode(origin)->i_ino);
                        goto fail;
                }

        }
}

So this is failing if we find a dentry which has origin but no index. And this will hit if a overlay was mounted with index=off, hard link copied up and then remounted with index=on. In that case it will return -EIO.

My question is, that does this have to be an error. If we are supporting this use case, where index can be turned on later, then can we just warn and continue? Or set OVL_XATTR_INDEX on upper to indicate that this upper should have an index.

Quoted 9 lines

I mean ORIGIN started with the exclusive purpose of inode number stability. But this is sort of infrastructure which keeps track of ORIGIN of copy up source and can be used for metadata copy up as well. So indexing should not put restrictions on what files ORIGIN can be set on. Instead both metacopy and index should not make use of ORIGIN where they this things are broken for them.

Thoughts?

It is interesting to note that the commit that introduced this limitation fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink") was merged as a "last minute" (rc7) fix patch before final v4.12 and had this text in commit message:
"We can relax this in the future when we are able to index upper object by
origin."

I am not sure it is easy to relax this limitation, but I may be wrong. I'll take another swing at writing the full story. Hope I will get it right this time...

Index is a 1-to-1 mapping from origin *inode* to upper *inode*. Highlighting *inode* because there may be many lower or upper aliases of that inode.

The index key (its name) is the file handle of origin inode. The index itself is an upper alias (link) of upper inode.

If more than 1 upper inode point to the same origin inode, then index cannot be consistent for both upper aliases. This would actually be a common scenario if hardlinks are broken due to index=off (or kernel v4.12 without the rc7 fix) and then index is turned on and more lower aliases are copied up.

On lookup, we can detected that index found by origin
file handle is not a hardlink of upper inode and ignore it instead of returning EIO on:
if (upper && d_inode(upper) != inode) {
pr_warn_ratelimited("overlayfs: wrong index found
(index=%pd2, ino=%lu, upper ino=%lu).\n",

                                    index, inode->i_ino, d_inode(upper)->i_ino);
                goto fail;
        }

And we could have allowed for "hard link with origin but no index" instead of returning EIO.

In those cases, we would need to get a hashed overlay inode by address of upper inode, instead of by address of origin inode and we would have to not set the OVL_INDEX flag on lookup.

I guess one of the reasons we did not do it on first version of index feature is that we wanted to keep things simple and we did not have a good enough reason to set ORIGIN for broken hardlinks.

So I am now open to the possibility that we may be able to set ORIGIN for broken hardlinks without breaking anything, but will need to see patches before I can say if we missed something. Maybe Miklos can see something that I have missed.

In any case, for the sake of simplicity, I wouldn't bother doing metacopy up of lower hardlinks in first version of metacopy feature.

Thanks for insisting on a good answer (not sure I have provided one yet)

Quoted 1 line · Amir Goldstein

On Wed, Oct 11, 2017 at 9:34 PM, Vivek Goyal <[email protected]> wrote:

Quoted 69 lines

Quoted 9 lines

So this is failing if we find a dentry which has origin but no index. And this will hit if a overlay was mounted with index=off, hard link copied up and then remounted with index=on. In that case it will return -EIO.

My question is, that does this have to be an error. If we are supporting this use case, where index can be turned on later, then can we just warn and continue? Or set OVL_XATTR_INDEX on upper to indicate that this upper should have an index.

Quoted 9 lines

I mean ORIGIN started with the exclusive purpose of inode number stability. But this is sort of infrastructure which keeps track of ORIGIN of copy up source and can be used for metadata copy up as well. So indexing should not put restrictions on what files ORIGIN can be set on. Instead both metacopy and index should not make use of ORIGIN where they this things are broken for them.

Thoughts?

It is interesting to note that the commit that introduced this limitation fbaf94ee3cd5 ("ovl: don't set origin on broken lower hardlink") was merged as a "last minute" (rc7) fix patch before final v4.12 and had this text in commit message:
"We can relax this in the future when we are able to index upper object by
origin."

I am not sure it is easy to relax this limitation, but I may be wrong. I'll take another swing at writing the full story. Hope I will get it right this time...

Index is a 1-to-1 mapping from origin *inode* to upper *inode*. Highlighting *inode* because there may be many lower or upper aliases of that inode.

The index key (its name) is the file handle of origin inode. The index itself is an upper alias (link) of upper inode.

If more than 1 upper inode point to the same origin inode, then index cannot be consistent for both upper aliases. This would actually be a common scenario if hardlinks are broken due to index=off (or kernel v4.12 without the rc7 fix) and then index is turned on and more lower aliases are copied up.

On lookup, we can detected that index found by origin
file handle is not a hardlink of upper inode and ignore it instead of returning EIO on:
if (upper && d_inode(upper) != inode) {
pr_warn_ratelimited("overlayfs: wrong index found
(index=%pd2, ino=%lu, upper ino=%lu).\n",

                                    index, inode->i_ino, d_inode(upper)->i_ino);
                goto fail;
        }

And we could have allowed for "hard link with origin but no index" instead of returning EIO.

In those cases, we would need to get a hashed overlay inode by address of upper inode, instead of by address of origin inode and we would have to not set the OVL_INDEX flag on lookup.

I guess one of the reasons we did not do it on first version of index feature is that we wanted to keep things simple and we did not have a good enough reason to set ORIGIN for broken hardlinks.

So I am now open to the possibility that we may be able to set ORIGIN for broken hardlinks without breaking anything, but will need to see patches before I can say if we missed something. Maybe Miklos can see something that I have missed.

Quoted 2 lines

In any case, for the sake of simplicity, I wouldn't bother doing metacopy up of lower hardlinks in first version of metacopy feature.

Ok, I can see that current code will return -EIO if index is enabled after some copy up have taken place with index=off. So for now, I will disable metadata copyup on hardlinks if index=off.

But it would be nice if we can move away from this restriction.

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.

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

---
 fs/overlayfs/copy_up.c   | 65 ++++++++++++++++++++++++++++++++++++++++++++++--
 fs/overlayfs/inode.c     |  3 ++-
 fs/overlayfs/overlayfs.h |  5 +++-
 fs/overlayfs/util.c      | 21 ++++++++++++++--
 4 files changed, 88 insertions(+), 6 deletions(-)

fs/overlayfs/copy_up.c +38 -0

index ebd404629c6d..682852a78163 100644
@@ -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,9 +449,31 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
        goto out;
 }
 
+static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
+{
+       struct path upperpath;
+       int err;
+
+       ovl_path_upper(c->dentry, &upperpath);
+       BUG_ON(upperpath.dentry == NULL);
+
+       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;
+
+       smp_wmb();
+       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;
+       bool set_size = false;
 
        err = ovl_copy_xattr(c->lowerpath.dentry, temp);
        if (err)

@@ -476,8 +508,29 @@ 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;
+

fs/overlayfs/copy_up.c +8 -2

+               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
+       }
 out:
        dput(temp);
        return err;

@@ -639,7 +696,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)

@@ -649,6 +706,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_data_inode(&ctx);
                ovl_copy_up_end(dentry);
        }
        do_delayed_call(&done);

@@ -679,8 +738,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 */

fs/overlayfs/inode.c +2 -1

index a619addecafc..e5825b8948e0 100644
@@ -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))

fs/overlayfs/overlayfs.h +4 -1

index c706a6f99928..773f5ad75729 100644
@@ -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,

fs/overlayfs/util.c +19 -3

index a4ce1c9944f0..91c542d1a39d 100644
@@ -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) {
+       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

fs/overlayfs/copy_up.c static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) · line 568

Quoted 6 lines

fs/overlayfs/copy_up.c +2 -0

@@ -511,6 +564,10 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
                goto out_cleanup;
 
        ovl_inode_update(d_inode(c->dentry), newdentry);
+       if (c->metadata_only) {
+               smp_wmb();

This wmb does not address the only problem imo.
The problem is you must not allow inode to appear to have An upper dentry before you made sure metacopy flag is visible. So first you need to set metacopy flag before updating upper. Then you also need to test them is correct order with rmb.

On Tue, Oct 10, 2017 at 08:03:05PM +0300, Amir Goldstein wrote:

[..]

Quoted 12 lines

fs/overlayfs/copy_up.c static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) · line 568

Quoted 6 lines

fs/overlayfs/copy_up.c +2 -0

@@ -511,6 +564,10 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
                goto out_cleanup;
 
        ovl_inode_update(d_inode(c->dentry), newdentry);
+       if (c->metadata_only) {
+               smp_wmb();

This wmb does not address the only problem imo.
The problem is you must not allow inode to appear to have An upper dentry before you made sure metacopy flag is visible. So first you need to set metacopy flag before updating upper. Then you also need to test them is correct order with rmb.

Ok, so conceptually it makse sense to me that OVL_METACOPY flag should be set before upper dentry is made visible.

So ovl_set_flag(OVL_METACOPY) should move before ovl_inode_update(). ovl_inode_update() already has a smp_wmb(). So write side is probably fine.

But read side, ovl_upperdentry_dereference() only has data dependency barrier and not smp_rmb(). (lockless_dereference(oi->__upperdentry)). I am not sure if that's sufficient for ovl_inode->flags read or not. May be we need to replace data dependency barrier with smp_rmb() in in ovl_upperdentry_dereference()?

Even if we do this, I think we need barriers around setting/resetting of OVL_METACOPY for the reset case. I will explain this in other patch.

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy 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 metacopy bit.

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

---
 fs/overlayfs/copy_up.c   |  9 +++------
 fs/overlayfs/overlayfs.h |  3 +++
 fs/overlayfs/util.c      | 23 ++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +3 -6

index 682852a78163..b10269b80803 100644
@@ -465,8 +465,7 @@ static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
        if (err)
                return err;
 
-       smp_wmb();
-       ovl_clear_flag(OVL_METACOPY, d_inode(c->dentry));
+       ovl_clear_metacopy_flag(d_inode(c->dentry));
        return err;
 }
 

@@ -564,10 +563,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
                goto out_cleanup;
 
        ovl_inode_update(d_inode(c->dentry), newdentry);
-       if (c->metadata_only) {
-               smp_wmb();
-               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
-       }
+       if (c->metadata_only)
+               ovl_set_metacopy_flag(d_inode(c->dentry));
 out:
        dput(temp);
        return err;

fs/overlayfs/overlayfs.h +3 -0

index 773f5ad75729..df3f513de3cc 100644
@@ -234,6 +234,9 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
 void ovl_set_flag(unsigned long flag, struct inode *inode);
 void ovl_clear_flag(unsigned long flag, struct inode *inode);
 bool ovl_test_flag(unsigned long flag, struct inode *inode);
+void ovl_set_metacopy_flag(struct inode *inode);
+void ovl_clear_metacopy_flag(struct inode *inode);
+bool ovl_test_metacopy_flag(struct inode *inode);
 bool ovl_inuse_trylock(struct dentry *dentry);
 void ovl_inuse_unlock(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry, bool *locked);

fs/overlayfs/util.c +22 -2

index 91c542d1a39d..000f78b92a72 100644
@@ -227,6 +227,27 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
        oe->has_upper = true;
 }
 
+bool ovl_test_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_wmb() in ovl_set_metacopy_flag() */
+       smp_rmb();
+       return ovl_test_flag(OVL_METACOPY, inode);
+}
+
+void ovl_set_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_rmb() in ovl_test_metacopy_flag() */
+       smp_wmb();
+       ovl_set_flag(OVL_METACOPY, inode);
+}
+
+void ovl_clear_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_rmb() in ovl_test_metacopy_flag() */
+       smp_wmb();
+       ovl_clear_flag(OVL_METACOPY, inode);
+}
+
 bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
        if (!S_ISREG(d_inode(dentry)->i_mode))
                return false;

@@ -237,7 +258,7 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
        if (!(OPEN_FMODE(flags) & FMODE_WRITE))
                return false;
 
-       if (!ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
+       if (!ovl_test_metacopy_flag(d_inode(dentry)))
                return false;
 
        return true;
--

2.13.5

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Quoted 94 lines

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

---
 fs/overlayfs/copy_up.c   |  9 +++------
 fs/overlayfs/overlayfs.h |  3 +++
 fs/overlayfs/util.c      | 23 ++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +3 -6

index 682852a78163..b10269b80803 100644
@@ -465,8 +465,7 @@ static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
        if (err)
                return err;
 
-       smp_wmb();
-       ovl_clear_flag(OVL_METACOPY, d_inode(c->dentry));
+       ovl_clear_metacopy_flag(d_inode(c->dentry));
        return err;
 }
 

@@ -564,10 +563,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
                goto out_cleanup;
 
        ovl_inode_update(d_inode(c->dentry), newdentry);
-       if (c->metadata_only) {
-               smp_wmb();
-               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
-       }
+       if (c->metadata_only)
+               ovl_set_metacopy_flag(d_inode(c->dentry));
 out:
        dput(temp);
        return err;

fs/overlayfs/overlayfs.h +3 -0

index 773f5ad75729..df3f513de3cc 100644
@@ -234,6 +234,9 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
 void ovl_set_flag(unsigned long flag, struct inode *inode);
 void ovl_clear_flag(unsigned long flag, struct inode *inode);
 bool ovl_test_flag(unsigned long flag, struct inode *inode);
+void ovl_set_metacopy_flag(struct inode *inode);
+void ovl_clear_metacopy_flag(struct inode *inode);
+bool ovl_test_metacopy_flag(struct inode *inode);
 bool ovl_inuse_trylock(struct dentry *dentry);
 void ovl_inuse_unlock(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry, bool *locked);

fs/overlayfs/util.c +22 -2

index 91c542d1a39d..000f78b92a72 100644
@@ -227,6 +227,27 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
        oe->has_upper = true;
 }
 
+bool ovl_test_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_wmb() in ovl_set_metacopy_flag() */
+       smp_rmb();
+       return ovl_test_flag(OVL_METACOPY, inode);
+}
+
+void ovl_set_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_rmb() in ovl_test_metacopy_flag() */
+       smp_wmb();
+       ovl_set_flag(OVL_METACOPY, inode);
+}
+
+void ovl_clear_metacopy_flag(struct inode *inode)
+{
+       /* Pairs with smp_rmb() in ovl_test_metacopy_flag() */
+       smp_wmb();
+       ovl_clear_flag(OVL_METACOPY, inode);
+}
+
 bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
        if (!S_ISREG(d_inode(dentry)->i_mode))
                return false;

@@ -237,7 +258,7 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
        if (!(OPEN_FMODE(flags) & FMODE_WRITE))
                return false;
 
-       if (!ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
+       if (!ovl_test_metacopy_flag(d_inode(dentry)))
                return false;
 
        return true;
--

2.13.5

Quoted 22 lines · Amir Goldstein

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 7 lines

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make sure by the time upperdentry is visible, OVL_METACOPY is visible too.

Quoted 7 lines

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Say a file has been metadata copied up only. So upper is visible and OVL_METACOPY flag is set.

Now, what happens if two paths try to data copy up the file. Say first path gets the oi->lock and starts doing data copy. Second path enters ovl_copy_up_flags() and checks if data copy up is required or not. If OVL_METACOPY is still set, then second process will block on oi->lock and once first process exits will check OVL_METACOPY again under lock and bail out. So that is not a problem.

What about the other case. That is OVL_METACOPY gets cleared before data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() around resetting of OVL_METACOPY, what will make sure that ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is complete. Of course we will not like to return to caller of ovl_copy_up_flags() while data copy up is still going on on a different cpu.

Quoted 52 lines · Vivek Goyal

Quoted 22 lines · Amir Goldstein

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 7 lines

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make sure by the time upperdentry is visible, OVL_METACOPY is visible too.

Quoted 7 lines

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Say a file has been metadata copied up only. So upper is visible and OVL_METACOPY flag is set.

Now, what happens if two paths try to data copy up the file. Say first path gets the oi->lock and starts doing data copy. Second path enters ovl_copy_up_flags() and checks if data copy up is required or not. If OVL_METACOPY is still set, then second process will block on oi->lock and once first process exits will check OVL_METACOPY again under lock and bail out. So that is not a problem.

Quoted 8 lines

What about the other case. That is OVL_METACOPY gets cleared before data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() around resetting of OVL_METACOPY, what will make sure that ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is complete. Of course we will not like to return to caller of ovl_copy_up_flags() while data copy up is still going on on a different cpu.

vfs_removexattr() between data copy up and clearing OVL_METACOPY takes inode lock.
IIUC, any lock/unlock has a full memory barrier.

Quoted 62 lines · Amir Goldstein

Quoted 60 lines · Vivek Goyal

Quoted 22 lines · Amir Goldstein

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 7 lines

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make sure by the time upperdentry is visible, OVL_METACOPY is visible too.

Quoted 7 lines

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Say a file has been metadata copied up only. So upper is visible and OVL_METACOPY flag is set.

Now, what happens if two paths try to data copy up the file. Say first path gets the oi->lock and starts doing data copy. Second path enters ovl_copy_up_flags() and checks if data copy up is required or not. If OVL_METACOPY is still set, then second process will block on oi->lock and once first process exits will check OVL_METACOPY again under lock and bail out. So that is not a problem.

What about the other case. That is OVL_METACOPY gets cleared before data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() around resetting of OVL_METACOPY, what will make sure that ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is complete. Of course we will not like to return to caller of ovl_copy_up_flags() while data copy up is still going on on a different cpu.

Quoted 3 lines

vfs_removexattr() between data copy up and clearing OVL_METACOPY takes inode lock.
IIUC, any lock/unlock has a full memory barrier.

When I read memory-barrier.txt, it seems to suggest that RELEASE can let instructions outside critical region sneak into critical region. If that's the case, actually clearing of metaflag can happen before xattr actually got removed. Though I can't think what will go wrong in that case.

Also will all this work without other cpu having any matching barrier? Again, I am not barrier expert and I am trying to understand these and

one of the messages seems to be that most of the time they have to be paired. So in this case, we are taking inode lock/unlock on one cpu but cpu which is checking value of OVL_METACOPY, really does not have any matching acquire/release or some other kind of barrier.

Quoted 63 lines · Vivek Goyal

Quoted 62 lines · Amir Goldstein

Quoted 60 lines · Vivek Goyal

Quoted 22 lines · Amir Goldstein

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 7 lines

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make sure by the time upperdentry is visible, OVL_METACOPY is visible too.

Quoted 7 lines

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Say a file has been metadata copied up only. So upper is visible and OVL_METACOPY flag is set.

Now, what happens if two paths try to data copy up the file. Say first path gets the oi->lock and starts doing data copy. Second path enters ovl_copy_up_flags() and checks if data copy up is required or not. If OVL_METACOPY is still set, then second process will block on oi->lock and once first process exits will check OVL_METACOPY again under lock and bail out. So that is not a problem.

What about the other case. That is OVL_METACOPY gets cleared before data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() around resetting of OVL_METACOPY, what will make sure that ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is complete. Of course we will not like to return to caller of ovl_copy_up_flags() while data copy up is still going on on a different cpu.

Quoted 8 lines

Quoted 3 lines

vfs_removexattr() between data copy up and clearing OVL_METACOPY takes inode lock.
IIUC, any lock/unlock has a full memory barrier.

When I read memory-barrier.txt, it seems to suggest that RELEASE can let instructions outside critical region sneak into critical region. If that's the case, actually clearing of metaflag can happen before xattr actually got removed. Though I can't think what will go wrong in that case.

clear_bit takes a spinlock so I *think* clearing of flag cannot sneak before removing xattr.

Quoted 3 lines

Also will all this work without other cpu having any matching barrier? Again, I am not barrier expert and I am trying to understand these and

Me neither, trying to understand this along with you.

Quoted 5 lines

one of the messages seems to be that most of the time they have to be paired. So in this case, we are taking inode lock/unlock on one cpu but cpu which is checking value of OVL_METACOPY, really does not have any matching acquire/release or some other kind of barrier.

Other CPU needs rmb before checking flag to guarantee the order of setting METACOPY flag before setting upperdentry.

If other CPU sees positive upperdentry and the METACOPY flag cleared, then I *think* it should be safe to return the upper inode from d_real(), because RELEASE on inode lock should have the proper guaranties for consistency of inode data structures in memory.

Quoted 91 lines · Amir Goldstein

Quoted 63 lines · Vivek Goyal

Quoted 62 lines · Amir Goldstein

Quoted 60 lines · Vivek Goyal

Quoted 22 lines · Amir Goldstein

Quoted 21 lines · Vivek Goyal

We can access and check metacopy flag outside ovl_inode->lock. IOW, say a file was meta copied up in the past and it has metacopy flag set. Now a data copy up operation in progress. Say another thread reads state of this flag and if flag clearing is visible before file has been fully copied up, that can cause problems.

CPU1 CPU2
ovl_copy_up_flags() acquire(oi->lock)
ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode()
ovl_test_metacopy_flag() ovl_copy_up_data()

                                            ovl_clear_metacopy_flag()
                                        release(oi->lock)

Say CPU2 is copying up data and in the end clears metacopy flag. But if CPU1 perceives clearing of metacopy before actual data copy up operation being finished, that can become a problem. We want this clearing of flag to be visible only if all previous write operations have finished.

Hence this patch introduces smp_wmb() on metacopy flag set/clear operation and smp_rmb() on metacopy flag test operation.

Quoted 7 lines

Quoted 5 lines

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

Please revisit your changes after addressing my comment on patch 5.

Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make sure by the time upperdentry is visible, OVL_METACOPY is visible too.

Quoted 7 lines

IIUC the flow you describe above should be addressed by testing metacopy flag again under ovl inode lock.

The only subtle part imo is making metacopy flag visible Before making upper dentry visible.
Clearing metacopy flag should not be a problem imo, As it comes after fsync, but didn't look closely.

Say a file has been metadata copied up only. So upper is visible and OVL_METACOPY flag is set.

Now, what happens if two paths try to data copy up the file. Say first path gets the oi->lock and starts doing data copy. Second path enters ovl_copy_up_flags() and checks if data copy up is required or not. If OVL_METACOPY is still set, then second process will block on oi->lock and once first process exits will check OVL_METACOPY again under lock and bail out. So that is not a problem.

What about the other case. That is OVL_METACOPY gets cleared before data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() around resetting of OVL_METACOPY, what will make sure that ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is complete. Of course we will not like to return to caller of ovl_copy_up_flags() while data copy up is still going on on a different cpu.

Quoted 8 lines

Quoted 3 lines

vfs_removexattr() between data copy up and clearing OVL_METACOPY takes inode lock.
IIUC, any lock/unlock has a full memory barrier.

When I read memory-barrier.txt, it seems to suggest that RELEASE can let instructions outside critical region sneak into critical region. If that's the case, actually clearing of metaflag can happen before xattr actually got removed. Though I can't think what will go wrong in that case.

clear_bit takes a spinlock so I *think* clearing of flag cannot sneak before removing xattr.

Quoted 3 lines

Also will all this work without other cpu having any matching barrier? Again, I am not barrier expert and I am trying to understand these and

Me neither, trying to understand this along with you.

Quoted 5 lines

one of the messages seems to be that most of the time they have to be paired. So in this case, we are taking inode lock/unlock on one cpu but cpu which is checking value of OVL_METACOPY, really does not have any matching acquire/release or some other kind of barrier.

Other CPU needs rmb before checking flag to guarantee the order of setting METACOPY flag before setting upperdentry.

Quoted 4 lines

If other CPU sees positive upperdentry and the METACOPY flag cleared, then I *think* it should be safe to return the upper inode from d_real(), because RELEASE on inode lock should have the proper guaranties for consistency of inode data structures in memory.

Nah, smp_wmb before clearing METACOPY is correct, just like smp_wmb in ovl_inode_update(). probably should copy the same comment.

But that was already there before this wrappers patch, so I don't see the need for this patch.

Quoted 4 lines

When I read memory-barrier.txt, it seems to suggest that RELEASE can let instructions outside critical region sneak into critical region. If that's the case, actually clearing of metaflag can happen before xattr actually got removed. Though I can't think what will go wrong in that case.

clear_bit takes a spinlock so I *think* clearing of flag cannot sneak before removing xattr.

On Sat, Oct 14, 2017 at 09:05:17AM +0300, Amir Goldstein wrote:

[..]

Quoted 5 lines

Quoted 4 lines

When I read memory-barrier.txt, it seems to suggest that RELEASE can let instructions outside critical region sneak into critical region. If that's the case, actually clearing of metaflag can happen before xattr actually got removed. Though I can't think what will go wrong in that case.

Quoted 2 lines

clear_bit takes a spinlock so I *think* clearing of flag cannot sneak before removing xattr.

Which spinlock does clear_bit take? Are you referring to ovl_inode->lock mutex?