[PATCH 00/10] overlayfs assorted fixes for v4.13

[PATCH 00/10] overlayfs assorted fixes for v4.13

[Amir Goldstein]

Miklos,

This is a collection of fixes on top of current overlayfs-next.
None of the fixes are urgent for -rc1, so up to you.
Also wanted to remind you that the top commit of overlayfs-next
is missing your s-o-b.

Patch 1 is a fix for an oversight from v4.12.
Patch 2 is a fix for error handing from v4.7.
Patches 3-7 are various fixes to index=on,ro mount.
Patches 8-10 are forward compat fixes for when index dir
will be used for NFS export.

Specifically, patches 9-10 introduce a behavior change in the
case of changes to lower layer (rename or delete of lower dir).
The new behavior is to verify origin fh and try to decode it
if verification fails (overlayfs.txt updated).

This change in needed for snapshots and I argue in the commit
message of patch 10 why it is needed for indexing of directories.
I admit that the argument is not rigorous, but since the behavior of
changes to lower layer is documented as "undefined", I recon you
don't really mind if we change it for index=on.

The rational of making this change in v4.13 as opposed to when
directory indexing is implemented, is that index=on already introduces
a behavior change related to copying layers, so IMO it is better to
introduce all those behavior changes together on index=on.

Thanks,
Amir.

Amir Goldstein (10):
  ovl: mark parent impure on ovl_link()
  ovl: fix random return value on mount
  ovl: fix origin verification of index dir
  ovl: remove unneeded check for IS_ERR()
  ovl: suppress file handle support warnings on read-only mount
  ovl: force read-only mount with no index dir
  ovl: mount overlay read-only on failure to verify index dir
  ovl: do not cleanup directory and whiteout index entries
  ovl: verify origin of merge dir lower
  ovl: follow decoded origin file handle of merge dir

 Documentation/filesystems/overlayfs.txt | 20 +++++++++++
 fs/dcache.c                             |  1 +
 fs/overlayfs/copy_up.c                  |  4 +--
 fs/overlayfs/dir.c                      | 20 ++++++++---
 fs/overlayfs/namei.c                    | 63 +++++++++++++++++++++++++++++----
 fs/overlayfs/overlayfs.h                |  5 +--
 fs/overlayfs/super.c                    | 35 +++++++++---------
 fs/overlayfs/util.c                     |  6 ++--
 8 files changed, 118 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH 01/10] ovl: mark parent impure on ovl_link()

[Amir Goldstein]

When linking a file with copy up origin into a new parent, mark the
new parent dir "impure".

Fixes: ee1d6d37b6b8 ("ovl: mark upper dir with type origin entries "impure"")
Cc: <stable@vger.kernel.org> # v4.12
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  4 ++--
 fs/overlayfs/dir.c       | 20 ++++++++++++++++----
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/util.c      |  6 +++---
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index acb6f97deb97..aeb22def9f80 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -338,7 +338,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 	struct inode *udir = d_inode(upperdir);
 
 	/* Mark parent "impure" because it may now contain non-pure upper */
-	err = ovl_set_impure(c->parent, upperdir);
+	err = ovl_set_impure(c->parent);
 	if (err)
 		return err;
 
@@ -551,7 +551,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		 * Mark parent "impure" because it may now contain non-pure
 		 * upper
 		 */
-		err = ovl_set_impure(c->parent, c->destdir);
+		err = ovl_set_impure(c->parent);
 		if (err)
 			return err;
 	}
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 641d9ee97f91..5df99952feba 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -92,7 +92,8 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		return -ESTALE;
 
 	if (hardlink) {
-		err = ovl_do_link(hardlink, dir, newdentry, debug);
+		err = ovl_do_link(ovl_dentry_upper(hardlink), dir, newdentry,
+				  debug);
 	} else {
 		switch (attr->mode & S_IFMT) {
 		case S_IFREG:
@@ -492,6 +493,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		return err;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
+
+	/*
+	 * When linking a file with copy up origin into a new parent, mark the
+	 * new parent dir "impure".
+	 */
+	if (hardlink && ovl_type_origin(hardlink)) {
+		err = ovl_set_impure(dentry->d_parent);
+		if (err)
+			goto out_revert_creds;
+	}
+
 	err = -ENOMEM;
 	override_cred = prepare_creds();
 	if (override_cred) {
@@ -609,7 +621,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 	inode = d_inode(old);
 	ihold(inode);
 
-	err = ovl_create_or_link(new, inode, NULL, ovl_dentry_upper(old));
+	err = ovl_create_or_link(new, inode, NULL, old);
 	if (err)
 		iput(inode);
 
@@ -987,12 +999,12 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 		 * lookup the origin inodes of the entries to fill d_ino.
 		 */
 		if (ovl_type_origin(old)) {
-			err = ovl_set_impure(new->d_parent, new_upperdir);
+			err = ovl_set_impure(new->d_parent);
 			if (err)
 				goto out_revert_creds;
 		}
 		if (!overwrite && ovl_type_origin(new)) {
-			err = ovl_set_impure(old->d_parent, old_upperdir);
+			err = ovl_set_impure(old->d_parent);
 			if (err)
 				goto out_revert_creds;
 		}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 60d26605e039..7aa1c48390a5 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -225,7 +225,7 @@ bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 		       const char *name, const void *value, size_t size,
 		       int xerr);
-int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
+int ovl_set_impure(struct dentry *dentry);
 void ovl_set_flag(unsigned long flag, struct inode *inode);
 bool ovl_test_flag(unsigned long flag, struct inode *inode);
 bool ovl_inuse_trylock(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index c492ba75c659..82c3dad6ddf4 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -353,7 +353,7 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 	return err;
 }
 
-int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
+int ovl_set_impure(struct dentry *dentry)
 {
 	int err;
 
@@ -364,8 +364,8 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 	 * Do not fail when upper doesn't support xattrs.
 	 * Upper inodes won't have origin nor redirect xattr anyway.
 	 */
-	err = ovl_check_setxattr(dentry, upperdentry, OVL_XATTR_IMPURE,
-				 "y", 1, 0);
+	err = ovl_check_setxattr(dentry, ovl_dentry_upper(dentry),
+				 OVL_XATTR_IMPURE, "y", 1, 0);
 	if (!err)
 		ovl_set_flag(OVL_IMPURE, d_inode(dentry));
 
-- 
2.7.4

[PATCH 02/10] ovl: fix random return value on mount

[Amir Goldstein]

On failure to prepare_creds(), mount fails with a random
return value, as err was last set to an integer cast of
a valid lower mnt pointer or set to 0 if inodes index feature
is enabled.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 3fe6e52f0626 ("ovl: override creds with the ones from ...")
Cc: <stable@vger.kernel.org> # v4.7
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 44dc2d6ffe0f..1cf5d3538309 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1090,6 +1090,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else
 		sb->s_d_op = &ovl_dentry_operations;
 
+	err = -ENOMEM;
 	ufs->creator_cred = cred = prepare_creds();
 	if (!cred)
 		goto out_put_indexdir;
-- 
2.7.4

[PATCH 03/10] ovl: fix origin verification of index dir

[Amir Goldstein]

Commit 54fb347e836f ("ovl: verify index dir matches upper dir")
introduced a new ovl_fh flag OVL_FH_FLAG_PATH_UPPER to indicate
an upper file handle, but forgot to add the flag to the mask of
valid flags, so index dir origin verification always discards
existing origin and stores a new one.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 7aa1c48390a5..8bfb35cb77c3 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -47,7 +47,8 @@ enum ovl_flag {
 /* Is the real inode encoded in fid an upper inode? */
 #define OVL_FH_FLAG_PATH_UPPER	(1 << 2)
 
-#define OVL_FH_FLAG_ALL (OVL_FH_FLAG_BIG_ENDIAN | OVL_FH_FLAG_ANY_ENDIAN)
+#define OVL_FH_FLAG_ALL (OVL_FH_FLAG_BIG_ENDIAN | OVL_FH_FLAG_ANY_ENDIAN | \
+			 OVL_FH_FLAG_PATH_UPPER)
 
 #if defined(__LITTLE_ENDIAN)
 #define OVL_FH_FLAG_CPU_ENDIAN 0
-- 
2.7.4

[PATCH 04/10] ovl: remove unneeded check for IS_ERR()

[Amir Goldstein]

ovl_workdir_create() returns a valid index dentry or NULL.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 1cf5d3538309..c88493b01d8d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1058,10 +1058,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 		ufs->indexdir = ovl_workdir_create(sb, ufs, workpath.dentry,
 						   OVL_INDEXDIR_NAME, true);
-		err = PTR_ERR(ufs->indexdir);
-		if (IS_ERR(ufs->indexdir))
-			goto out_put_lower_mnt;
-
 		if (ufs->indexdir) {
 			/* Verify upper root is index dir origin */
 			err = ovl_verify_origin(ufs->indexdir, ufs->upper_mnt,
-- 
2.7.4

[PATCH 05/10] ovl: suppress file handle support warnings on read-only mount

[Amir Goldstein]

An overlay mount with no upper layer (a.k.a ovl_force_readonly)
does not need an index, because there is no copy up, so there is
no reason to spew warnings about lower layer file handle support
for index=on.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c88493b01d8d..6381e71b0d5d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -922,6 +922,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	} else if (!ufs->config.upperdir && stacklen == 1) {
 		pr_err("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n");
 		goto out_free_lowertmp;
+	} else if (!ufs->config.upperdir || !ufs->config.workdir) {
+		/* Read-only mount - skip file handle support checks */
+		ufs->config.index = false;
 	}
 
 	err = -ENOMEM;
-- 
2.7.4

[PATCH 06/10] ovl: force read-only mount with no index dir

[Amir Goldstein]

When workdir creation fails, overlay is mounted read-only and
remount,rw is not allowed. When index dir creation fails, overlay
is mounted readonly, but we also need to enforce no remount,rw in
that case.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 6381e71b0d5d..215b6d23d944 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -274,7 +274,8 @@ static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
 /* Will this overlay be forced to mount/remount ro? */
 static bool ovl_force_readonly(struct ovl_fs *ufs)
 {
-	return (!ufs->upper_mnt || !ufs->workdir);
+	return (!ufs->upper_mnt || !ufs->workdir ||
+		(ufs->config.index && !ufs->indexdir));
 }
 
 /**
@@ -298,7 +299,7 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ufs->config.redirect_dir != ovl_redirect_dir_def)
 		seq_printf(m, ",redirect_dir=%s",
 			   ufs->config.redirect_dir ? "on" : "off");
-	if (ufs->config.index != ovl_index_def)
+	if (!ovl_force_readonly(ufs) && ufs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s",
 			   ufs->config.index ? "on" : "off");
 	return 0;
@@ -1050,7 +1051,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
 		ufs->same_sb = NULL;
 
-	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
+	if (ufs->upper_mnt && ufs->workdir && ufs->config.index) {
 		/* Verify lower root is upper root origin */
 		err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
 					stack[0].dentry, false, true);
@@ -1080,10 +1081,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			goto out_put_indexdir;
 	}
 
-	/* Show index=off/on in /proc/mounts for any of the reasons above */
-	if (!ufs->indexdir)
-		ufs->config.index = false;
-
 	if (remote)
 		sb->s_d_op = &ovl_reval_dentry_operations;
 	else
-- 
2.7.4

[PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Amir Goldstein]

Just like on failure to create work dir or index dir, when
index dir verification or cleanup fails, mount the overlay
read-only with a warning instead of failing the mount.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 215b6d23d944..fbb317ad55f6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1066,19 +1066,21 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			/* Verify upper root is index dir origin */
 			err = ovl_verify_origin(ufs->indexdir, ufs->upper_mnt,
 						upperpath.dentry, true, true);
-			if (err)
-				pr_err("overlayfs: failed to verify index dir origin\n");
-
 			/* Cleanup bad/stale/orphan index entries */
 			if (!err)
 				err = ovl_indexdir_cleanup(ufs->indexdir,
 							   ufs->upper_mnt,
 							   stack, numlower);
+			if (err)
+				pr_warn("overlayfs: failed to verify index dir (err=%i); mounting read-only\n",
+					err);
+		}
+		if (err || !ufs->indexdir) {
+			pr_warn("overlayfs: delete index dir or mount with '-o index=off' to disable inodes index.\n");
+			sb->s_flags |= MS_RDONLY;
+			dput(ufs->indexdir);
+			ufs->indexdir = NULL;
 		}
-		if (err || !ufs->indexdir)
-			pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
-		if (err)
-			goto out_put_indexdir;
 	}
 
 	if (remote)
-- 
2.7.4

[PATCH 08/10] ovl: do not cleanup directory and whiteout index entries

[Amir Goldstein]

Directory index entries are going to be used for looking up
redirected upper dirs by lower dir fh when decoding an overlay
file handle of a merge dir.

Whiteout index entries are going to be used as an indication that
an exported overlay file handle should be treated as stale (i.e.
after unlink of the overlay inode).

We don't know the verification rules for directory and whiteout
index entries, because they have not been implemented yet, so skip
verification of those entries and let newer kernels deal with them.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 9bc0e580a5b3..86f09230a3db 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -397,9 +397,18 @@ int ovl_verify_index(struct dentry *index, struct path *lowerstack,
 	if (!d_inode(index))
 		return 0;
 
-	err = -EISDIR;
-	if (d_is_dir(index))
-		goto fail;
+	/*
+	 * Directory index entries are going to be used for looking up
+	 * redirected upper dirs by lower dir fh when decoding an overlay
+	 * file handle of a merge dir. Whiteout index entries are going to be
+	 * used as an indication that an exported overlay file handle should
+	 * be treated as stale (i.e. after unlink of the overlay inode).
+	 * We don't know the verification rules for directory and whiteout
+	 * index entries, because they have not been implemented yet, so skip
+	 * verification of those entries and let newer kernels deal with them.
+	 */
+	if (d_is_dir(index) || ovl_is_whiteout(index))
+		return 0;
 
 	err = -EINVAL;
 	if (index->d_name.len < sizeof(struct ovl_fh)*2)
-- 
2.7.4

[PATCH 09/10] ovl: verify origin of merge dir lower

[Amir Goldstein]

When inodes index feature is enabled, verify that lower merge dir found
by name matches the origin file handle stored in xattr on upper dir.
If origin xattr does not exist, update it with the lower found by name.

If lower dir does not match the origin fh stored in upper dir, do not
merge the lower dir and treat upper dir as pure upper.  This behavior
is not friendy to the use case of copied overlay layers, where origin
file handles are broken, but trying to mount an overlay with inodes
index enabled is going to fail anyway for copied layers.

Setting the origin xattr on the upper merge dir also serves as an
indication that this dir may contain whiteouts, which is going to be
used to prevent exposing whiteouts to readdir() in case lower dir was
removed while overlay was offline.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
 fs/overlayfs/namei.c                    | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index 36f528a7fdd6..9b9e8efc3977 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -262,6 +262,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
 the behavior of the overlay is undefined, though it will not result in
 a crash or deadlock.
 
+When the underlying filesystems supports NFS export, overlay mount can be
+made more resilient to offline and online changes of the underlying lower
+layer by enabling the "inodes index" feature.
+
+On every copy_up, an NFS file handle of the lower inode, along with the
+UUID of the lower filesystem, are encoded and stored in an extended
+attribute "trusted.overlay.origin" on the upper inode.
+
+With the "inodes index" feature, a lookup of a merged directory, that
+found a lower directory at the lookup path or at the path pointed to by
+the "trusted.overlay.redirect" extended attribute, will verify that the
+found lower directory file handle and lower filesystem UUID match the
+origin file handle that was stored at copy_up time.  If a found lower
+directory does not match the stored origin, that directory will be not be
+merged with the upper directory.
+
 Testsuite
 ---------
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 86f09230a3db..ec81d27b12be 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -648,6 +648,16 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
+		/* Verify that uppermost lower matches the copy up origin fh */
+		if (this && upperdentry && !ctr && ovl_indexdir(dentry->d_sb)) {
+			err = ovl_verify_origin(upperdentry, lowerpath.mnt,
+						this, false, true);
+			if (err) {
+				dput(this);
+				break;
+			}
+		}
+
 		stack[ctr].dentry = this;
 		stack[ctr].mnt = lowerpath.mnt;
 		ctr++;
-- 
2.7.4

[PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

When inodes index feature is enabled, if lower dir does not match the
origin fh stored in upper dir, or if no lower dir was found by name,
try to decode the stored origin fh and use the result as the merge dir
lower dentry.

This change is needed for indexing of merge directories. A merge
directory is indexed by the file handle of the uppermost lower dir and
the index will have a reference to the upper dir by file handle.
Following down from upper by origin file handle ensures the integrity
of the index for non-stale origin entries. index entries with stale origin
are going to be cleaned up on mount anyway.

When there is more than a single lower layer, a lower merge directory
followed from upper dir by origin fh is not followed down again to lower
layers by name.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.txt |  6 +++++-
 fs/dcache.c                             |  1 +
 fs/overlayfs/namei.c                    | 38 +++++++++++++++++++++++++++++----
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index 9b9e8efc3977..ceeb77d86671 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -276,7 +276,11 @@ the "trusted.overlay.redirect" extended attribute, will verify that the
 found lower directory file handle and lower filesystem UUID match the
 origin file handle that was stored at copy_up time.  If a found lower
 directory does not match the stored origin, that directory will be not be
-merged with the upper directory.
+merged with the upper directory.  If the stored origin file handle can be
+decoded to an existing lower directory, the decoded directory will be
+merged with the upper directory instead.  The "inodes index" feature,
+therefore, makes an overlay mount cope better with the situations of
+lower directory rename and delete.
 
 Testsuite
 ---------
diff --git a/fs/dcache.c b/fs/dcache.c
index a9f995f6859e..c6d3d76f4bee 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3497,6 +3497,7 @@ bool is_subdir(struct dentry *new_dentry, struct dentry *old_dentry)
 
 	return result;
 }
+EXPORT_SYMBOL(is_subdir);
 
 static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry)
 {
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ec81d27b12be..42f839d26e94 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -83,9 +83,21 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 	goto err_free;
 }
 
-static int ovl_acceptable(void *ctx, struct dentry *dentry)
+static int ovl_acceptable(void *mnt, struct dentry *dentry)
 {
-	return 1;
+	/*
+	 * A non-dir origin may be disconnected, which is fine, because
+	 * we only need it for its unique inode number.
+	 */
+	if (!d_is_dir(dentry))
+		return 1;
+
+	/* Don't want to follow a deleted empty lower directory */
+	if (d_unhashed(dentry))
+		return 0;
+
+	/* Check if directory belongs to the layer mounted at mnt */
+	return is_subdir(dentry, ((struct vfsmount *)mnt)->mnt_root);
 }
 
 static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
@@ -160,7 +172,7 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
 	bytes = (fh->len - offsetof(struct ovl_fh, fid));
 	origin = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
 				    bytes >> 2, (int)fh->type,
-				    ovl_acceptable, NULL);
+				    ovl_acceptable, mnt);
 	if (IS_ERR(origin)) {
 		/* Treat stale file handle as "origin unknown" */
 		if (origin == ERR_PTR(-ESTALE))
@@ -293,7 +305,6 @@ static int ovl_check_origin(struct dentry *upperdentry,
 	struct dentry *origin = NULL;
 	int i;
 
-
 	for (i = 0; i < numlower; i++) {
 		mnt = lowerstack[i].mnt;
 		origin = ovl_get_origin(upperdentry, mnt);
@@ -677,6 +688,25 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
+	/*
+	 * If lower not found by name or couldn't verify origin fh, try to
+	 * follow the decoded origin fh in upper to the first lower dir.
+	 */
+	if (!d.stop && poe->numlower && upperdentry && !ctr &&
+	    ovl_indexdir(dentry->d_sb)) {
+		err = ovl_check_origin(upperdentry, roe->lowerstack,
+				       roe->numlower, &stack, &ctr);
+		if (err)
+			goto out_put;
+		/*
+		 * XXX: We do not continue layers lookup from decoded origin for
+		 * more than a single lower layer. This would require setting
+		 * d.redirect to decoded origin path and jump back to the
+		 * lowerstack layers lookup loop with 'i' set to the root layer
+		 * number where we found the decoded origin.
+		 */
+	}
+
 	/* Lookup index by lower inode and verify it matches upper inode */
 	if (ctr && !d.is_dir && ovl_indexdir(dentry->d_sb)) {
 		struct dentry *origin = stack[0].dentry;
-- 
2.7.4

Re: [PATCH 00/10] overlayfs assorted fixes for v4.13

[Amir Goldstein]

On Tue, Jul 11, 2017 at 3:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This is a collection of fixes on top of current overlayfs-next.
> None of the fixes are urgent for -rc1, so up to you.
> Also wanted to remind you that the top commit of overlayfs-next
> is missing your s-o-b.
>
> Patch 1 is a fix for an oversight from v4.12.
> Patch 2 is a fix for error handing from v4.7.
> Patches 3-7 are various fixes to index=on,ro mount.
> Patches 8-10 are forward compat fixes for when index dir
> will be used for NFS export.
>
> Specifically, patches 9-10 introduce a behavior change in the
> case of changes to lower layer (rename or delete of lower dir).
> The new behavior is to verify origin fh and try to decode it
> if verification fails (overlayfs.txt updated).
>
> This change in needed for snapshots and I argue in the commit
> message of patch 10 why it is needed for indexing of directories.
> I admit that the argument is not rigorous, but since the behavior of
> changes to lower layer is documented as "undefined", I recon you
> don't really mind if we change it for index=on.
>
> The rational of making this change in v4.13 as opposed to when
> directory indexing is implemented, is that index=on already introduces
> a behavior change related to copying layers, so IMO it is better to
> introduce all those behavior changes together on index=on.
>
> Thanks,
> Amir.
>

P.S.

A branch with these fixes is available at:
https://github.com/amir73il/linux/commits/for-miklos

And three new xfstests to test mount behavior are available at:
https://github.com/amir73il/xfstests/commits/overlayfs-devel

7918225 overlay: test mount error cases with index=on
a2f7df6 overlay: test mount error cases with exclusive directories
53f0c38 overlay: test cases that force read-only mount



> Amir Goldstein (10):
>   ovl: mark parent impure on ovl_link()
>   ovl: fix random return value on mount
>   ovl: fix origin verification of index dir
>   ovl: remove unneeded check for IS_ERR()
>   ovl: suppress file handle support warnings on read-only mount
>   ovl: force read-only mount with no index dir
>   ovl: mount overlay read-only on failure to verify index dir
>   ovl: do not cleanup directory and whiteout index entries
>   ovl: verify origin of merge dir lower
>   ovl: follow decoded origin file handle of merge dir
>
>  Documentation/filesystems/overlayfs.txt | 20 +++++++++++
>  fs/dcache.c                             |  1 +
>  fs/overlayfs/copy_up.c                  |  4 +--
>  fs/overlayfs/dir.c                      | 20 ++++++++---
>  fs/overlayfs/namei.c                    | 63 +++++++++++++++++++++++++++++----
>  fs/overlayfs/overlayfs.h                |  5 +--
>  fs/overlayfs/super.c                    | 35 +++++++++---------
>  fs/overlayfs/util.c                     |  6 ++--
>  8 files changed, 118 insertions(+), 36 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH 06/10] ovl: force read-only mount with no index dir

[Miklos Szeredi]

On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> When workdir creation fails, overlay is mounted read-only and
> remount,rw is not allowed. When index dir creation fails, overlay
> is mounted readonly, but we also need to enforce no remount,rw in
> that case.

I don't understand the logic for ovl_show_options().  It should show
the options supplied, right?  It doesn't currently do that for
"index=FOO" but this patch doesn't help that, afaics.

Thanks,
Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 6381e71b0d5d..215b6d23d944 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -274,7 +274,8 @@ static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
>  /* Will this overlay be forced to mount/remount ro? */
>  static bool ovl_force_readonly(struct ovl_fs *ufs)
>  {
> -       return (!ufs->upper_mnt || !ufs->workdir);
> +       return (!ufs->upper_mnt || !ufs->workdir ||
> +               (ufs->config.index && !ufs->indexdir));
>  }
>
>  /**
> @@ -298,7 +299,7 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>         if (ufs->config.redirect_dir != ovl_redirect_dir_def)
>                 seq_printf(m, ",redirect_dir=%s",
>                            ufs->config.redirect_dir ? "on" : "off");
> -       if (ufs->config.index != ovl_index_def)
> +       if (!ovl_force_readonly(ufs) && ufs->config.index != ovl_index_def)
>                 seq_printf(m, ",index=%s",
>                            ufs->config.index ? "on" : "off");
>         return 0;
> @@ -1050,7 +1051,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
>                 ufs->same_sb = NULL;
>
> -       if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
> +       if (ufs->upper_mnt && ufs->workdir && ufs->config.index) {
>                 /* Verify lower root is upper root origin */
>                 err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
>                                         stack[0].dentry, false, true);
> @@ -1080,10 +1081,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                         goto out_put_indexdir;
>         }
>
> -       /* Show index=off/on in /proc/mounts for any of the reasons above */
> -       if (!ufs->indexdir)
> -               ufs->config.index = false;
> -
>         if (remote)
>                 sb->s_d_op = &ovl_reval_dentry_operations;
>         else
> --
> 2.7.4
>

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Miklos Szeredi]

On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Just like on failure to create work dir or index dir, when
> index dir verification or cleanup fails, mount the overlay
> read-only with a warning instead of failing the mount.

Why?

The failure to create case was done to ensure we can mount the overlay
read-only with the same options if the underlying layers are read-only
as well.

This, however, does not have such a use case.  Or does it?

Thanks,
Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 215b6d23d944..fbb317ad55f6 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1066,19 +1066,21 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                         /* Verify upper root is index dir origin */
>                         err = ovl_verify_origin(ufs->indexdir, ufs->upper_mnt,
>                                                 upperpath.dentry, true, true);
> -                       if (err)
> -                               pr_err("overlayfs: failed to verify index dir origin\n");
> -
>                         /* Cleanup bad/stale/orphan index entries */
>                         if (!err)
>                                 err = ovl_indexdir_cleanup(ufs->indexdir,
>                                                            ufs->upper_mnt,
>                                                            stack, numlower);
> +                       if (err)
> +                               pr_warn("overlayfs: failed to verify index dir (err=%i); mounting read-only\n",
> +                                       err);
> +               }
> +               if (err || !ufs->indexdir) {
> +                       pr_warn("overlayfs: delete index dir or mount with '-o index=off' to disable inodes index.\n");
> +                       sb->s_flags |= MS_RDONLY;
> +                       dput(ufs->indexdir);
> +                       ufs->indexdir = NULL;
>                 }
> -               if (err || !ufs->indexdir)
> -                       pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
> -               if (err)
> -                       goto out_put_indexdir;
>         }
>
>         if (remote)
> --
> 2.7.4
>

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> When inodes index feature is enabled, if lower dir does not match the
> origin fh stored in upper dir, or if no lower dir was found by name,
> try to decode the stored origin fh and use the result as the merge dir
> lower dentry.
>
> This change is needed for indexing of merge directories. A merge
> directory is indexed by the file handle of the uppermost lower dir and
> the index will have a reference to the upper dir by file handle.
> Following down from upper by origin file handle ensures the integrity
> of the index for non-stale origin entries. index entries with stale origin
> are going to be cleaned up on mount anyway.

Not sure I understand.  In what case is following down by origin needed?

Thanks,
Miklos

Re: [PATCH 06/10] ovl: force read-only mount with no index dir

[Amir Goldstein]

On Thu, Jul 13, 2017 at 11:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> When workdir creation fails, overlay is mounted read-only and
>> remount,rw is not allowed. When index dir creation fails, overlay
>> is mounted readonly, but we also need to enforce no remount,rw in
>> that case.
>
> I don't understand the logic for ovl_show_options().  It should show
> the options supplied, right?  It doesn't currently do that for
> "index=FOO" but this patch doesn't help that, afaics.
>

I'll explain my reasoning for changing the logic of ovl_show_options(),
but in the end, I'm perfectly fine with leaving that function untouched
or with removing the != def_ condition altogether.

for the purpose of the test in ovl_force_readonly(), I wanted to preserve
the admin (or distro) choice for index configuration, so that the following
sequence will fail to remount rw in case index dir creation fails:

mount -t overlay d /m -oro,upperdir=/u,lowerdir=/l,workdlir=/w,index=on
mount /m -oremount,rw

So I removed the "/* Show index=off/on.." hunk which lost the index config.
This is different from the case of !ovl_can_decode_fh() which sets
config.index = false and warns "falling back to index=off.", because there
is no way for admin to ever fix  !ovl_can_decode_fh() failure.
In this case (failure to create indexdir), we preserve the config and warn
"failed ... mounting read-only; ... mount with '-o index=off' to disable.."
to let the admin maybe fix the reason for the failure later while letting
users read the data in the mean while without breaking hardlinks.

So far, I guess you agree with the change?

Then it left ovl_show_options() showing "ro,index=on" which is weird,
because indexing is not on, but showing "ro,index=off" (because of !indexdir)
would have been weird as well, because it leaves no apparent explanation
for the failure to remount rw.
So I opted for "on ro mount, the value of index is not interesting" to
reconcile this dilemma.

Really, I have no strong opinion on the matter, so feel free to change
ovl_show_options() in any way you like. I'll shout if I find your choice
confusing for any configuration.

Thanks,
Amir.

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Amir Goldstein]

On Thu, Jul 13, 2017 at 11:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Just like on failure to create work dir or index dir, when
>> index dir verification or cleanup fails, mount the overlay
>> read-only with a warning instead of failing the mount.
>
> Why?

First of all, why not? to me, it makes more sense to treat
"cannot create indexdir" and "cannot use existing indexdir" the same way
and provide slightly better user experience.
But I agree that this patch doesn't fix any bug or correctness issue.

>
> The failure to create case was done to ensure we can mount the overlay
> read-only with the same options if the underlying layers are read-only
> as well.
>
> This, however, does not have such a use case.  Or does it?
>

I can think of two corner use cases:
1. We have a bug that causes ovl_indexdir_cleanup() failure.
2. Trying to reuse workdir from previous mount with a new upperdir.

The second use case happened to me both with unionmount-testsuite
and with xfstests.
I pushed a fix to unionmount-testsuite:
d283eab - Use separate workdir per upper dir
and posted a fix for xfstests.

Of course I am not concerned about out of date testsuite failing tests,
but it goes to show that if utilities *can* reuse workdir with new
upperdir they may very well do so, just on account on being used
to overlayfs cleaning workdir for them on mount.

If such a utility exists, say to create a new overlay of /home/guest/ dir
on every login and it happens to reuse the same workdir for every login,
then upgrading to new kernel with CONFIG_OVERLAYFS_INDEX=y will
result in failure to mount /home/guest on login.

I claim, that we would be better off falling back to ro mount, which could,
maybe slightly, alter the pitch of the voice of the shouting we get from
admins.

But maybe I am miss-reading the shouting chain. I have no experience
interacting with downstream projects.

Thanks,
Amir.

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Thu, Jul 13, 2017 at 11:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> When inodes index feature is enabled, if lower dir does not match the
>> origin fh stored in upper dir, or if no lower dir was found by name,
>> try to decode the stored origin fh and use the result as the merge dir
>> lower dentry.
>>
>> This change is needed for indexing of merge directories. A merge
>> directory is indexed by the file handle of the uppermost lower dir and
>> the index will have a reference to the upper dir by file handle.
>> Following down from upper by origin file handle ensures the integrity
>> of the index for non-stale origin entries. index entries with stale origin
>> are going to be cleaned up on mount anyway.
>
> Not sure I understand.  In what case is following down by origin needed?
>

First, a snip from cover letter, parts of it may belong in this commit message:

-------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8

Specifically, patches 9-10 introduce a behavior change in the
case of changes to lower layer (rename or delete of lower dir).
The new behavior is to verify origin fh and try to decode it
if verification fails (overlayfs.txt updated).

This change in needed for snapshots and I argue in the commit
message of patch 10 why it is needed for indexing of directories.
I admit that the argument is not rigorous, but since the behavior of
changes to lower layer is documented as "undefined", I recon you
don't really mind if we change it for index=on.

The rational of making this change in v4.13 as opposed to when
directory indexing is implemented, is that index=on already introduces
a behavior change related to copying layers, so IMO it is better to
introduce all those behavior changes together on index=on.

-------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8

My main argument is that 10/10 is needed for snapshots along with patch
9/10 and assuming that you don't object to the change of behavior in 9/10,
then perhaps you are ambivalent to this change as well (apparently not).

My secondary NFS export related argument goes something like this.

<more hand waving, but while writing stuff on the board>
An indexed merge dir would look something like:

lower/dir[encode_fh] = 0001
upper/dir[encode_fh] = 0002
upper/dir[origin] = 0001
index/0001[upper] = 0002
overlay/dir[encode_fh] = 0001

When encoding the overlay/dir we get a file handle of 0001.
When decoding overlay handle 0001, we find an index that points
to upper that points to origin 0001, so all is sane.

Now, if lower/dir is renamed to dir2 and a new dir is created in its place
we get:

lower/dir[encode_fh] = 0003
lower/dir2[encode_fh] = 0001
upper/dir[encode_fh] = 0002
upper/dir[origin] = 0001
index/0001[upper_fh] = 0002
overlay/dir[encode_fh] = ???

Now when we encode overlay/dir we either get 0003 (current kernel)
or 0002 (patch 9/10) or 0001 (patch 10/10).
decoding 0002 will get us the pure upper dir not merged with the new
lower/dir which is consistent with what you get in lookup (path 9/10).

However! if NFS client has the file handle 0001, from before lower/dir
was renamed, then decoding that file handle, will get the same object as
when decoding 0002 and that same object will not be re-decoded to
0001, which is another inconsistency.

So yes, this certainly qualifies for "If the underlying filesystem is changed,
the behavior of the overlay is undefined...", but can if you agree that the
cost of patch 10/10 is not high, then it makes the entire
lookup/encode/decode flow file handle verified and then we can work with
stronger assumptions on overlay NFS handles.

</more hand waving>

Feeling any more at ease with this argument?

Amir.

Re: [PATCH 06/10] ovl: force read-only mount with no index dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 8:11 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 11:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> When workdir creation fails, overlay is mounted read-only and
>>> remount,rw is not allowed. When index dir creation fails, overlay
>>> is mounted readonly, but we also need to enforce no remount,rw in
>>> that case.
>>
>> I don't understand the logic for ovl_show_options().  It should show
>> the options supplied, right?  It doesn't currently do that for
>> "index=FOO" but this patch doesn't help that, afaics.
>>
>
> I'll explain my reasoning for changing the logic of ovl_show_options(),
> but in the end, I'm perfectly fine with leaving that function untouched
> or with removing the != def_ condition altogether.

The != def_ is apparently what other fs do, so I don't have a problem with that.

>
> for the purpose of the test in ovl_force_readonly(), I wanted to preserve
> the admin (or distro) choice for index configuration, so that the following
> sequence will fail to remount rw in case index dir creation fails:
>
> mount -t overlay d /m -oro,upperdir=/u,lowerdir=/l,workdlir=/w,index=on
> mount /m -oremount,rw
>
> So I removed the "/* Show index=off/on.." hunk which lost the index config.
> This is different from the case of !ovl_can_decode_fh() which sets
> config.index = false and warns "falling back to index=off.", because there
> is no way for admin to ever fix  !ovl_can_decode_fh() failure.
> In this case (failure to create indexdir), we preserve the config and warn
> "failed ... mounting read-only; ... mount with '-o index=off' to disable.."
> to let the admin maybe fix the reason for the failure later while letting
> users read the data in the mean while without breaking hardlinks.
>
> So far, I guess you agree with the change?

I think it would be clearer if we just never altered config.index and
used a separate flag to decide if we can actually use indexing or not.

I'll have a go at cleaning up ovl_fill_super(), and fix these little
things in the process.

Thanks,
Miklos

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 8:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 11:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Just like on failure to create work dir or index dir, when
>>> index dir verification or cleanup fails, mount the overlay
>>> read-only with a warning instead of failing the mount.
>>
>> Why?
>
> First of all, why not? to me, it makes more sense to treat
> "cannot create indexdir" and "cannot use existing indexdir" the same way
> and provide slightly better user experience.

Not sure.  Failure is the best way to alert the user of a problem and
"cannot use existing indexdir" is a permanent error that can only be
fixed by removing the indexdir.

The failure to create workdir case is possibly temporary error (we
could check for EROFS to make sure) and one that happens in real life.
It was an actual bug report that resulted in this behavior AFAIR.

> But I agree that this patch doesn't fix any bug or correctness issue.
>
>>
>> The failure to create case was done to ensure we can mount the overlay
>> read-only with the same options if the underlying layers are read-only
>> as well.
>>
>> This, however, does not have such a use case.  Or does it?
>>
>
> I can think of two corner use cases:
> 1. We have a bug that causes ovl_indexdir_cleanup() failure.
> 2. Trying to reuse workdir from previous mount with a new upperdir.
>
> The second use case happened to me both with unionmount-testsuite
> and with xfstests.
> I pushed a fix to unionmount-testsuite:
> d283eab - Use separate workdir per upper dir
> and posted a fix for xfstests.
>
> Of course I am not concerned about out of date testsuite failing tests,
> but it goes to show that if utilities *can* reuse workdir with new
> upperdir they may very well do so, just on account on being used
> to overlayfs cleaning workdir for them on mount.
>
> If such a utility exists, say to create a new overlay of /home/guest/ dir
> on every login and it happens to reuse the same workdir for every login,
> then upgrading to new kernel with CONFIG_OVERLAYFS_INDEX=y will
> result in failure to mount /home/guest on login.
>
> I claim, that we would be better off falling back to ro mount, which could,
> maybe slightly, alter the pitch of the voice of the shouting we get from
> admins.
>
> But maybe I am miss-reading the shouting chain. I have no experience
> interacting with downstream projects.

Maybe better just add big warnings about back incompatibility of
CONFIG_OVERLAYFS_INDEX=y to make sure unsuspecting sysadmins don't
turn this on accidentally.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 9:42 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 11:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> When inodes index feature is enabled, if lower dir does not match the
>>> origin fh stored in upper dir, or if no lower dir was found by name,
>>> try to decode the stored origin fh and use the result as the merge dir
>>> lower dentry.
>>>
>>> This change is needed for indexing of merge directories. A merge
>>> directory is indexed by the file handle of the uppermost lower dir and
>>> the index will have a reference to the upper dir by file handle.
>>> Following down from upper by origin file handle ensures the integrity
>>> of the index for non-stale origin entries. index entries with stale origin
>>> are going to be cleaned up on mount anyway.
>>
>> Not sure I understand.  In what case is following down by origin needed?
>>
>
> First, a snip from cover letter, parts of it may belong in this commit message:
>
> -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8
>
> Specifically, patches 9-10 introduce a behavior change in the
> case of changes to lower layer (rename or delete of lower dir).
> The new behavior is to verify origin fh and try to decode it
> if verification fails (overlayfs.txt updated).
>
> This change in needed for snapshots and I argue in the commit
> message of patch 10 why it is needed for indexing of directories.
> I admit that the argument is not rigorous, but since the behavior of
> changes to lower layer is documented as "undefined", I recon you
> don't really mind if we change it for index=on.

The behavior is undefined IFF the change is done while the overlay is
mounted.  Offline modifications are allowed and have defined behavior.

Now following lower renames is also a defined behavior but it's a
different defined behavior, so I don't think we should switch to that
without explicitly opting in.  And I don't think the opt-in has to
necessarily happen together with indexing.

So I'd argue for a separate mount option (verify_dir was the suggested name?).


>
> The rational of making this change in v4.13 as opposed to when
> directory indexing is implemented, is that index=on already introduces
> a behavior change related to copying layers, so IMO it is better to
> introduce all those behavior changes together on index=on.
>
> -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8
>
> My main argument is that 10/10 is needed for snapshots along with patch
> 9/10 and assuming that you don't object to the change of behavior in 9/10,
> then perhaps you are ambivalent to this change as well (apparently not).

I missed the fact that 9/10 also changes behavior.  Not good.

> My secondary NFS export related argument goes something like this.
>
> <more hand waving, but while writing stuff on the board>
> An indexed merge dir would look something like:
>
> lower/dir[encode_fh] = 0001
> upper/dir[encode_fh] = 0002
> upper/dir[origin] = 0001
> index/0001[upper] = 0002
> overlay/dir[encode_fh] = 0001
>
> When encoding the overlay/dir we get a file handle of 0001.
> When decoding overlay handle 0001, we find an index that points
> to upper that points to origin 0001, so all is sane.
>
> Now, if lower/dir is renamed to dir2 and a new dir is created in its place
> we get:
>
> lower/dir[encode_fh] = 0003
> lower/dir2[encode_fh] = 0001
> upper/dir[encode_fh] = 0002
> upper/dir[origin] = 0001
> index/0001[upper_fh] = 0002
> overlay/dir[encode_fh] = ???
>
> Now when we encode overlay/dir we either get 0003 (current kernel)
> or 0002 (patch 9/10) or 0001 (patch 10/10).
> decoding 0002 will get us the pure upper dir not merged with the new
> lower/dir which is consistent with what you get in lookup (path 9/10).
>
> However! if NFS client has the file handle 0001, from before lower/dir
> was renamed, then decoding that file handle, will get the same object as
> when decoding 0002 and that same object will not be re-decoded to
> 0001, which is another inconsistency.
>
> So yes, this certainly qualifies for "If the underlying filesystem is changed,
> the behavior of the overlay is undefined...", but can if you agree that the
> cost of patch 10/10 is not high, then it makes the entire
> lookup/encode/decode flow file handle verified and then we can work with
> stronger assumptions on overlay NFS handles.
>
> </more hand waving>
>
> Feeling any more at ease with this argument?

For NFS export and snapshots this certainly makes sense.  But enabling
it for index=on by default might not be a good idea, IMO.

Thanks,
Miklos

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Amir Goldstein]

On Fri, Jul 14, 2017 at 1:05 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jul 14, 2017 at 8:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Thu, Jul 13, 2017 at 11:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Just like on failure to create work dir or index dir, when
>>>> index dir verification or cleanup fails, mount the overlay
>>>> read-only with a warning instead of failing the mount.
>>>
>>> Why?
>>
>> First of all, why not? to me, it makes more sense to treat
>> "cannot create indexdir" and "cannot use existing indexdir" the same way
>> and provide slightly better user experience.
>
> Not sure.  Failure is the best way to alert the user of a problem and
> "cannot use existing indexdir" is a permanent error that can only be
> fixed by removing the indexdir.
>
> The failure to create workdir case is possibly temporary error (we
> could check for EROFS to make sure) and one that happens in real life.
> It was an actual bug report that resulted in this behavior AFAIR.
>
>> But I agree that this patch doesn't fix any bug or correctness issue.
>>
>>>
>>> The failure to create case was done to ensure we can mount the overlay
>>> read-only with the same options if the underlying layers are read-only
>>> as well.
>>>
>>> This, however, does not have such a use case.  Or does it?
>>>
>>
>> I can think of two corner use cases:
>> 1. We have a bug that causes ovl_indexdir_cleanup() failure.
>> 2. Trying to reuse workdir from previous mount with a new upperdir.
>>
>> The second use case happened to me both with unionmount-testsuite
>> and with xfstests.
>> I pushed a fix to unionmount-testsuite:
>> d283eab - Use separate workdir per upper dir
>> and posted a fix for xfstests.
>>
>> Of course I am not concerned about out of date testsuite failing tests,
>> but it goes to show that if utilities *can* reuse workdir with new
>> upperdir they may very well do so, just on account on being used
>> to overlayfs cleaning workdir for them on mount.
>>
>> If such a utility exists, say to create a new overlay of /home/guest/ dir
>> on every login and it happens to reuse the same workdir for every login,
>> then upgrading to new kernel with CONFIG_OVERLAYFS_INDEX=y will
>> result in failure to mount /home/guest on login.
>>
>> I claim, that we would be better off falling back to ro mount, which could,
>> maybe slightly, alter the pitch of the voice of the shouting we get from
>> admins.
>>
>> But maybe I am miss-reading the shouting chain. I have no experience
>> interacting with downstream projects.
>
> Maybe better just add big warnings about back incompatibility of
> CONFIG_OVERLAYFS_INDEX=y to make sure unsuspecting sysadmins don't
> turn this on accidentally.
>

What? always warn on boot? on mount? I donno. To me the comment
about backward compat in Kconfig seems sufficient and no sysadmin
can mistakenly set index=on on mount, but as I said, don't know
enough about downstream to say much.
i can only say that your arguments sound just as convincing and i won't
shed any tears if this patch is dropped.
We can always apply it as stable fix if the need arise.

Amir.

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 12:35 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> What? always warn on boot? on mount? I donno. To me the comment
> about backward compat in Kconfig seems sufficient and no sysadmin
> can mistakenly set index=on on mount, but as I said, don't know
> enough about downstream to say much.

I meant warning in Kconfig.   I see that there is a warning there, but
it could be made shorter and more explicit.   No need to explain the
hows and whys, just warn that enabling this option is not backward
compatible.

And if distros enable it despite this warning, and it burns their
users, that's their problem.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Fri, Jul 14, 2017 at 1:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jul 14, 2017 at 9:42 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Thu, Jul 13, 2017 at 11:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, Jul 11, 2017 at 2:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> When inodes index feature is enabled, if lower dir does not match the
>>>> origin fh stored in upper dir, or if no lower dir was found by name,
>>>> try to decode the stored origin fh and use the result as the merge dir
>>>> lower dentry.
>>>>
>>>> This change is needed for indexing of merge directories. A merge
>>>> directory is indexed by the file handle of the uppermost lower dir and
>>>> the index will have a reference to the upper dir by file handle.
>>>> Following down from upper by origin file handle ensures the integrity
>>>> of the index for non-stale origin entries. index entries with stale origin
>>>> are going to be cleaned up on mount anyway.
>>>
>>> Not sure I understand.  In what case is following down by origin needed?
>>>
>>
>> First, a snip from cover letter, parts of it may belong in this commit message:
>>
>> -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8
>>
>> Specifically, patches 9-10 introduce a behavior change in the
>> case of changes to lower layer (rename or delete of lower dir).
>> The new behavior is to verify origin fh and try to decode it
>> if verification fails (overlayfs.txt updated).
>>
>> This change in needed for snapshots and I argue in the commit
>> message of patch 10 why it is needed for indexing of directories.
>> I admit that the argument is not rigorous, but since the behavior of
>> changes to lower layer is documented as "undefined", I recon you
>> don't really mind if we change it for index=on.
>
> The behavior is undefined IFF the change is done while the overlay is
> mounted.  Offline modifications are allowed and have defined behavior.

OK. but when NFS client has file handle it changes the rules, because
"offline" modifications are not as "offline" as they use to be.

>
> Now following lower renames is also a defined behavior but it's a
> different defined behavior, so I don't think we should switch to that
> without explicitly opting in.  And I don't think the opt-in has to
> necessarily happen together with indexing.
>
> So I'd argue for a separate mount option (verify_dir was the suggested name?).

Yes, I can bring it back. Not a problem on my end.
My thinking was - indexing is mainly needed for NFS export and NFS export
benefits from verify_dir, so why add another configuration?
Is the use case of using indexing strictly for not breaking hardlinks so
appealing that we should add another dimension to the test matrix?

>
>
>>
>> The rational of making this change in v4.13 as opposed to when
>> directory indexing is implemented, is that index=on already introduces
>> a behavior change related to copying layers, so IMO it is better to
>> introduce all those behavior changes together on index=on.
>>
>> -------->8-------->8-------->8-------->8-------->8-------->8-------->8-------->8
>>
>> My main argument is that 10/10 is needed for snapshots along with patch
>> 9/10 and assuming that you don't object to the change of behavior in 9/10,
>> then perhaps you are ambivalent to this change as well (apparently not).
>
> I missed the fact that 9/10 also changes behavior.  Not good.

Yes, I should have been more loud about this.
The changes to overlay.txt describe this conditional behavior change,
but not the commit message.

>
>> My secondary NFS export related argument goes something like this.
>>
...
>>
>> Feeling any more at ease with this argument?
>
> For NFS export and snapshots this certainly makes sense.  But enabling
> it for index=on by default might not be a good idea, IMO.
>

My intention for NFS export (already scribbled on ovl-nfs-export-wip branch)
was to enable it based on underlying exportfs support AND index=on.
I didn't think that another opt-in config option was in order?

The only justification to merge patches 9,10 to v4.13 is if we want to
bundle hardlinks and NFS export on the same config option, so document
and make all the behavior changes in the same merge cycle and with
the same config option.

If you don't want to bundle index=on with NFS export, then no rush to merge
these patches now. The added benefit on patch 9 (retroactive marking merge
dir with origin for not exposing whiteouts) is relevant to a patch that is not
going to land in v4.13 anyway. right?

Amir.

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Amir Goldstein]

On Fri, Jul 14, 2017 at 1:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jul 14, 2017 at 12:35 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> What? always warn on boot? on mount? I donno. To me the comment
>> about backward compat in Kconfig seems sufficient and no sysadmin
>> can mistakenly set index=on on mount, but as I said, don't know
>> enough about downstream to say much.
>
> I meant warning in Kconfig.   I see that there is a warning there, but
> it could be made shorter and more explicit.   No need to explain the
> hows and whys, just warn that enabling this option is not backward
> compatible.

By all means. I assume you will clean that up.

>
> And if distros enable it despite this warning, and it burns their
> users, that's their problem.
>

Hmm... but the issue related to this commit is not backward compatibility
is the sense that is depicted in Kconfig warning.
The warning speaks of mounting new overlay with an old kernel.
The concern discussed in this thread is mounting an old overlay
with a new kernel:

This used to work with old kernel:
rm -rf /u.new
mkdir /u.new
mount -t overlay d /m -oupperdir=/u.new,lowerdir=/l.old,workdlir=/w.old

And doesn't work with new kernel with INDEX=y.

Another big behavior change that me good to warn about in Kconfig
is not being able to copy layers.

I think it would be best if you do the rewording ;)

Thanks,
Amir.

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 1:17 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jul 14, 2017 at 1:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, Jul 14, 2017 at 12:35 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>>> What? always warn on boot? on mount? I donno. To me the comment
>>> about backward compat in Kconfig seems sufficient and no sysadmin
>>> can mistakenly set index=on on mount, but as I said, don't know
>>> enough about downstream to say much.
>>
>> I meant warning in Kconfig.   I see that there is a warning there, but
>> it could be made shorter and more explicit.   No need to explain the
>> hows and whys, just warn that enabling this option is not backward
>> compatible.
>
> By all means. I assume you will clean that up.
>
>>
>> And if distros enable it despite this warning, and it burns their
>> users, that's their problem.
>>
>
> Hmm... but the issue related to this commit is not backward compatibility
> is the sense that is depicted in Kconfig warning.
> The warning speaks of mounting new overlay with an old kernel.
> The concern discussed in this thread is mounting an old overlay
> with a new kernel:
>
> This used to work with old kernel:
> rm -rf /u.new
> mkdir /u.new
> mount -t overlay d /m -oupperdir=/u.new,lowerdir=/l.old,workdlir=/w.old
>
> And doesn't work with new kernel with INDEX=y.
>
> Another big behavior change that me good to warn about in Kconfig
> is not being able to copy layers.

Right.  The fact is: INDEX=y is not compatible with INDEX=n.  Despite
that, it might well make sense for some to enable it by default,
because compatibility of overlay images is not always (rarely?) an
issue in practice.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> If you don't want to bundle index=on with NFS export, then no rush to merge
> these patches now. The added benefit on patch 9 (retroactive marking merge
> dir with origin for not exposing whiteouts) is relevant to a patch that is not
> going to land in v4.13 anyway. right?

Right.

Can we maybe postpone the verification until fh decode time?  That
would spare us from creating a new config option.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Mon, Jul 24, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> If you don't want to bundle index=on with NFS export, then no rush to merge
>> these patches now. The added benefit on patch 9 (retroactive marking merge
>> dir with origin for not exposing whiteouts) is relevant to a patch that is not
>> going to land in v4.13 anyway. right?
>
> Right.
>
> Can we maybe postpone the verification until fh decode time?

Not sure I follow.
Failure to verify lower dir can result in merge dir not being merged or being
merged with a different lower dir (followed by fh).

The question is which inode we choose to encode in case lower dir cannot be
verified: the unverified lower inode, the stored origin inode or the
upper inode.

Are you suggesting that "normal" lookup will merge the unverified lower dir
and then a pair of encode/decode will result in another dentry?
or are you suggesting to return ESTALE in that case?
If latter, then that error we be permanent? encode/decode will alway end up
with ESTALE??

Please explain.

Amir.

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Mon, Jul 24, 2017 at 2:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Jul 24, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>>> If you don't want to bundle index=on with NFS export, then no rush to merge
>>> these patches now. The added benefit on patch 9 (retroactive marking merge
>>> dir with origin for not exposing whiteouts) is relevant to a patch that is not
>>> going to land in v4.13 anyway. right?
>>
>> Right.
>>
>> Can we maybe postpone the verification until fh decode time?
>
> Not sure I follow.
> Failure to verify lower dir can result in merge dir not being merged or being
> merged with a different lower dir (followed by fh).
>
> The question is which inode we choose to encode in case lower dir cannot be
> verified: the unverified lower inode, the stored origin inode or the
> upper inode.
>
> Are you suggesting that "normal" lookup will merge the unverified lower dir
> and then a pair of encode/decode will result in another dentry?
> or are you suggesting to return ESTALE in that case?
> If latter, then that error we be permanent? encode/decode will alway end up
> with ESTALE??

Putting a bit more thought into this...

I think we have several different cases

 a) offline move of lower dir, no active NFS export

 b) online move of lower dir, no active NFS export

 c) offline move of lower dir, active NFS export

 d) online move of lower dir, active NFS export

 e) offline move of lower dir, snapshotfs

 f) online move of lower dir, snapshotfs

The behavior of (a) is well defined: we do not follow origin of
directories for merging.  It might make sense to introduce a variant
that does follow origin, but I'm not sure I see the usefulness.

The behavior of (b) is undefined, it might make sense to add an
ovl_d_revalidate() and make that case behave like (a)

For first version we can say that (c) and (d) are both undefined.  I
think the logical behavior would be to return ESTALE when we detect
such an inconsistency (not sure how expensive that would be) and then
the lookup would end up with the updated lower.

For (f) we obviously need to follow the origin dir and I guess we
don't care about (e).

To summarize, only snapshotfs and maybe some other special application
of overlay would need the directory following behavior.   For NFS
export we can just do whatever is simplest in the unverified case for
now.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Tue, Jul 25, 2017 at 2:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Jul 24, 2017 at 2:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Jul 24, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>
>>>> If you don't want to bundle index=on with NFS export, then no rush to merge
>>>> these patches now. The added benefit on patch 9 (retroactive marking merge
>>>> dir with origin for not exposing whiteouts) is relevant to a patch that is not
>>>> going to land in v4.13 anyway. right?
>>>
>>> Right.
>>>
>>> Can we maybe postpone the verification until fh decode time?
>>
>> Not sure I follow.
>> Failure to verify lower dir can result in merge dir not being merged or being
>> merged with a different lower dir (followed by fh).
>>
>> The question is which inode we choose to encode in case lower dir cannot be
>> verified: the unverified lower inode, the stored origin inode or the
>> upper inode.
>>
>> Are you suggesting that "normal" lookup will merge the unverified lower dir
>> and then a pair of encode/decode will result in another dentry?
>> or are you suggesting to return ESTALE in that case?
>> If latter, then that error we be permanent? encode/decode will alway end up
>> with ESTALE??
>
> Putting a bit more thought into this...
>
> I think we have several different cases
>
>  a) offline move of lower dir, no active NFS export
>
>  b) online move of lower dir, no active NFS export
>
>  c) offline move of lower dir, active NFS export
>
>  d) online move of lower dir, active NFS export
>
>  e) offline move of lower dir, snapshotfs
>
>  f) online move of lower dir, snapshotfs
>
> The behavior of (a) is well defined: we do not follow origin of
> directories for merging.  It might make sense to introduce a variant
> that does follow origin, but I'm not sure I see the usefulness.
>
> The behavior of (b) is undefined, it might make sense to add an
> ovl_d_revalidate() and make that case behave like (a)
>
> For first version we can say that (c) and (d) are both undefined.  I
> think the logical behavior would be to return ESTALE when we detect
> such an inconsistency (not sure how expensive that would be) and then
> the lookup would end up with the updated lower.
>

I understand. This sounds reasonable. The hard part about making
behavior of (d) defined is that we cannot tell if a lower dir that was
encoded has not been copied up or was copied up and then lower has
moved.
The cost of indexing merge dir on copy up or on decode is too high imo.
So actually, following decoded origin of merge dir doesn't make decode
behavior 100% defined, just more likely to return a consistent
dentry..

Bottom line is that splitting follow origin to a new mount flag for
snapshot is fine by me. Do you intend to preserve the behavior change
of NOT following unverified lower dir?  Without this change ESTALE
error could become persistent.

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Tue, Jul 25, 2017 at 4:30 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jul 25, 2017 at 2:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Mon, Jul 24, 2017 at 2:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Mon, Jul 24, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>>> If you don't want to bundle index=on with NFS export, then no rush to merge
>>>>> these patches now. The added benefit on patch 9 (retroactive marking merge
>>>>> dir with origin for not exposing whiteouts) is relevant to a patch that is not
>>>>> going to land in v4.13 anyway. right?
>>>>
>>>> Right.
>>>>
>>>> Can we maybe postpone the verification until fh decode time?
>>>
>>> Not sure I follow.
>>> Failure to verify lower dir can result in merge dir not being merged or being
>>> merged with a different lower dir (followed by fh).
>>>
>>> The question is which inode we choose to encode in case lower dir cannot be
>>> verified: the unverified lower inode, the stored origin inode or the
>>> upper inode.
>>>
>>> Are you suggesting that "normal" lookup will merge the unverified lower dir
>>> and then a pair of encode/decode will result in another dentry?
>>> or are you suggesting to return ESTALE in that case?
>>> If latter, then that error we be permanent? encode/decode will alway end up
>>> with ESTALE??
>>
>> Putting a bit more thought into this...
>>
>> I think we have several different cases
>>
>>  a) offline move of lower dir, no active NFS export
>>
>>  b) online move of lower dir, no active NFS export
>>
>>  c) offline move of lower dir, active NFS export
>>
>>  d) online move of lower dir, active NFS export
>>
>>  e) offline move of lower dir, snapshotfs
>>
>>  f) online move of lower dir, snapshotfs
>>
>> The behavior of (a) is well defined: we do not follow origin of
>> directories for merging.  It might make sense to introduce a variant
>> that does follow origin, but I'm not sure I see the usefulness.
>>
>> The behavior of (b) is undefined, it might make sense to add an
>> ovl_d_revalidate() and make that case behave like (a)
>>
>> For first version we can say that (c) and (d) are both undefined.  I
>> think the logical behavior would be to return ESTALE when we detect
>> such an inconsistency (not sure how expensive that would be) and then
>> the lookup would end up with the updated lower.
>>
>
> I understand. This sounds reasonable. The hard part about making
> behavior of (d) defined is that we cannot tell if a lower dir that was
> encoded has not been copied up or was copied up and then lower has
> moved.

Wait, copy-up creates index, no?

> Bottom line is that splitting follow origin to a new mount flag for
> snapshot is fine by me. Do you intend to preserve the behavior change
> of NOT following unverified lower dir?  Without this change ESTALE
> error could become persistent.

I don't get it.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Tue, Jul 25, 2017 at 6:16 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jul 25, 2017 at 4:30 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Jul 25, 2017 at 2:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Mon, Jul 24, 2017 at 2:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Mon, Jul 24, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>> On Fri, Jul 14, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>
>>>>>> If you don't want to bundle index=on with NFS export, then no rush to merge
>>>>>> these patches now. The added benefit on patch 9 (retroactive marking merge
>>>>>> dir with origin for not exposing whiteouts) is relevant to a patch that is not
>>>>>> going to land in v4.13 anyway. right?
>>>>>
>>>>> Right.
>>>>>
>>>>> Can we maybe postpone the verification until fh decode time?
>>>>
>>>> Not sure I follow.
>>>> Failure to verify lower dir can result in merge dir not being merged or being
>>>> merged with a different lower dir (followed by fh).
>>>>
>>>> The question is which inode we choose to encode in case lower dir cannot be
>>>> verified: the unverified lower inode, the stored origin inode or the
>>>> upper inode.
>>>>
>>>> Are you suggesting that "normal" lookup will merge the unverified lower dir
>>>> and then a pair of encode/decode will result in another dentry?
>>>> or are you suggesting to return ESTALE in that case?
>>>> If latter, then that error we be permanent? encode/decode will alway end up
>>>> with ESTALE??
>>>
>>> Putting a bit more thought into this...
>>>
>>> I think we have several different cases
>>>
>>>  a) offline move of lower dir, no active NFS export
>>>
>>>  b) online move of lower dir, no active NFS export
>>>
>>>  c) offline move of lower dir, active NFS export
>>>
>>>  d) online move of lower dir, active NFS export
>>>
>>>  e) offline move of lower dir, snapshotfs
>>>
>>>  f) online move of lower dir, snapshotfs
>>>
>>> The behavior of (a) is well defined: we do not follow origin of
>>> directories for merging.  It might make sense to introduce a variant
>>> that does follow origin, but I'm not sure I see the usefulness.
>>>
>>> The behavior of (b) is undefined, it might make sense to add an
>>> ovl_d_revalidate() and make that case behave like (a)
>>>
>>> For first version we can say that (c) and (d) are both undefined.  I
>>> think the logical behavior would be to return ESTALE when we detect
>>> such an inconsistency (not sure how expensive that would be) and then
>>> the lookup would end up with the updated lower.
>>>
>>
>> I understand. This sounds reasonable. The hard part about making
>> behavior of (d) defined is that we cannot tell if a lower dir that was
>> encoded has not been copied up or was copied up and then lower has
>> moved.
>
> Wait, copy-up creates index, no?

That's what I thought at first, but then realized we only *need* to
index renamed and unlinked upper objects for NFS export.
My concern w.r.t. indexing on copy up was that performance of index
create and lookup can degrade for large index dir, so better index
only what we need in order to be able to decode.
But if copy up creates index then behavior of (d) can be well defined.
B.t.w my current NFS export WIP does index on copy up.

>
>> Bottom line is that splitting follow origin to a new mount flag for
>> snapshot is fine by me. Do you intend to preserve the behavior change
>> of NOT following unverified lower dir?  Without this change ESTALE
>> error could become persistent.
>
> I don't get it.

Right. That involves some more lower renames. Imagine 2 merged dirs A
and B both decoded by origin. Then the 2 lower dirs swapped. Now
decode of A yields an inconsistent index so return ESTALE. But lookup
of A will follow to unverified lower B and encode of that dentry will
be same as encode of B before the swap, so we are bound to always get
ESTALE after encode/decode. I suppose there are other ways out of this
loop hole besides not following unverified lower. I just don't see
them right now.

Amir.

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Wed, Jul 26, 2017 at 12:19 AM, Amir Goldstein <amir73il@gmail.com> wrote:

>> Wait, copy-up creates index, no?
>
> That's what I thought at first, but then realized we only *need* to
> index renamed and unlinked upper objects for NFS export.
> My concern w.r.t. indexing on copy up was that performance of index
> create and lookup can degrade for large index dir, so better index
> only what we need in order to be able to decode.

Guessing that there won't be many merge dirs in most situations, so
this won't be an issue.

> But if copy up creates index then behavior of (d) can be well defined.
> B.t.w my current NFS export WIP does index on copy up.

Lets keep it that way for the first version.

>>> Bottom line is that splitting follow origin to a new mount flag for
>>> snapshot is fine by me. Do you intend to preserve the behavior change
>>> of NOT following unverified lower dir?  Without this change ESTALE
>>> error could become persistent.
>>
>> I don't get it.
>
> Right. That involves some more lower renames. Imagine 2 merged dirs A
> and B both decoded by origin. Then the 2 lower dirs swapped. Now
> decode of A yields an inconsistent index so return ESTALE. But lookup
> of A will follow to unverified lower B and encode of that dentry will
> be same as encode of B before the swap, so we are bound to always get
> ESTALE after encode/decode. I suppose there are other ways out of this
> loop hole besides not following unverified lower. I just don't see
> them right now.

Aren't we allowed to update the stale index?

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Wed, Jul 26, 2017 at 10:47 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:

> Aren't we allowed to update the stale index?

The answer is no, of course (otherwise client would get success for
something it should get error for).  Eeturning ESTALE is the right
thing to do.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Miklos Szeredi]

On Wed, Jul 26, 2017 at 10:51 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Jul 26, 2017 at 10:47 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> Aren't we allowed to update the stale index?
>
> The answer is no, of course (otherwise client would get success for
> something it should get error for).  Eeturning ESTALE is the right
> thing to do.

Can encode an overlay-generation in the handle.  That would allow us
to have a stale index as well as a new one for the same underlying
origin.

Thanks,
Miklos

Re: [PATCH 10/10] ovl: follow decoded origin file handle of merge dir

[Amir Goldstein]

On Wed, Jul 26, 2017 at 11:54 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Jul 26, 2017 at 10:51 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Wed, Jul 26, 2017 at 10:47 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>>> Aren't we allowed to update the stale index?
>>
>> The answer is no, of course (otherwise client would get success for
>> something it should get error for).  Eeturning ESTALE is the right
>> thing to do.
>
> Can encode an overlay-generation in the handle.  That would allow us
> to have a stale index as well as a new one for the same underlying
> origin.
>

Yes, we can do that.

We will never have two different index generations on-disk at the same time.
A new generation index always obsoletes an old index generation
(index name does not include the generation).

Verifying index/origin consistency should happen no later than at encode time,
but it really doesn't cost much to verify that at merge dir lookup time.
If index/origin is not consistent with merge dir upper/lower, need to replace it
with a newer generation consistent index and need to update origin xattr.
I would think that it also makes sense to verify index->upper->origin->index
consistency on mount, as we anyway should check that index->upper reference
is not stale.

But I must wonder: does carrying this extra complexity for index generation
really worth the effort of not changing behavior of modified lower dir
(when user opts-in for the change with index=on)?
Do you think there are users out there depending on the current behavior
in a setup that does NOT involve copying layers??

Anyway, another not-really-a-reason I am wondering if we can avoid
index generation is that I was going to use struct ovl_fh for both on-disk
(origin xattr and index name) and for on-wire (NFS), so I'm still resisting
the idea to diverge the structs...

Thanks,
Amir.

Re: [PATCH 07/10] ovl: mount overlay read-only on failure to verify index dir

[Amir Goldstein]

On Mon, Jul 24, 2017 at 10:33 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jul 14, 2017 at 1:17 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Jul 14, 2017 at 1:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Fri, Jul 14, 2017 at 12:35 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>
>>>> What? always warn on boot? on mount? I donno. To me the comment
>>>> about backward compat in Kconfig seems sufficient and no sysadmin
>>>> can mistakenly set index=on on mount, but as I said, don't know
>>>> enough about downstream to say much.
>>>
>>> I meant warning in Kconfig.   I see that there is a warning there, but
>>> it could be made shorter and more explicit.   No need to explain the
>>> hows and whys, just warn that enabling this option is not backward
>>> compatible.
>>
>> By all means. I assume you will clean that up.
>>
>>>
>>> And if distros enable it despite this warning, and it burns their
>>> users, that's their problem.
>>>
>>
>> Hmm... but the issue related to this commit is not backward compatibility
>> is the sense that is depicted in Kconfig warning.
>> The warning speaks of mounting new overlay with an old kernel.
>> The concern discussed in this thread is mounting an old overlay
>> with a new kernel:
>>
>> This used to work with old kernel:
>> rm -rf /u.new
>> mkdir /u.new
>> mount -t overlay d /m -oupperdir=/u.new,lowerdir=/l.old,workdlir=/w.old
>>
>> And doesn't work with new kernel with INDEX=y.
>>
>> Another big behavior change that me good to warn about in Kconfig
>> is not being able to copy layers.
>
> Right.  The fact is: INDEX=y is not compatible with INDEX=n.  Despite
> that, it might well make sense for some to enable it by default,
> because compatibility of overlay images is not always (rarely?) an
> issue in practice.
>

Miklos,

Do you plan to push any changes to Kconfig warning for v4.13?
Plan to relax mount failures with -oro,index=on or keep it as is?

Let me know if you want me to do anything in that regard.

Thanks,
Amir.