[PATCH v4 00/15] overlayfs constant inode numbers

[PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

Miklos,

This is v4 series of redirect by file handle and constant inode numbers.
this series fixes constant inode numbers for stat(2) for overlayfs
configuration of all layers on the same fs.

I tried harder to keep this series "as simple as possible", according to
conclusions of our discussion on v3. To this end, I removed that prep
patches of ovl_path_type() and OVL_TYPE_COPYUP(), which I may or may not
need to re-introduced for the next part of the work.

The last patch (verify_lower) is not needed for constant inode numbers,
of course, but is provided here as a demonstration of how redirect_fh can
be extended for the snapshot use case. I did not yet implement decode fh
on failure to verify lower dir. It should not be hard, but I did not want
to stray from "as simple as possible" too much, so I'll defer it to the
second part.

Redirect by file handle is used to lookup of the copy up origin of
non-dir inode. The origin inode reference is then held by overlay dentry
and its inode number used as the overlay object inode number.

Overlayfs configuration of all layers on same fs, also gains:
- Persistent inode numbers for directories
- Consistent st_dev for all overlay objects

Overlayfs configuration of single lower and upper on same fs, also gains:
- With '-o verify_lower', upper dir merged only with verified lower dir

This series is available for testing on [1].

xfstest overlay/017 was added a check for constant and persistent inode
numbers across rename and mount cycle [2].

unionmount-testsuite was instrumented to verify constant inode numbers
after rename/link and mount cycle for the --samefs setup [3].

unionmount-testsuite also sets the new mount option 'verify_lower' on
supported configurations (i.e. ./run --ov=0 --samefs).

I also ran few manual tests of mangling lower dirs to test lower dir
verification failures, but still did not integrate verify_lower with
snapshot to run the full testsuite of lower changes.

Tested the following layer configurations:
 ./run --ov{,=0,=1} {,--samefs}

Where 'samefs' is tmpfs and xfs with sb->s_uuid patch.
When running './run --ov=0 --samefs' with either tmpfs (default) or
with un-patched xfs, the test fails on inode number consistency check
as expected, because copy up origin cannot be followed.

The following test fails inode number consistency check on an origin
lower hardlink, as expected:
 ./run --ov=1 --samefs rename-mass-5

I did not teach unionmount-testsuite to identify the lower hardlink case,
as I intent to fix this problem with the second part of this work.

Amir.

Changes since v3:
- Constant inode numbers only for samefs case
- No constant inode numbers for origin with nlink > 1
- Store lower fs uuid along side fh in overlay.origin (no root fh)
- Verify lower fs uuid is non zero
- Drop ovl_path_type() and OVL_TYPE_COPYUP() patches
- No overlay.redirect for non-dir
- No lookup by name for non-dir origin
- Store non-connectable fh (for non-dir)
- No verify that origin followed by fh is under layer root
- Added 'verify_lower' mount option

[1] https://github.com/amir73il/linux/commits/overlayfs-devel
[2] https://github.com/amir73il/xfstests/commits/overlayfs-devel
[3] https://github.com/amir73il/unionmount-testsuite/commits/overlayfs-devel


Amir Goldstein (15):
  ovl: check if all layers are on the same fs
  ovl: store file handle of lower inode on copy up
  ovl: use an auxiliary var for overlay root entry
  ovl: factor out ovl_lookup_data()
  ovl: store the file type in ovl_lookup_data
  ovl: pass the stack index on ovl_lookup_data
  ovl: lookup copy up origin of non-dir inode
  ovl: lookup non-dir copy up origin by file handle
  ovl: validate lower layer uuid on redirect by fh
  ovl: constant st_ino/st_dev across copy up
  ovl: persistent inode number for directories
  ovl: fix du --one-file-system on overlay mount
  ovl: persistent inode numbers for upper hardlinks
  ovl: update documentation w.r.t. constant inode numbers
  ovl: add support for verify_lower option

 Documentation/filesystems/overlayfs.txt |   9 +-
 fs/overlayfs/copy_up.c                  | 109 +++++++++++
 fs/overlayfs/dir.c                      |  21 ++-
 fs/overlayfs/inode.c                    |  28 ++-
 fs/overlayfs/namei.c                    | 325 ++++++++++++++++++++++++++++----
 fs/overlayfs/overlayfs.h                |  27 +++
 fs/overlayfs/ovl_entry.h                |   6 +
 fs/overlayfs/super.c                    |  60 ++++++
 fs/overlayfs/util.c                     |  49 +++++
 9 files changed, 599 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH v4 01/15] ovl: check if all layers are on the same fs

[Amir Goldstein]

Some features can only work when all lower layers are on the same fs
and some features require that upper layer is also on the same fs.
Test those conditions during mount time, so features can check them later.

Add helper ovl_same_lower_sb() to return the common super block in case
all lower layers are on the same fs and helper ovl_same_sb() to return
the super block common to all layers.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  3 +++
 fs/overlayfs/super.c     |  9 +++++++++
 fs/overlayfs/util.c      | 14 ++++++++++++++
 4 files changed, 28 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 741dc0b..2ddbd44 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -151,6 +151,8 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
+struct super_block *ovl_same_lower_sb(struct super_block *sb);
+struct super_block *ovl_same_sb(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 bool ovl_dentry_remote(struct dentry *dentry);
 bool ovl_dentry_weird(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 59614fa..68fa932 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -29,6 +29,9 @@ struct ovl_fs {
 	const struct cred *creator_cred;
 	bool tmpfile;
 	wait_queue_head_t copyup_wq;
+	/* sb common to all (or all lower) layers */
+	struct super_block *same_lower_sb;
+	struct super_block *same_sb;
 };
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c072a0c..7d56aa8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -898,6 +898,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ufs->lower_mnt = kcalloc(numlower, sizeof(struct vfsmount *), GFP_KERNEL);
 	if (ufs->lower_mnt == NULL)
 		goto out_put_workdir;
+
 	for (i = 0; i < numlower; i++) {
 		struct vfsmount *mnt = clone_private_mount(&stack[i]);
 
@@ -914,11 +915,19 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 		ufs->lower_mnt[ufs->numlower] = mnt;
 		ufs->numlower++;
+
+		/* Check if all lower layers are on same sb */
+		if (i == 0)
+			ufs->same_lower_sb = mnt->mnt_sb;
+		else if (ufs->same_lower_sb != mnt->mnt_sb)
+			ufs->same_lower_sb = NULL;
 	}
 
 	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
 	if (!ufs->upper_mnt)
 		sb->s_flags |= MS_RDONLY;
+	else if (ufs->upper_mnt->mnt_sb == ufs->same_lower_sb)
+		ufs->same_sb = ufs->same_lower_sb;
 
 	if (remote)
 		sb->s_d_op = &ovl_reval_dentry_operations;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 8a1e24a..a474ab9 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -41,6 +41,20 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 	return override_creds(ofs->creator_cred);
 }
 
+struct super_block *ovl_same_lower_sb(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	return ofs->same_lower_sb;
+}
+
+struct super_block *ovl_same_sb(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	return ofs->same_sb;
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
 	size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
-- 
2.7.4

[PATCH v4 02/15] ovl: store file handle of lower inode on copy up

[Amir Goldstein]

Sometimes it is interesting to know if an upper file is pure
upper or a copy up target, and if it is a copy up target, it
may be interesting to find the copy up origin.

This will be used to preserve lower inode numbers across copy up.

Store the lower inode file handle in upper inode extended attribute
overlay.origin on copy up to use it later for these cases.
Store the lower filesystem uuid along side the file handle, so we can
validate that we are looking for the origin file in the original fs.

On failure to encode lower file handle, store an invalid 'null' handle,
so we can always use the overlay.origin xattr to distinguish between
a copy up and a pure upper inode.

If lower fs does not support NFS export ops or if not all layers are
on the same fs, don't try to encode a lower file handle and store the
'null' handle instead.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 109 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |  22 ++++++++++
 fs/overlayfs/ovl_entry.h |   2 +
 fs/overlayfs/super.c     |  17 ++++++++
 fs/overlayfs/util.c      |  14 ++++++
 5 files changed, 164 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 906ea6c..767ae77 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -20,6 +20,8 @@
 #include <linux/namei.h>
 #include <linux/fdtable.h>
 #include <linux/ratelimit.h>
+#include <linux/mount.h>
+#include <linux/exportfs.h>
 #include "overlayfs.h"
 #include "ovl_entry.h"
 
@@ -232,6 +234,105 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
+static struct ovl_fh *ovl_encode_fh(struct dentry *lower)
+{
+	struct ovl_fh *fh;
+	int fh_type, fh_len, dwords;
+	void *buf = NULL;
+	void *ret = NULL;
+	int buflen = MAX_HANDLE_SZ;
+	int err;
+
+	err = -ENOMEM;
+	buf = kmalloc(buflen, GFP_TEMPORARY);
+	if (!buf)
+		goto out_err;
+
+	fh = buf;
+	dwords = (buflen - offsetof(struct ovl_fh, fid)) >> 2;
+	/*
+	 * We encode a non-connectable file handle for non-dir, because we
+	 * only need to find the lower inode number and we don't want to pay
+	 * the price or reconnecting the dentry.
+	 */
+	fh_type = exportfs_encode_fh(lower,
+				     (struct fid *)fh->fid,
+				     &dwords, 0);
+	fh_len = (dwords << 2) + offsetof(struct ovl_fh, fid);
+
+	err = -EOVERFLOW;
+	if (fh_len > buflen || fh_type <= 0 || fh_type == FILEID_INVALID)
+		goto out_err;
+
+	fh->version = OVL_FH_VERSION;
+	fh->magic = OVL_FH_MAGIC;
+	fh->type = fh_type;
+	fh->len = fh_len;
+	memcpy(fh->uuid, lower->d_sb->s_uuid, sizeof(fh->uuid));
+
+	err = -ENOMEM;
+	ret = kmalloc(fh_len, GFP_KERNEL);
+	if (!ret)
+		goto out_err;
+
+	memcpy(ret, buf, fh_len);
+
+	kfree(buf);
+	return ret;
+
+out_err:
+	pr_warn_ratelimited("overlay: failed to get redirect fh (%i)\n", err);
+	kfree(buf);
+	kfree(ret);
+	return ERR_PTR(err);
+}
+
+static const struct ovl_fh null_fh = {
+	.version = OVL_FH_VERSION,
+	.magic = OVL_FH_MAGIC,
+	.type = FILEID_INVALID,
+	.len = sizeof(struct ovl_fh),
+};
+
+static int ovl_set_origin(struct dentry *dentry, struct dentry *upper)
+{
+	struct path lowerpath;
+	const struct ovl_fh *fh = NULL;
+	int err;
+
+	ovl_path_lower(dentry, &lowerpath);
+	if (WARN_ON(!lowerpath.mnt))
+		return -EIO;
+
+	/*
+	 * redirect_fh is disabled if not all layers are on the same fs, so
+	 * file handles the we encode are unique across all layers.
+	 */
+	if (ovl_redirect_fh(dentry->d_sb))
+		fh = ovl_encode_fh(lowerpath.dentry);
+	/*
+	 * When redirect_fh is disabled or on failure to encode lower fh,
+	 * store an invalid 'null' fh, so we can use the overlay.origin xattr
+	 * to distignuish between a copy up and a pure upper inode.  If lower
+	 * fs does not support encoding fh, disable redirect_fh and don't try
+	 * to encode again.
+	 */
+	if (IS_ERR_OR_NULL(fh)) {
+		err = PTR_ERR(fh);
+		if (err == -EOPNOTSUPP) {
+			pr_warn("overlay: file handle not supported by lower - turning off redirect_fh\n");
+			ovl_clear_redirect_fh(dentry->d_sb);
+		}
+		fh = &null_fh;
+	}
+
+	err = ovl_do_setxattr(upper, OVL_XATTR_ORIGIN, fh, fh->len, 0);
+
+	if (fh != &null_fh)
+		kfree(fh);
+	return err;
+}
+
 static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 			      struct dentry *dentry, struct path *lowerpath,
 			      struct kstat *stat, const char *link,
@@ -316,6 +417,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (err)
 		goto out_cleanup;
 
+	/*
+	 * Store identifier of lower inode in upper inode xattr to
+	 * allow lookup of the copy up origin inode.
+	 */
+	err = ovl_set_origin(dentry, temp);
+	if (err)
+		goto out_cleanup;
+
 	if (tmpfile)
 		err = ovl_do_link(temp, udir, upper, true);
 	else
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 2ddbd44..da37aaf 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -20,6 +20,26 @@ enum ovl_path_type {
 #define OVL_XATTR_PREFIX XATTR_TRUSTED_PREFIX "overlay."
 #define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
 #define OVL_XATTR_REDIRECT OVL_XATTR_PREFIX "redirect"
+#define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
+
+/*
+ * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
+ * where:
+ * origin.fh	- exported file handle of the lower file
+ * origin.uuid	- uuid of the lower filesystem
+ */
+#define OVL_FH_VERSION	0
+#define OVL_FH_MAGIC	0xfb
+
+/* On-disk and in-memeory format for redirect by file handle */
+struct ovl_fh {
+	unsigned char version;	/* 0 */
+	unsigned char magic;	/* 0xfb */
+	unsigned char len;	/* size of this header + size of fid */
+	unsigned char type;	/* fid_type of fid */
+	unsigned char uuid[16];	/* uuid of filesystem */
+	unsigned char fid[0];	/* file identifier */
+} __packed;
 
 #define OVL_ISUPPER_MASK 1UL
 
@@ -172,6 +192,8 @@ bool ovl_redirect_dir(struct super_block *sb);
 void ovl_clear_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);
+bool ovl_redirect_fh(struct super_block *sb);
+void ovl_clear_redirect_fh(struct super_block *sb);
 void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry);
 void ovl_inode_init(struct inode *inode, struct inode *realinode,
 		    bool is_upper);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 68fa932..3abf025 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -32,6 +32,8 @@ struct ovl_fs {
 	/* sb common to all (or all lower) layers */
 	struct super_block *same_lower_sb;
 	struct super_block *same_sb;
+	/* redirect by file handle */
+	bool redirect_fh;
 };
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7d56aa8..de246a5 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -17,6 +17,7 @@
 #include <linux/statfs.h>
 #include <linux/seq_file.h>
 #include <linux/posix_acl_xattr.h>
+#include <linux/exportfs.h>
 #include "overlayfs.h"
 #include "ovl_entry.h"
 
@@ -929,6 +930,22 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ufs->upper_mnt->mnt_sb == ufs->same_lower_sb)
 		ufs->same_sb = ufs->same_lower_sb;
 
+	/*
+	 * Redirect by file handle is used to find a dentry in one of the
+	 * layers, so the handle must be unique across all layers.
+	 * Therefore, enable redirect by file handle, only if all layers are
+	 * on the same sb which supports lookup by file handles.
+	 *
+	 * XXX: We could relax this to same_lower_sb, but we currently use
+	 * redirect_fh for constant inode numbers, which require same_sb.
+	 * Also, for NFS export of overlay, it is easier if all layers are on
+	 * the same fs, because then we can export the encoded file handle
+	 * without adding a layer descriptor to it.
+	 */
+	if (ufs->same_sb && ufs->same_sb->s_export_op &&
+	    ufs->same_sb->s_export_op->fh_to_dentry)
+		ufs->redirect_fh = true;
+
 	if (remote)
 		sb->s_d_op = &ovl_reval_dentry_operations;
 	else
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index a474ab9..9db0588 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -222,6 +222,20 @@ void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect)
 	oe->redirect = redirect;
 }
 
+bool ovl_redirect_fh(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	return ofs->redirect_fh;
+}
+
+void ovl_clear_redirect_fh(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	ofs->redirect_fh = false;
+}
+
 void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
-- 
2.7.4

Re: [PATCH v4 02/15] ovl: store file handle of lower inode on copy up

[Amir Goldstein]

On Mon, May 1, 2017 at 4:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Sometimes it is interesting to know if an upper file is pure
> upper or a copy up target, and if it is a copy up target, it
> may be interesting to find the copy up origin.
>
> This will be used to preserve lower inode numbers across copy up.
>
> Store the lower inode file handle in upper inode extended attribute
> overlay.origin on copy up to use it later for these cases.
> Store the lower filesystem uuid along side the file handle, so we can
> validate that we are looking for the origin file in the original fs.
>
> On failure to encode lower file handle, store an invalid 'null' handle,
> so we can always use the overlay.origin xattr to distinguish between
> a copy up and a pure upper inode.
>
> If lower fs does not support NFS export ops or if not all layers are
> on the same fs, don't try to encode a lower file handle and store the
> 'null' handle instead.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
[...]
> @@ -316,6 +417,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>         if (err)
>                 goto out_cleanup;
>
> +       /*
> +        * Store identifier of lower inode in upper inode xattr to
> +        * allow lookup of the copy up origin inode.
> +        */
> +       err = ovl_set_origin(dentry, temp);

calls setxattr - should be inside the temp->d_inode code block

> +       if (err)
> +               goto out_cleanup;
> +

Re: [PATCH v4 02/15] ovl: store file handle of lower inode on copy up

[Amir Goldstein]

On Wed, May 3, 2017 at 6:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, May 1, 2017 at 4:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Sometimes it is interesting to know if an upper file is pure
>> upper or a copy up target, and if it is a copy up target, it
>> may be interesting to find the copy up origin.
>>
>> This will be used to preserve lower inode numbers across copy up.
>>
>> Store the lower inode file handle in upper inode extended attribute
>> overlay.origin on copy up to use it later for these cases.
>> Store the lower filesystem uuid along side the file handle, so we can
>> validate that we are looking for the origin file in the original fs.
>>
>> On failure to encode lower file handle, store an invalid 'null' handle,
>> so we can always use the overlay.origin xattr to distinguish between
>> a copy up and a pure upper inode.
>>
>> If lower fs does not support NFS export ops or if not all layers are
>> on the same fs, don't try to encode a lower file handle and store the
>> 'null' handle instead.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
> [...]
>> @@ -316,6 +417,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>         if (err)
>>                 goto out_cleanup;
>>
>> +       /*
>> +        * Store identifier of lower inode in upper inode xattr to
>> +        * allow lookup of the copy up origin inode.
>> +        */
>> +       err = ovl_set_origin(dentry, temp);
>
> calls setxattr - should be inside the temp->d_inode code block
>

No. it doesn't - sorry for the noise.

[PATCH v4 03/15] ovl: use an auxiliary var for overlay root entry

[Amir Goldstein]

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index b8b0778..d0a3e4a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -220,6 +220,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	const struct cred *old_cred;
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct ovl_entry *poe = dentry->d_parent->d_fsdata;
+	struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
 	struct path *stack = NULL;
 	struct dentry *upperdir, *upperdentry = NULL;
 	unsigned int ctr = 0;
@@ -259,7 +260,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			if (!upperredirect)
 				goto out_put_upper;
 			if (d.redirect[0] == '/')
-				poe = dentry->d_sb->s_root->d_fsdata;
+				poe = roe;
 		}
 		upperopaque = d.opaque;
 	}
@@ -290,10 +291,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (d.stop)
 			break;
 
-		if (d.redirect &&
-		    d.redirect[0] == '/' &&
-		    poe != dentry->d_sb->s_root->d_fsdata) {
-			poe = dentry->d_sb->s_root->d_fsdata;
+		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
+			poe = roe;
 
 			/* Find the current layer on the root dentry */
 			for (i = 0; i < poe->numlower; i++)
-- 
2.7.4

[PATCH v4 04/15] ovl: factor out ovl_lookup_data()

[Amir Goldstein]

Split helper ovl_lookup_data() out of ovl_lookup_single().
The helper takes care of updating the ovl_lookup_data context
according to the dentry that was found in layer.

Decorate ovl_lookup_data() with some more comments.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index d0a3e4a..e6de13d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -96,34 +96,27 @@ static bool ovl_is_opaquedir(struct dentry *dentry)
 	return false;
 }
 
-static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
-			     const char *name, unsigned int namelen,
-			     size_t prelen, const char *post,
-			     struct dentry **ret)
+/* Update ovl_lookup_data struct from dentry found in layer */
+static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
+			   size_t prelen, const char *post,
+			   struct dentry **ret)
 {
-	struct dentry *this;
 	int err;
 
-	this = lookup_one_len_unlocked(name, base, namelen);
-	if (IS_ERR(this)) {
-		err = PTR_ERR(this);
-		this = NULL;
-		if (err == -ENOENT || err == -ENAMETOOLONG)
-			goto out;
-		goto out_err;
-	}
 	if (!this->d_inode)
 		goto put_and_out;
 
+	/* Don't support traversing automounts and other weirdness */
 	if (ovl_dentry_weird(this)) {
-		/* Don't support traversing automounts and other weirdness */
 		err = -EREMOTE;
 		goto out_err;
 	}
+	/* Stop lookup in lower layers on whiteout */
 	if (ovl_is_whiteout(this)) {
 		d->stop = d->opaque = true;
 		goto put_and_out;
 	}
+	/* Stop lookup in lower layers on non-dir */
 	if (!d_can_lookup(this)) {
 		d->stop = true;
 		if (d->is_dir)
@@ -131,10 +124,15 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 		goto out;
 	}
 	d->is_dir = true;
+	/* Stop lookup in lower layers on opaque dir */
 	if (!d->last && ovl_is_opaquedir(this)) {
 		d->stop = d->opaque = true;
 		goto out;
 	}
+	/*
+	 * Check redirect dir even if d->last, because with redirect_dir,
+	 * a merge dir may have an opaque dir parent.
+	 */
 	err = ovl_check_redirect(this, d, prelen, post);
 	if (err)
 		goto out_err;
@@ -152,6 +150,25 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 	return err;
 }
 
+static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
+			     const char *name, unsigned int namelen,
+			     size_t prelen, const char *post,
+			     struct dentry **ret)
+{
+	struct dentry *this = lookup_one_len_unlocked(name, base, namelen);
+	int err;
+
+	if (IS_ERR(this)) {
+		err = PTR_ERR(this);
+		*ret = NULL;
+		if (err == -ENOENT || err == -ENAMETOOLONG)
+			return 0;
+		return err;
+	}
+
+	return ovl_lookup_data(this, d, prelen, post, ret);
+}
+
 static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 			    struct dentry **ret)
 {
-- 
2.7.4

[PATCH v4 05/15] ovl: store the file type in ovl_lookup_data

[Amir Goldstein]

d.is_dir is used to stop layers lookup when finding non-dir under
dir entry.

Instead of storing a boolean d.is_dir in ovl_lookup_data, store
the upper inode file type in d.mode and stop layers lookup on any
file type change.

The d.mode generalization is needed for lookup of non-dir copy up
origin.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e6de13d..7c2b038 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -17,7 +17,7 @@
 
 struct ovl_lookup_data {
 	struct qstr name;
-	bool is_dir;
+	umode_t mode;
 	bool opaque;
 	bool stop;
 	bool last;
@@ -101,6 +101,7 @@ static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 			   size_t prelen, const char *post,
 			   struct dentry **ret)
 {
+	mode_t mode;
 	int err;
 
 	if (!this->d_inode)
@@ -116,14 +117,18 @@ static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 		d->stop = d->opaque = true;
 		goto put_and_out;
 	}
+	/* Stop lookup in lower layers on file type change */
+	mode = this->d_inode->i_mode & S_IFMT;
+	if (d->mode && d->mode != mode) {
+		d->stop = true;
+		goto put_and_out;
+	}
+	d->mode = mode;
 	/* Stop lookup in lower layers on non-dir */
 	if (!d_can_lookup(this)) {
 		d->stop = true;
-		if (d->is_dir)
-			goto put_and_out;
 		goto out;
 	}
-	d->is_dir = true;
 	/* Stop lookup in lower layers on opaque dir */
 	if (!d->last && ovl_is_opaquedir(this)) {
 		d->stop = d->opaque = true;
@@ -249,7 +254,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	int err;
 	struct ovl_lookup_data d = {
 		.name = dentry->d_name,
-		.is_dir = false,
+		.mode = 0,
 		.opaque = false,
 		.stop = false,
 		.last = !poe->numlower,
-- 
2.7.4

[PATCH v4 06/15] ovl: pass the stack index on ovl_lookup_data

[Amir Goldstein]

This is needed for looking up copy up origin.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 7c2b038..6fabbc1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -21,6 +21,7 @@ struct ovl_lookup_data {
 	bool opaque;
 	bool stop;
 	bool last;
+	int idx;
 	char *redirect;
 };
 
@@ -258,6 +259,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.opaque = false,
 		.stop = false,
 		.last = !poe->numlower,
+		.idx = 0,
 		.redirect = NULL,
 	};
 
@@ -299,6 +301,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		struct path lowerpath = poe->lowerstack[i];
 
 		d.last = i == poe->numlower - 1;
+		d.idx = i + 1;
 		err = ovl_lookup_layer(lowerpath.dentry, &d, &this);
 		if (err)
 			goto out_put;
-- 
2.7.4

[PATCH v4 07/15] ovl: lookup copy up origin of non-dir inode

[Amir Goldstein]

When non directory upper has overlay.origin xattr, lookup in lower
layers to find the copy up origin inode.

Until this change, a non-dir dentry could have had oe->numlower == 1
with oe->lowerstack[0] pointing at the copy up origin path right after
copy up, but not when a non-dir dentry was created by ovl_lookup().

After this change, a non-dir dentry could be pointing at a lower dentry
after ovl_lookup(), which may or may not be the copy up origin.

For now, we are not doing anything with this reference, so it is not
significant if this is the actual copy up origin.  Soon, we will verify
that this is the actual copy up origin using file handles.

This is going to be used for persistent inode numbers across copy up.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6fabbc1..66072b0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -97,6 +97,13 @@ static bool ovl_is_opaquedir(struct dentry *dentry)
 	return false;
 }
 
+static bool ovl_is_copyup(struct dentry *dentry)
+{
+	int res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
+
+	return res > 0;
+}
+
 /* Update ovl_lookup_data struct from dentry found in layer */
 static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 			   size_t prelen, const char *post,
@@ -125,13 +132,16 @@ static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 		goto put_and_out;
 	}
 	d->mode = mode;
-	/* Stop lookup in lower layers on non-dir */
+	/*
+	 * Stop lookup in lower layers on opaque dir and on non-dir
+	 * which is not upper or has no copy up origin.
+	 */
 	if (!d_can_lookup(this)) {
-		d->stop = true;
-		goto out;
-	}
-	/* Stop lookup in lower layers on opaque dir */
-	if (!d->last && ovl_is_opaquedir(this)) {
+		if (d->idx > 0 || !ovl_is_copyup(this)) {
+			d->stop = true;
+			goto out;
+		}
+	} else if (!d->last && ovl_is_opaquedir(this)) {
 		d->stop = d->opaque = true;
 		goto out;
 	}
-- 
2.7.4

[PATCH v4 08/15] ovl: lookup non-dir copy up origin by file handle

[Amir Goldstein]

When redirect_fh is enabled, if overlay.origin xattr is found on a
non-dir upper inode, instead of lookup of the copy up origin in lower
layer by name, try to get it by calling exportfs_decode_fh().

On failure to lookup by file handle to lower layer or if redirect_fh is
disabled, do not lookup the copy up origin by name, because the lower
found by name could be another file in case the upper file was renamed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 176 +++++++++++++++++++++++++++++++++++++++++++++--
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  14 ++++
 3 files changed, 186 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 66072b0..695a78e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -12,6 +12,8 @@
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/ratelimit.h>
+#include <linux/mount.h>
+#include <linux/exportfs.h>
 #include "overlayfs.h"
 #include "ovl_entry.h"
 
@@ -22,7 +24,10 @@ struct ovl_lookup_data {
 	bool stop;
 	bool last;
 	int idx;
-	char *redirect;
+	bool by_path;		/* redirect by path: */
+	char *redirect;		/* - path to follow */
+	bool by_fh;		/* redirect by file handle: */
+	struct ovl_fh *fh;	/* - file handle to follow */
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
@@ -82,6 +87,51 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 	goto err_free;
 }
 
+static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
+{
+	int res;
+	void *buf = NULL;
+
+	res = vfs_getxattr(dentry, name, NULL, 0);
+	if (res <= 0) {
+		if (res == -ENODATA || res == -EOPNOTSUPP)
+			return 0;
+		goto fail;
+	}
+	buf = kzalloc(res, GFP_TEMPORARY);
+	if (!buf) {
+		res = -ENOMEM;
+		goto fail;
+	}
+
+	res = vfs_getxattr(dentry, name, buf, res);
+	if (res < 0 || !ovl_redirect_fh_ok(buf, res))
+		goto fail;
+
+	return (struct ovl_fh *)buf;
+
+err_free:
+	kfree(buf);
+	return NULL;
+fail:
+	pr_warn_ratelimited("overlayfs: failed to get %s (%i)\n",
+			    name, res);
+	goto err_free;
+}
+
+static void ovl_check_redirect_fh(struct dentry *dentry,
+				  struct ovl_lookup_data *d)
+{
+	kfree(d->fh);
+	d->fh = ovl_get_fh(dentry, OVL_XATTR_ORIGIN);
+}
+
+static void ovl_reset_redirect_fh(struct ovl_lookup_data *d)
+{
+	kfree(d->fh);
+	d->fh = NULL;
+}
+
 static bool ovl_is_opaquedir(struct dentry *dentry)
 {
 	int res;
@@ -149,9 +199,23 @@ static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 	 * Check redirect dir even if d->last, because with redirect_dir,
 	 * a merge dir may have an opaque dir parent.
 	 */
-	err = ovl_check_redirect(this, d, prelen, post);
-	if (err)
-		goto out_err;
+	if (d->by_path) {
+		err = ovl_check_redirect(this, d, prelen, post);
+		if (err)
+			goto out_err;
+	}
+	/*
+	 * If non-dir has a valid origin file handle, it will be used to
+	 * find the copy up origin in lower layers.
+	 *
+	 * Directory lookup by fh is not desired for all workloads, so it
+	 * will be enabled by a future mount option.
+	 */
+	if (d->by_fh && !d_is_dir(this)) {
+		ovl_check_redirect_fh(this, d);
+		d->stop = !d->fh;
+	}
+
 out:
 	*ret = this;
 	return 0;
@@ -225,6 +289,76 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 	return 0;
 }
 
+static struct dentry *ovl_decode_fh(struct vfsmount *mnt,
+				    const struct ovl_fh *fh,
+				    int (*acceptable)(void *, struct dentry *))
+{
+	int bytes = (fh->len - offsetof(struct ovl_fh, fid));
+
+	/*
+	 * When redirect_fh is disabled, 'invalid' file handles are stored
+	 * to indicate that this entry has been copied up.
+	 */
+	if (!bytes || (int)fh->type == FILEID_INVALID)
+		return ERR_PTR(-ESTALE);
+
+	/*
+	 * Several layers can be on the same fs and decoded dentry may be in
+	 * either one of those layers. We are looking for a match of dentry
+	 * and mnt to find out to which layer the decoded dentry belongs to.
+	 */
+	return exportfs_decode_fh(mnt, (struct fid *)fh->fid,
+				  bytes >> 2, (int)fh->type,
+				  acceptable, mnt);
+}
+
+static int ovl_acceptable(void *ctx, struct dentry *dentry)
+{
+	return 1;
+}
+
+/* Lookup by file handle in a lower layer mounted at @mnt */
+static int ovl_lookup_layer_fh(struct vfsmount *mnt, struct ovl_lookup_data *d,
+			       struct dentry **ret)
+{
+	struct dentry *this = ovl_decode_fh(mnt, d->fh, ovl_acceptable);
+	int err;
+
+	if (IS_ERR(this)) {
+		err = PTR_ERR(this);
+		*ret = NULL;
+		if (err == -ESTALE)
+			return 0;
+		return err;
+	}
+
+	/* If found by file handle - don't follow that handle again */
+	ovl_reset_redirect_fh(d);
+	return ovl_lookup_data(this, d, 0, "", ret);
+}
+
+/* Find a lower layer where file handle should be decoded */
+static int ovl_find_layer_by_fh(struct dentry *dentry, int idx,
+				struct ovl_fh *fh)
+{
+	struct super_block *same_sb = ovl_same_sb(dentry->d_sb);
+
+	/* We only support redirect_fh when all layers are on the same fs */
+	if (!same_sb)
+		return -1;
+
+	/*
+	 * Since all layers are on the same fs, we use the first layer for
+	 * decoding the file handle.  We may get a disconnected dentry,
+	 * which is fine, because we only need to hold the origin inode in
+	 * cache and use its inode number.  We may even get a connected dentry,
+	 * that is not under the first layer's root.  That is also fine for
+	 * using it's inode number - it's the same as if we held a reference
+	 * to a dentry in first layer that was moved under us.
+	 */
+	return 0;
+}
+
 /*
  * Returns next layer in stack starting from top.
  * Returns -1 if this is the last layer.
@@ -270,7 +404,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.stop = false,
 		.last = !poe->numlower,
 		.idx = 0,
+		.by_path = true,
 		.redirect = NULL,
+		.by_fh = ofs->redirect_fh,
+		.fh = NULL,
 	};
 
 	if (dentry->d_name.len > ofs->namelen)
@@ -299,7 +436,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		upperopaque = d.opaque;
 	}
 
-	if (!d.stop && poe->numlower) {
+	if (!d.stop && (poe->numlower || d.fh)) {
 		err = -ENOMEM;
 		stack = kcalloc(ofs->numlower, sizeof(struct path),
 				GFP_TEMPORARY);
@@ -307,6 +444,33 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put_upper;
 	}
 
+	/* Lookup non-dir copy up origin by file handle */
+	if (!d.stop && d.fh && !S_ISDIR(d.mode)) {
+		/* Find layer where file handle should be decoded */
+		i = ovl_find_layer_by_fh(dentry, 0, d.fh);
+		if (i < 0 || i > roe->numlower)
+			goto alloc_entry;
+
+		d.last = true;
+		d.by_path = false;
+		err = ovl_lookup_layer_fh(roe->lowerstack[i].mnt, &d, &this);
+		if (err)
+			goto out_put;
+
+		if (!this)
+			goto alloc_entry;
+
+		stack[ctr].dentry = this;
+		stack[ctr].mnt = roe->lowerstack[i].mnt;
+		ctr++;
+
+		/* Looked up by fh - do not lookup also by path */
+		goto alloc_entry;
+	}
+
+	/* Lookup lower layers by path */
+	d.by_path = true;
+	d.by_fh = false;
 	for (i = 0; !d.stop && i < poe->numlower; i++) {
 		struct path lowerpath = poe->lowerstack[i];
 
@@ -338,6 +502,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
+alloc_entry:
 	oe = ovl_alloc_entry(ctr);
 	err = -ENOMEM;
 	if (!oe)
@@ -386,6 +551,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	dput(upperdentry);
 	kfree(upperredirect);
 out:
+	kfree(d.fh);
 	kfree(d.redirect);
 	revert_creds(old_cred);
 	return ERR_PTR(err);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index da37aaf..90181e16 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -194,6 +194,7 @@ const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
 bool ovl_redirect_fh(struct super_block *sb);
 void ovl_clear_redirect_fh(struct super_block *sb);
+bool ovl_redirect_fh_ok(const char *redirect, size_t size);
 void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry);
 void ovl_inode_init(struct inode *inode, struct inode *realinode,
 		    bool is_upper);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 9db0588..08c55e6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -236,6 +236,20 @@ void ovl_clear_redirect_fh(struct super_block *sb)
 	ofs->redirect_fh = false;
 }
 
+bool ovl_redirect_fh_ok(const char *redirect, size_t size)
+{
+	struct ovl_fh *fh = (void *)redirect;
+
+	if (size < sizeof(struct ovl_fh) || size < fh->len)
+		return false;
+
+	if (fh->version > OVL_FH_VERSION ||
+	    fh->magic != OVL_FH_MAGIC)
+		return false;
+
+	return true;
+}
+
 void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
-- 
2.7.4

[PATCH v4 09/15] ovl: validate lower layer uuid on redirect by fh

[Amir Goldstein]

On copy up, we store in xattr overlay.origin the lower file handle
along with the lower fs uuid.

Before decoding the file handle in overlay.origin verify:
- All lower layers are on the same fs
- UUID of lower fs matches the stored uuid

In the future we will support finding the lower layer by uuid so
we won't have to require that all lower layers are on the same fs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c |  8 ++++++--
 fs/overlayfs/super.c | 15 +++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 695a78e..2be2917 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -343,8 +343,12 @@ static int ovl_find_layer_by_fh(struct dentry *dentry, int idx,
 {
 	struct super_block *same_sb = ovl_same_sb(dentry->d_sb);
 
-	/* We only support redirect_fh when all layers are on the same fs */
-	if (!same_sb)
+	/*
+	 * We only support redirect_fh when all layers are on the same fs.
+	 * Make sure that the stored uuid matches the uuid of the lower
+	 * layer where file handle will be decoded.
+	 */
+	if (!same_sb || memcmp(same_sb->s_uuid, fh->uuid, sizeof(fh->uuid)))
 		return -1;
 
 	/*
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index de246a5..e639750 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -18,6 +18,7 @@
 #include <linux/seq_file.h>
 #include <linux/posix_acl_xattr.h>
 #include <linux/exportfs.h>
+#include <linux/uuid.h>
 #include "overlayfs.h"
 #include "ovl_entry.h"
 
@@ -941,10 +942,20 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	 * Also, for NFS export of overlay, it is easier if all layers are on
 	 * the same fs, because then we can export the encoded file handle
 	 * without adding a layer descriptor to it.
+	 *
+	 * We use the lower fs uuid to validate that file handles are decoded
+	 * from the same fs they were encoded from, so redirect_fh requires
+	 * that the lower fs has filled a valid uuid in sb->s_uuid.
 	 */
 	if (ufs->same_sb && ufs->same_sb->s_export_op &&
-	    ufs->same_sb->s_export_op->fh_to_dentry)
-		ufs->redirect_fh = true;
+	    ufs->same_sb->s_export_op->fh_to_dentry) {
+		uuid_le *uuid = (uuid_le *) ufs->same_lower_sb->s_uuid;
+
+		if (uuid_le_cmp(*uuid, NULL_UUID_LE))
+			ufs->redirect_fh = true;
+		else
+			pr_warn("overlayfs: lower fs needs to report s_uuid.\n");
+	}
 
 	if (remote)
 		sb->s_d_op = &ovl_reval_dentry_operations;
-- 
2.7.4

[PATCH v4 10/15] ovl: constant st_ino/st_dev across copy up

[Amir Goldstein]

When all layers are on the same underlying filesystem, let stat(2)
return st_dev/st_ino values of the copy up origin inode if it is
known.

This results in constant st_ino/st_dev representation of files in an
overlay mount before and after copy up.

When the underlying filesystem support NFS exportfs, the result is also
persistent st_ino/st_dev representation before and after mount cycle.

Lower hardlinks are broken on copy up to differnt upper files, so we
cannot use the lower origin st_ino for those different files, even for
the same fs case.

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 17b8418..9c0c4e1 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -69,7 +69,32 @@ static int ovl_getattr(const struct path *path, struct kstat *stat,
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = vfs_getattr(&realpath, stat, request_mask, flags);
 	revert_creds(old_cred);
-	return err;
+	if (err)
+		return err;
+
+	/*
+	 * When all layers are on the same fs, we use st_ino of the copy up
+	 * origin, if we know it.
+	 * This guaranties constant st_dev/st_ino across copy up.
+	 *
+	 * When redirect_fh is enabled, this also guaranties persistent
+	 * st_ino/st_dev across mount cycle.
+	 */
+	if (ovl_same_sb(dentry->d_sb)) {
+		struct dentry *lower = ovl_dentry_lower(dentry);
+
+		/*
+		 * Lower hardlinks are broken on copy up to differnt upper
+		 * files, so we cannot use the lower origin st_ino for those
+		 * different files, even for the same fs case.
+		 */
+		if (lower && lower->d_inode->i_nlink == 1) {
+			stat->dev = lower->d_sb->s_dev;
+			stat->ino = lower->d_inode->i_ino;
+		}
+	}
+
+	return 0;
 }
 
 int ovl_permission(struct inode *inode, int mask)
-- 
2.7.4

[PATCH v4 11/15] ovl: persistent inode number for directories

[Amir Goldstein]

stat(2) on overlay directories reports the overlay temp inode
number, which is constant across copy up, but is not persistent.

When all layers are on the same fs, report the copy up origin inode
number for directories.

This inode number is persistent, unique across the overlay mount and
constant across copy up.

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

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index bfabc65..17aa007 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -154,8 +154,24 @@ static int ovl_dir_getattr(const struct path *path, struct kstat *stat,
 	if (err)
 		return err;
 
+	/*
+	 * Always use the overlay st_dev for directories, so 'find -xdev' will
+	 * scan the entire overlay mount and won't cross the overlay mount
+	 * boundaries.
+	 */
 	stat->dev = dentry->d_sb->s_dev;
-	stat->ino = dentry->d_inode->i_ino;
+	/*
+	 * When all layers are not on the same fs, the pair real st_ino and
+	 * overlay st_dev is not unique, so use the non persistent overlay
+	 * st_ino.
+	 *
+	 * When all layers are on the same fs, use the copy up origin st_ino,
+	 * which is persistent, unique and constant across copy up.
+	 */
+	if (!ovl_same_sb(dentry->d_sb))
+		stat->ino = dentry->d_inode->i_ino;
+	else if (OVL_TYPE_UPPER(type) && OVL_TYPE_MERGE(type))
+		stat->ino = ovl_dentry_lower(dentry)->d_inode->i_ino;
 
 	/*
 	 * It's probably not worth it to count subdirs to get the
-- 
2.7.4

[PATCH v4 12/15] ovl: fix du --one-file-system on overlay mount

[Amir Goldstein]

Overlay directory inodes report overlay st_dev to stat(2).
Overlay non-dir inodes report real st_dev and real st_ino to stat(2).

Due to the different st_dev values for dir and non-dir inodes, when
executing the command du -x on an overlay mount, the result is wrong,
because non-dirs are not accounted for in the overlay disk usage.

The reason that the overlay st_dev is not used for non-dir is because
the tuple overlay st_dev and real st_ino is not unique when overlay
layers are not on the same fs.

When all overlay layers are on the same fs, that tuple is unique, so
use overlay st_dev for non-dirs to get the correct result from du -x.

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 9c0c4e1..ec5230f 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -73,8 +73,10 @@ static int ovl_getattr(const struct path *path, struct kstat *stat,
 		return err;
 
 	/*
-	 * When all layers are on the same fs, we use st_ino of the copy up
-	 * origin, if we know it.
+	 * When all layers are on the same fs, all real inode number are
+	 * unique, so we use the overlay st_dev, which is friendly to du -x.
+	 *
+	 * We also use st_ino of the copy up origin, if we know it.
 	 * This guaranties constant st_dev/st_ino across copy up.
 	 *
 	 * When redirect_fh is enabled, this also guaranties persistent
@@ -83,15 +85,14 @@ static int ovl_getattr(const struct path *path, struct kstat *stat,
 	if (ovl_same_sb(dentry->d_sb)) {
 		struct dentry *lower = ovl_dentry_lower(dentry);
 
+		stat->dev = dentry->d_sb->s_dev;
 		/*
 		 * Lower hardlinks are broken on copy up to differnt upper
 		 * files, so we cannot use the lower origin st_ino for those
 		 * different files, even for the same fs case.
 		 */
-		if (lower && lower->d_inode->i_nlink == 1) {
-			stat->dev = lower->d_sb->s_dev;
+		if (lower && lower->d_inode->i_nlink == 1)
 			stat->ino = lower->d_inode->i_ino;
-		}
 	}
 
 	return 0;
-- 
2.7.4

[PATCH v4 13/15] ovl: persistent inode numbers for upper hardlinks

[Amir Goldstein]

An upper type non directory dentry that is a copy up target
should have a reference to its lower copy up origin.

There are three ways for an upper type dentry to be instantiated:
1. A lower type dentry that is being copied up
2. An entry that is found in upper dir by ovl_lookup()
3. A negative dentry is hardlinked to an upper type dentry

In the first case, the lower reference is set before copy up.
In the second case, the lower reference is found by ovl_lookup().
In the last case of hardlinked upper dentry, it is not easy to
update the lower reference of the negative dentry.  Instead,
drop the newly hardlinked negative dentry from dcache and let
the next access call ovl_lookup() to find its lower reference.

This makes sure that the inode number reported by stat(2) after
the hardlink is created is the same inode number that will be
reported by stat(2) after mount cycle, which is the inode number
of the lower copy up origin of the hardlink source.

NOTE that this does not fix breaking of lower hardlinks on copy
up, but only fixes the case of lower nlink == 1, whose upper copy
up inode is hardlinked in upper dir.

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

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 17aa007..d275fa3 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -198,6 +198,9 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
 		inc_nlink(inode);
 	}
 	d_instantiate(dentry, inode);
+	/* Force lookup of new upper hardlink to find its lower */
+	if (hardlink)
+		d_drop(dentry);
 }
 
 static bool ovl_type_merge(struct dentry *dentry)
-- 
2.7.4

[PATCH v4 14/15] ovl: update documentation w.r.t. constant inode numbers

[Amir Goldstein]

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index 634d03e..c9e884b 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -21,12 +21,19 @@ from accessing the corresponding object from the original filesystem.
 This is most obvious from the 'st_dev' field returned by stat(2).
 
 While directories will report an st_dev from the overlay-filesystem,
-all non-directory objects will report an st_dev from the lower or
+non-directory objects may report an st_dev from the lower filesystem or
 upper filesystem that is providing the object.  Similarly st_ino will
 only be unique when combined with st_dev, and both of these can change
 over the lifetime of a non-directory object.  Many applications and
 tools ignore these values and will not be affected.
 
+In the special case of all overlay layers on the same underlying
+filesystem, all objects will report an st_dev from the overlay
+filesystem and st_ino from the underlying filesystem.  This will
+make the overlay mount more compliant with filesystem scanners and
+overlay objects will be distinguishable from the corresponding
+objects in the original filesystem.
+
 Upper and Lower
 ---------------
 
-- 
2.7.4

[PATCH v4 15/15] ovl: add support for verify_lower option

[Amir Goldstein]

When overlayfs is mounted with option 'verify_lower', a directory inode
found in lower layer by name or by redirect_dir is verified against the
file handle of the copy up origin that is stored in the upper layer.

The 'verify_lower' option should not be used after copying layers,
because the new lower directory inodes would fail verification.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/namei.c     | 71 +++++++++++++++++++++++++++++++++++++++++++-----
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 23 ++++++++++++++++
 fs/overlayfs/util.c      |  7 +++++
 6 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 767ae77..0aa626a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -234,7 +234,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
-static struct ovl_fh *ovl_encode_fh(struct dentry *lower)
+struct ovl_fh *ovl_encode_fh(struct dentry *lower)
 {
 	struct ovl_fh *fh;
 	int fh_type, fh_len, dwords;
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 2be2917..4580ac0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -27,7 +27,8 @@ struct ovl_lookup_data {
 	bool by_path;		/* redirect by path: */
 	char *redirect;		/* - path to follow */
 	bool by_fh;		/* redirect by file handle: */
-	struct ovl_fh *fh;	/* - file handle to follow */
+	bool verify_fh;		/* verify by file handle: */
+	struct ovl_fh *fh;	/* - file handle to follow/verify */
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
@@ -206,14 +207,14 @@ static int ovl_lookup_data(struct dentry *this, struct ovl_lookup_data *d,
 	}
 	/*
 	 * If non-dir has a valid origin file handle, it will be used to
-	 * find the copy up origin in lower layers.
-	 *
-	 * Directory lookup by fh is not desired for all workloads, so it
-	 * will be enabled by a future mount option.
+	 * find the copy up origin in lower layers.  If verify_lower is
+	 * enabled a directory origin file handle will be used to verify
+	 * lower directory that was found by path.
 	 */
-	if (d->by_fh && !d_is_dir(this)) {
+	if (d->by_fh && (d->verify_fh || !d_is_dir(this))) {
 		ovl_check_redirect_fh(this, d);
-		d->stop = !d->fh;
+		if (!d_is_dir(this) && !d->fh)
+			d->stop = true;
 	}
 
 out:
@@ -364,6 +365,57 @@ static int ovl_find_layer_by_fh(struct dentry *dentry, int idx,
 }
 
 /*
+ * Verify that a lower directory matches the stored file handle.
+ * Return 0 on match, > 0 on mismatch, < 0 on error.
+ */
+static int ovl_verify_lower_fh(struct dentry **lower,
+			       struct ovl_lookup_data *d)
+{
+	struct ovl_fh *fh;
+	struct inode *inode;
+	int ret;
+
+	/* We should be called only to verify lower dir matches fh */
+	if (WARN_ON(!d->fh) || !S_ISDIR(d->mode))
+		return -EIO;
+
+	/* We currently support verify_lower for single lower layer */
+	if (WARN_ON(!d->last))
+		return -EIO;
+
+	/* If we have a copy up origin, we should have found a lower dir */
+	if (!*lower) {
+		pr_warn_ratelimited("overlayfs: failed to find lower dir\n");
+		return -ENOENT;
+	}
+
+	fh = ovl_encode_fh(*lower);
+	if (IS_ERR(fh)) {
+		ret = PTR_ERR(fh);
+		fh = NULL;
+		goto fail;
+	} else if (fh->len != d->fh->len || memcmp(fh, d->fh, fh->len)) {
+		ret = fh->len;
+		goto fail;
+	}
+
+	ret = 0;
+out:
+	/* Don't verify that handle again */
+	ovl_reset_redirect_fh(d);
+	kfree(fh);
+	return ret;
+
+fail:
+	inode = d_inode(*lower);
+	pr_warn_ratelimited("overlayfs: failed to verify lower dir (ino=%lu, ret=%i) - were layers copied?\n",
+			    inode ? inode->i_ino : 0, ret);
+	dput(*lower);
+	*lower = NULL;
+	goto out;
+}
+
+/*
  * Returns next layer in stack starting from top.
  * Returns -1 if this is the last layer.
  */
@@ -411,6 +463,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.by_path = true,
 		.redirect = NULL,
 		.by_fh = ofs->redirect_fh,
+		.verify_fh = ofs->config.verify_lower,
 		.fh = NULL,
 	};
 
@@ -484,6 +537,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (err)
 			goto out_put;
 
+		/* Verify that lower matches the copy up origin fh */
+		if (d.verify_fh && d.fh && ovl_verify_lower_fh(&this, &d))
+			break;
+
 		if (!this)
 			continue;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 90181e16..d9ff028a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -190,6 +190,7 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
 bool ovl_redirect_dir(struct super_block *sb);
 void ovl_clear_redirect_dir(struct super_block *sb);
+bool ovl_verify_lower(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
 bool ovl_redirect_fh(struct super_block *sb);
@@ -263,3 +264,4 @@ int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_copy_xattr(struct dentry *old, struct dentry *new);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
+struct ovl_fh *ovl_encode_fh(struct dentry *lower);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 3abf025..0e26af2 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -14,6 +14,7 @@ struct ovl_config {
 	char *workdir;
 	bool default_permissions;
 	bool redirect_dir;
+	bool verify_lower;
 };
 
 /* private information held for overlayfs's superblock */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e639750..a7c03ca 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -280,6 +280,7 @@ enum {
 	OPT_DEFAULT_PERMISSIONS,
 	OPT_REDIRECT_DIR_ON,
 	OPT_REDIRECT_DIR_OFF,
+	OPT_VERIFY_LOWER,
 	OPT_ERR,
 };
 
@@ -290,6 +291,7 @@ static const match_table_t ovl_tokens = {
 	{OPT_DEFAULT_PERMISSIONS,	"default_permissions"},
 	{OPT_REDIRECT_DIR_ON,		"redirect_dir=on"},
 	{OPT_REDIRECT_DIR_OFF,		"redirect_dir=off"},
+	{OPT_VERIFY_LOWER,		"verify_lower"},
 	{OPT_ERR,			NULL}
 };
 
@@ -362,6 +364,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->redirect_dir = false;
 			break;
 
+		case OPT_VERIFY_LOWER:
+			config->verify_lower = true;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -957,6 +963,23 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			pr_warn("overlayfs: lower fs needs to report s_uuid.\n");
 	}
 
+	/*
+	 * The verify_lower feature is used to verify that lower directory
+	 * found by path matches the stored copy up origin.  Currently, only
+	 * single lower layer on same fs as upper layer is supported.
+	 */
+	if (ufs->config.verify_lower) {
+		ufs->config.verify_lower = false;
+		if (!ufs->same_sb)
+			pr_warn("overlayfs: option \"verify_lower\" requires lower/upper on same fs.\n");
+		if (numlower > 1)
+			pr_warn("overlayfs: option \"verify_lower\" requires single lower layer.\n");
+		else if (!ufs->redirect_fh)
+			pr_warn("overlayfs: option \"verify_lower\" not supported by lower fs.\n");
+		else
+			ufs->config.verify_lower = true;
+	}
+
 	if (remote)
 		sb->s_d_op = &ovl_reval_dentry_operations;
 	else
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 08c55e6..dad924d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -207,6 +207,13 @@ void ovl_clear_redirect_dir(struct super_block *sb)
 	ofs->config.redirect_dir = false;
 }
 
+bool ovl_verify_lower(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	return ofs->config.verify_lower;
+}
+
 const char *ovl_dentry_get_redirect(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
-- 
2.7.4

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Mon, May 1, 2017 at 3:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This is v4 series of redirect by file handle and constant inode numbers.
> this series fixes constant inode numbers for stat(2) for overlayfs
> configuration of all layers on the same fs.

Great work.

I did some mutilation (aka. simplification) and pushed the result to
overlayfs-constino.v5

No time to check if it works or not, so probably not...  Will do
tomorrow, but if you have time, please feel free to test and review my
changes.

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Wed, May 3, 2017 at 6:43 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, May 1, 2017 at 3:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Miklos,
>>
>> This is v4 series of redirect by file handle and constant inode numbers.
>> this series fixes constant inode numbers for stat(2) for overlayfs
>> configuration of all layers on the same fs.
>
> Great work.
>
> I did some mutilation (aka. simplification) and pushed the result to
> overlayfs-constino.v5
>
> No time to check if it works or not, so probably not...  Will do
> tomorrow, but if you have time, please feel free to test and review my
> changes.
>

I will review and test tonight.
Thanks!

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Wed, May 3, 2017 at 6:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, May 3, 2017 at 6:43 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Mon, May 1, 2017 at 3:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Miklos,
>>>
>>> This is v4 series of redirect by file handle and constant inode numbers.
>>> this series fixes constant inode numbers for stat(2) for overlayfs
>>> configuration of all layers on the same fs.
>>
>> Great work.
>>
>> I did some mutilation (aka. simplification) and pushed the result to
>> overlayfs-constino.v5
>>

Simplified indeed!

>> No time to check if it works or not, so probably not...

Oh but it does :-)
Passed on my tests.
I pushed branch master of unionmunt-tests for testing constino,
so you can use that if you like,
because my overlayfs-devel branch already plays with verify_lower.

Anyway, I see that you relaxed the uuid_is_null() constrain, so now
you can also test ./run --ov=0 --samefs
with (default) tmpfs, although that is not really stress testing fh decoding..

>> tomorrow, but if you have time, please feel free to test and review my
>> changes.
>>
>

Reviewed.

Specifically:
ACK for simplifications of "store file handle of lower inode on copy up"
ACK for simplifications of "lookup non-dir copy-up-origin by file handle"

One bug here:

+       if (IS_ERR(origin)) {
+               if (origin == ERR_PTR(-ESTALE))
+                       origin = NULL;
+       }
+       if (ovl_dentry_weird(origin) ||

Should be else if.
xfstest overlay/019 (fsstress) hits NULL pointer deref at ovl_dentry_weird()

+           ((d_inode(origin)->i_mode ^ d_inode(dentry)->i_mode) & S_IFMT)) {
+               dput(origin);
+               origin = NULL;
+               goto invalid;
+       }
+


Thanks!
Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Wed, May 3, 2017 at 10:01 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, May 3, 2017 at 6:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, May 3, 2017 at 6:43 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Mon, May 1, 2017 at 3:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Miklos,
>>>>
>>>> This is v4 series of redirect by file handle and constant inode numbers.
>>>> this series fixes constant inode numbers for stat(2) for overlayfs
>>>> configuration of all layers on the same fs.
>>>
>>> Great work.
>>>
>>> I did some mutilation (aka. simplification) and pushed the result to
>>> overlayfs-constino.v5
>>>
>
> Simplified indeed!
>
>>> No time to check if it works or not, so probably not...
>
> Oh but it does :-)
> Passed on my tests.
> I pushed branch master of unionmunt-tests for testing constino,
> so you can use that if you like,
> because my overlayfs-devel branch already plays with verify_lower.
>
> Anyway, I see that you relaxed the uuid_is_null() constrain, so now

Wasn't intentional; will restore it...

> you can also test ./run --ov=0 --samefs
> with (default) tmpfs, although that is not really stress testing fh decoding..

Lets just add s_uuid to tmpfs, should be trivial.

>
>>> tomorrow, but if you have time, please feel free to test and review my
>>> changes.
>>>
>>
>
> Reviewed.

Thanks.

>
> Specifically:
> ACK for simplifications of "store file handle of lower inode on copy up"
> ACK for simplifications of "lookup non-dir copy-up-origin by file handle"
>
> One bug here:
>
> +       if (IS_ERR(origin)) {
> +               if (origin == ERR_PTR(-ESTALE))
> +                       origin = NULL;
> +       }
> +       if (ovl_dentry_weird(origin) ||
>
> Should be else if.
> xfstest overlay/019 (fsstress) hits NULL pointer deref at ovl_dentry_weird()
>
> +           ((d_inode(origin)->i_mode ^ d_inode(dentry)->i_mode) & S_IFMT)) {
> +               dput(origin);
> +               origin = NULL;
> +               goto invalid;
> +       }
> +

Right, will fix.

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

One more little detail: file handle byte order.  We store the fh in a
filesystem, which means it can be used by systems with different
endianness, which basically invalidates it. This scenario is highly
unlikely to happen in practice, so for simplicity perhaps we should
just store a bit into the origin structure indicating the byte order
used to create the file handle and verify it when using it.

Thoughts?

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Thu, May 4, 2017 at 12:15 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> One more little detail: file handle byte order.  We store the fh in a
> filesystem, which means it can be used by systems with different
> endianness, which basically invalidates it. This scenario is highly
> unlikely to happen in practice, so for simplicity perhaps we should
> just store a bit into the origin structure indicating the byte order
> used to create the file handle and verify it when using it.
>
> Thoughts?
>

Yes, just noticed this wrinkle myself.

Ideal world though:
file handles are exported over the network and should have been in
network order.
What if NFS server goes dead and a hot backup with different endianess
takes over? (even less likely than our use case).
Theoretically, filesystems could still be fixed to export (i.e.
ino/generation) in
network order, but that's not very practical for the question at hand.

Practical thought:
Instead of storing endieness bit in every single ovl_fh,
Store this handle at upper root overlay.origin:


#define OVL_ROOT_INO                2

static const struct ovl_root_fh {
       struct ovl_fh hdr;
       ino_t ino;
} root_fh = {
    .hdr = {
       .version = OVL_FH_VERSION,
       .magic = OVL_FH_MAGIC,
       .type = FILEID_ROOT,
       .len = sizeof(struct ovl_fh) + sizeof(ino_t),
    },
    .ino = OVL_ROOT_INO,
};

I realize that storing an integer overlay.one is 'simpler',
but I'm withdrawn to (my idea of) elegance...

Also, if you agree to check cross-endianess mount once at mount time,
then you should probably also restore the ofs->redirect_fh boolean, so
you can turn redirect_fh off at mount time... Don't you have to do that
anyway for the uuid_is_null() check?

Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Thu, May 4, 2017 at 1:18 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, May 4, 2017 at 12:15 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> One more little detail: file handle byte order.  We store the fh in a
>> filesystem, which means it can be used by systems with different
>> endianness, which basically invalidates it. This scenario is highly
>> unlikely to happen in practice, so for simplicity perhaps we should
>> just store a bit into the origin structure indicating the byte order
>> used to create the file handle and verify it when using it.
>>
>> Thoughts?
>>
>
> Yes, just noticed this wrinkle myself.
>
> Ideal world though:
> file handles are exported over the network and should have been in
> network order.
> What if NFS server goes dead and a hot backup with different endianess
> takes over? (even less likely than our use case).
> Theoretically, filesystems could still be fixed to export (i.e.
> ino/generation) in
> network order, but that's not very practical for the question at hand.
>
> Practical thought:
> Instead of storing endieness bit in every single ovl_fh,
> Store this handle at upper root overlay.origin:
>
>
> #define OVL_ROOT_INO                2
>
> static const struct ovl_root_fh {
>        struct ovl_fh hdr;
>        ino_t ino;
> } root_fh = {
>     .hdr = {
>        .version = OVL_FH_VERSION,
>        .magic = OVL_FH_MAGIC,
>        .type = FILEID_ROOT,
>        .len = sizeof(struct ovl_fh) + sizeof(ino_t),
>     },
>     .ino = OVL_ROOT_INO,
> };
>
> I realize that storing an integer overlay.one is 'simpler',
> but I'm withdrawn to (my idea of) elegance...
>

If you do like the idea, let me know if you want me to send a patch on
top of v5.

Do you plan to push v5 (+ "else if" bugfix) to overlayfs-next?

If we go for my solution to endianess, we can add that patch
as a fix patch (same goes for null uuid).

Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Thu, May 4, 2017 at 1:59 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> If you do like the idea, let me know if you want me to send a patch on
> top of v5.
>
> Do you plan to push v5 (+ "else if" bugfix) to overlayfs-next?
>
> If we go for my solution to endianess, we can add that patch
> as a fix patch (same goes for null uuid).

I don't think doing the checks once per mount gets us much.  Bytes are
cheap, checks are cheap, why add complexity?

Pushed .v6 with these fixes.  Takes care of future fixes to file
handle byte order by allowing CPU independent byte order as well.

I'd like to push to overlayfs-next if it appears to work and passes tests.

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Thu, May 4, 2017 at 3:10 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, May 4, 2017 at 1:59 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> If you do like the idea, let me know if you want me to send a patch on
>> top of v5.
>>
>> Do you plan to push v5 (+ "else if" bugfix) to overlayfs-next?
>>
>> If we go for my solution to endianess, we can add that patch
>> as a fix patch (same goes for null uuid).
>
> I don't think doing the checks once per mount gets us much.  Bytes are
> cheap, checks are cheap, why add complexity?
>

I should ask myslef that question more often...

> Pushed .v6 with these fixes.  Takes care of future fixes to file
> handle byte order by allowing CPU independent byte order as well.
>

One minor review comment.

I used uuid_le_cmp(*uuid, NULL_UUID_LE) arbitrarily in my original patch,
but if we want to stick to semantic sb->s_uuid is probably more accurately
described as uuid_be, because filesystems most likely copy it in raw format
from disk.

This is purely semantic of course, but if you think that matters, may as well
replace uuid_le with uuid_be.

> I'd like to push to overlayfs-next if it appears to work and passes tests.
>

Reviewed and passed all tests.

Cheers,
Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Thu, May 4, 2017 at 4:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> One minor review comment.
>
> I used uuid_le_cmp(*uuid, NULL_UUID_LE) arbitrarily in my original patch,
> but if we want to stick to semantic sb->s_uuid is probably more accurately
> described as uuid_be, because filesystems most likely copy it in raw format
> from disk.
>
> This is purely semantic of course, but if you think that matters, may as well
> replace uuid_le with uuid_be.

Okay.

More changes:

  - use vfs_getattr() to get the lower inode number instead of
lower->d_inode->i_ino.  For one i_ino is 32bit on 32bit archs, while
kstat.ino is always 64bit

  - merge the dir and non-dir getattr, they look very similar now

Pushed to overlayfs-constino.v7

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Fri, May 5, 2017 at 12:03 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, May 4, 2017 at 4:14 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> One minor review comment.
>>
>> I used uuid_le_cmp(*uuid, NULL_UUID_LE) arbitrarily in my original patch,
>> but if we want to stick to semantic sb->s_uuid is probably more accurately
>> described as uuid_be, because filesystems most likely copy it in raw format
>> from disk.
>>
>> This is purely semantic of course, but if you think that matters, may as well
>> replace uuid_le with uuid_be.
>
> Okay.
>
> More changes:
>
>   - use vfs_getattr() to get the lower inode number instead of
> lower->d_inode->i_ino.  For one i_ino is 32bit on 32bit archs, while
> kstat.ino is always 64bit
>
>   - merge the dir and non-dir getattr, they look very similar now

Nice, see suggestion below for 'improvement'.

You also forgot to mention in changes since v6:

   - store 'null' fh instead of 'invalid' fh

Reviewed and tested v7.

Regarding ovl_getattr(), it is a good example for code that could make
use of OVL_TYPE_COPYUP() for better readability.

OVL_TYPE_COPYUP() was originally meant to mark also dentries
with null origin fh, but it is useful IMO also to mark dentries for which
we hold an origin (and maybe grow the original meaning later).

Attached a patch to add OVL_TYPE_COPYUP() with diminished
interpretation, which also makes use of it in ovl_getattr().
If you take that patch, you probably want to apply it before the
ovl_getattr() change though.

BTW, I tested with that patch.

Cheers,
Amir.

[-- Attachment #2: v7-0001-ovl-set-the-COPYUP-type-flag-for-non-dirs.patch --]
[-- Type: text/x-patch, Size: 2712 bytes --]

From cb0b6dacef3cf3b1c65459d174db2392095da9fa Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Sun, 23 Apr 2017 23:12:34 +0300
Subject: [PATCH v7] ovl: set the COPYUP type flag for non-dirs

For directory entries, non zero oe->numlower implies OVL_TYPE_MERGE.
Define a new type flag OVL_TYPE_COPYUP to indicate that an entry holds
a reference to its lower copy up origin.

For directory entries COPYUP := MERGE && UPPER. For non-dir entries
COPYUP means that a lower type dentry has been recently copied up or
that we were able to find the copy up origin from overlay.fh xattr.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     |  5 ++---
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/util.c      | 10 ++++++----
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index fa202cf..1195a2c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -84,12 +84,11 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	 * persistent st_ino across mount cycle.
 	 */
 	if (ovl_same_sb(dentry->d_sb)) {
-		ovl_path_lower(dentry, &realpath);
-
-		if (OVL_TYPE_UPPER(type) && realpath.dentry) {
+		if (OVL_TYPE_COPYUP(type)) {
 			struct kstat lowerstat;
 			u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
 
+			ovl_path_lower(dentry, &realpath);
 			err = vfs_getattr(&realpath, &lowerstat,
 					  lowermask, flags);
 			if (err)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f3dd5f3..d3f2ee8 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -13,10 +13,12 @@
 enum ovl_path_type {
 	__OVL_PATH_UPPER	= (1 << 0),
 	__OVL_PATH_MERGE	= (1 << 1),
+	__OVL_PATH_COPYUP	= (1 << 2),
 };
 
 #define OVL_TYPE_UPPER(type)	((type) & __OVL_PATH_UPPER)
 #define OVL_TYPE_MERGE(type)	((type) & __OVL_PATH_MERGE)
+#define OVL_TYPE_COPYUP(type)	((type) & __OVL_PATH_COPYUP)
 
 #define OVL_XATTR_PREFIX XATTR_TRUSTED_PREFIX "overlay."
 #define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 4a7e5c8..fd4f78c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -83,11 +83,13 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 		type = __OVL_PATH_UPPER;
 
 		/*
-		 * Non-dir dentry can hold lower dentry from previous
-		 * location.
+		 * Non-dir dentry can hold lower dentry of its copy up origin.
 		 */
-		if (oe->numlower && d_is_dir(dentry))
-			type |= __OVL_PATH_MERGE;
+		if (oe->numlower) {
+			type |= __OVL_PATH_COPYUP;
+			if (d_is_dir(dentry))
+				type |= __OVL_PATH_MERGE;
+		}
 	} else {
 		if (oe->numlower > 1)
 			type |= __OVL_PATH_MERGE;
-- 
2.7.4

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Fri, May 5, 2017 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>
> You also forgot to mention in changes since v6:
>
>    - store 'null' fh instead of 'invalid' fh

And w.r.t. that change, the following comment in ovl_get_origin()
is a bit confusing now, because the comment referring to
'invalid' file handles was removed from the function.

/* Treat empty origin as "invalid" */

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Fri, May 5, 2017 at 9:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 5, 2017 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> You also forgot to mention in changes since v6:
>>
>>    - store 'null' fh instead of 'invalid' fh
>
> And w.r.t. that change, the following comment in ovl_get_origin()
> is a bit confusing now, because the comment referring to
> 'invalid' file handles was removed from the function.
>
> /* Treat empty origin as "invalid" */

Okay, with cosmetic fixes pushed to overlayfs-next.

I renamed OVL_TYPE_COPYUP to OVL_TYPE_ORIGIN, because, like you said,
the meaning is slightly different.  COPYUP means it has been copied
up, ORIGIN means we have a dentry pointing to the origin.

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Fri, May 5, 2017 at 12:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, May 5, 2017 at 9:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, May 5, 2017 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>
>>> You also forgot to mention in changes since v6:
>>>
>>>    - store 'null' fh instead of 'invalid' fh
>>
>> And w.r.t. that change, the following comment in ovl_get_origin()
>> is a bit confusing now, because the comment referring to
>> 'invalid' file handles was removed from the function.
>>
>> /* Treat empty origin as "invalid" */
>
> Okay, with cosmetic fixes pushed to overlayfs-next.

Looks good.

>
> I renamed OVL_TYPE_COPYUP to OVL_TYPE_ORIGIN, because, like you said,
> the meaning is slightly different.  COPYUP means it has been copied
> up, ORIGIN means we have a dentry pointing to the origin.
>

Ack.

Thanks,
Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Fri, May 5, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 5, 2017 at 12:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, May 5, 2017 at 9:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Fri, May 5, 2017 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>> You also forgot to mention in changes since v6:
>>>>
>>>>    - store 'null' fh instead of 'invalid' fh
>>>
>>> And w.r.t. that change, the following comment in ovl_get_origin()
>>> is a bit confusing now, because the comment referring to
>>> 'invalid' file handles was removed from the function.
>>>
>>> /* Treat empty origin as "invalid" */
>>
>> Okay, with cosmetic fixes pushed to overlayfs-next.
>
> Looks good.
>

Miklos,

FYI, I have implemented verify_lower mount option:
https://github.com/amir73il/linux/commits/ovl-verify-lower
and tested it below overlay snapshots tests, so for what its worth,
'store file handle' and 'lookup file handle' from overlayfs-next
(merged to current master) have now been also exercised with lots
of lower changes and mount cycles.

Is it too soon to nudge you about the pull request? ;-)

Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Miklos Szeredi]

On Wed, May 10, 2017 at 10:58 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 5, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, May 5, 2017 at 12:53 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Fri, May 5, 2017 at 9:55 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Fri, May 5, 2017 at 10:25 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>
>>>>> You also forgot to mention in changes since v6:
>>>>>
>>>>>    - store 'null' fh instead of 'invalid' fh
>>>>
>>>> And w.r.t. that change, the following comment in ovl_get_origin()
>>>> is a bit confusing now, because the comment referring to
>>>> 'invalid' file handles was removed from the function.
>>>>
>>>> /* Treat empty origin as "invalid" */
>>>
>>> Okay, with cosmetic fixes pushed to overlayfs-next.
>>
>> Looks good.
>>
>
> Miklos,
>
> FYI, I have implemented verify_lower mount option:
> https://github.com/amir73il/linux/commits/ovl-verify-lower
> and tested it below overlay snapshots tests, so for what its worth,
> 'store file handle' and 'lookup file handle' from overlayfs-next
> (merged to current master) have now been also exercised with lots
> of lower changes and mount cycles.
>
> Is it too soon to nudge you about the pull request? ;-)

I'll have a look.

My plan is to send a pull request to Linus for the const ino stuff,
and then leave the rest to 4.13.

This is the list I have in my head for what's missing:

- lookup origin dir for snapshots
- const ino for non-samefs
- correct d_ino for copied up entries
- NFS export support
- hardlink unbreaking
- sharing pages for reflink (*)
- ro/rw correctness for samefs with temp reflink (**)
- sharing pages in the general case (*)

(*) very preliminary design
(**) need to check overhead: I have a feeling that it's a heavyweight
solution for a tiny problem

The non-starred ones don't seem too hard and should aim for 4.13.

Thanks,
Miklos

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Wed, May 10, 2017 at 12:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, May 10, 2017 at 10:58 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> Is it too soon to nudge you about the pull request? ;-)
>
> I'll have a look.
>
> My plan is to send a pull request to Linus for the const ino stuff,
> and then leave the rest to 4.13.
>

Sure, no rush about verify_lower, I just meant to nudge about the pull
request for const ino.

> This is the list I have in my head for what's missing:
>
> - lookup origin dir for snapshots
> - const ino for non-samefs
> - correct d_ino for copied up entries
> - NFS export support
> - hardlink unbreaking
> - sharing pages for reflink (*)
> - ro/rw correctness for samefs with temp reflink (**)
> - sharing pages in the general case (*)
>

Just updated TODO with the a similar list yesterday ;)
https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO

> (*) very preliminary design
> (**) need to check overhead: I have a feeling that it's a heavyweight
> solution for a tiny problem
>

I am hoping to reduce the code complexity (of rocopyup) after
we have the workdir/inodes/ index, because taking care of
rocopyup/rwcopyup races can be similar to taking care of
link/unlink/link races... we'll see how it goes.

The read pages performance overhead should be reasonable with
sharing pages.

The inode create/delete overhead is something we will need to measure,
but from my experience with xfs, I suspect that the cost is going to be
fair enough that some people would like to pay it to get the "tiny" problem
solved.

Also, the "tiny" problem is also going to be manifested with hardlinks
copy up, where it may need to be fixed anyway.
see this commit message for the details:
https://github.com/amir73il/linux/commit/62a16f3389187d3c3efa22293bf65cdb7bd619c5



> The non-starred ones don't seem too hard and should aim for 4.13.
>

Indeed. I am going to start with hardlinks and const d_ino soon.
The case of non-dir is easier to handle first because:
1. workdir/inodes/#lower_ino is a hard link to upper,
    so fewer follow by file handles involved
2. const d_ino for directory is not very interesting -
    'find' always uses stat() to get inode number of directory,
    so 'find -ino N' is not affected by non-const dir d_ino.

Cheers,
Amir.

Re: [PATCH v4 00/15] overlayfs constant inode numbers

[Amir Goldstein]

On Wed, May 10, 2017 at 12:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
[...]
>
> My plan is to send a pull request to Linus for the const ino stuff,
> and then leave the rest to 4.13.
>
> This is the list I have in my head for what's missing:
>
> - lookup origin dir for snapshots
> - const ino for non-samefs
> - correct d_ino for copied up entries
> - NFS export support
> - hardlink unbreaking
[...]
>
> The non-starred ones don't seem too hard and should aim for 4.13.
>

Just to clarify, did you mean that we should aim for NFS export and hardlinks
for 4.13 for samefs? or non-samefs?
Because I can see how non-samefs is possible, but complicates the reverse
inode map, so I rather start with samefs.

It turns out that constant d_ino wasn't that hard.
It's quite an isolated patch based on your old overlay.ino POC, see:
https://github.com/amir73il/linux/tree/ovl-constino

Basically, it's working and I also have xfstests to verify it,
which I will post those and the patch later.

I think that the toll on readdir performance is not such a big problem,
because stat'ing the dir entries is served from cache, so 'ls -lR' is
not going to suffer, only 'find -ino' with cold cache will suffer.

There is still room for optimization for iterating 'very pure upper' dirs
(with no copy ups in them).

Amir.