[PATCH 0/3] Enable new features for more overlayfs setups

[PATCH 0/3] Enable new features for more overlayfs setups

[Amir Goldstein]

Miklos,

The following series enables recent overlayfs features for the
case of lower with no uuid and in particular a nested overlay.
Several posts [1] [2] suggest that both squashfs as lower fs
and nested overlay are used by OpenWrt users.

Turns out it is rather easy to enable xino, index and nfs_export
for some common setups.

Persistent and consistent inode numbers for lower squashfs were
tested with unionmount test:
 run --ov --squashfs --verify

Tests for xino and NFS export with nested overlay are available
in my xfstests tree [3]. I did not test NFS export with lower squashfs -
leaving that to interesed OpenWrt users. Intereseted party can
use my ovl-nested [4] branch for testing.

I am not sure how many users need these features for nested overlay,
but nested overlay with index is a pre-requisite for multiple overlayfs
snapshots support.

Note that patch [3/3] is an optimization that is not required for
passing the nested exportfs xfstest tests, but could be useful for
real lower (under nested) fs with large file handle sizes.

Thanks,
Amir.

[1] https://lkml.org/lkml/2017/12/13/220
[2] https://www.spinics.net/lists/linux-unionfs/msg01682.html
[3] https://github.com/amir73il/xfstests/commits/ovl-nested
[4] https://github.com/amir73il/linux/commits/ovl-nested

Amir Goldstein (3):
  ovl: relax requirement for non null uuid of lower fs
  ovl: disable xino for some nested overlay cases
  ovl: compact nested ovl_fh

 fs/overlayfs/copy_up.c   | 20 +++++++++++++++-
 fs/overlayfs/export.c    | 15 ++++++++++--
 fs/overlayfs/namei.c     | 34 +++++++++++++++++++--------
 fs/overlayfs/overlayfs.h | 16 ++++++++++---
 fs/overlayfs/super.c     | 50 ++++++++++++++++++++++++++++++++++++----
 fs/overlayfs/util.c      | 14 ++++++++---
 6 files changed, 126 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH 1/3] ovl: relax requirement for non null uuid of lower fs

[Amir Goldstein]

We use uuid to associate an overlay lower file handle with a lower layer,
so we can accept lower fs with null uuid as long as all lower layers with
null uuid are on the same fs.

This change allows enabling index and nfs_export features for the setup of
single lower fs of type squashfs - squashfs supports file handles, but has
a null uuid. This change also allows enabling index and nfs_export
features for nested overlayfs, where the lower overlay has nfs_export
enabled.

Enabling the index feature with single lower squashfs fixes the
unionmount-testsuite test:
  ./run --ov --squashfs --verify

As a by-product, if, like the lower squashfs, upper fs also uses the
generic export_encode_fh() implementation to export 32bit inode file
handles (e.g. ext4), then the xino_auto config/module/mount option will
enable unique overlay inode numbers.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 23 +++++++++++++++++++++--
 fs/overlayfs/util.c  |  3 +--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 2e0fc93c2c06..069e441e24eb 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1174,11 +1174,14 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
 }
 
 /* Get a unique fsid for the layer */
-static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
+static int ovl_get_fsid(struct ovl_fs *ofs, struct vfsmount *mnt)
 {
+	struct super_block *sb = mnt->mnt_sb;
 	unsigned int i;
 	dev_t dev;
 	int err;
+	bool need_uuid = ofs->config.nfs_export ||
+			 (ofs->config.index && ofs->upper_mnt);
 
 	/* fsid 0 is reserved for upper fs even with non upper overlay */
 	if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
@@ -1187,6 +1190,22 @@ static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
 	for (i = 0; i < ofs->numlowerfs; i++) {
 		if (ofs->lower_fs[i].sb == sb)
 			return i + 1;
+
+		/*
+		 * We use uuid to associate an overlay lower file handle with a
+		 * lower layer, so we can accept lower fs with null uuid as long
+		 * as all lower layers with null uuid are on the same fs.
+		 */
+		if (need_uuid &&
+		    uuid_equal(&ofs->lower_fs[i].sb->s_uuid, &sb->s_uuid)) {
+			need_uuid = false;
+			ofs->config.index = false;
+			ofs->config.nfs_export = false;
+			pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
+				uuid_is_null(&sb->s_uuid) ? "null" :
+							    "conflicting",
+				mnt->mnt_root);
+		}
 	}
 
 	err = get_anon_bdev(&dev);
@@ -1223,7 +1242,7 @@ static int ovl_get_lower_layers(struct ovl_fs *ofs, struct path *stack,
 		struct vfsmount *mnt;
 		int fsid;
 
-		err = fsid = ovl_get_fsid(ofs, stack[i].mnt->mnt_sb);
+		err = fsid = ovl_get_fsid(ofs, stack[i].mnt);
 		if (err < 0)
 			goto out;
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 8cfb62cc8672..6eabe7cf9652 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -65,8 +65,7 @@ struct super_block *ovl_same_sb(struct super_block *sb)
  */
 int ovl_can_decode_fh(struct super_block *sb)
 {
-	if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry ||
-	    uuid_is_null(&sb->s_uuid))
+	if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry)
 		return 0;
 
 	return sb->s_export_op->encode_fh ? -1 : FILEID_INO32_GEN;
-- 
2.17.1

[PATCH 2/3] ovl: disable xino for some nested overlay cases

[Amir Goldstein]

There are three possible cases when overlayfs is used as lower
layer for a nested overlay mount:

1. lower overlay is non-samefs with xino enabled
2. lower overlay is non-samefs with xino disabled
3. lower overlay is samefs

In the first case, lower layer uses high inode number bits, so they are
not available for the nested overlay and xino should be disabled.

In the second case, inode numbers of lower layer are not in a single
address space, so there is no use enabling xino in nested overlay.

In the last case (samefs) the lower layer inode number address space
is that of the underlying real fs, so we can assign fsid of the lower
layer according the the real underlying fs.

In the private case of all lower overlay layers on the same fs, which is
also the upper fs of the nested overlay, the nested overlay itself is
treated as "samefs", because inode numbers in all layers are from the
same address space.

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

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f61839e1054c..d32fe09a222f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -418,3 +418,11 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 
 /* export.c */
 extern const struct export_operations ovl_export_operations;
+
+/* super.c */
+extern struct file_system_type ovl_fs_type;
+
+static inline bool ovl_is_overlay_fs(struct super_block *sb)
+{
+	return sb->s_type == &ovl_fs_type;
+}
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 069e441e24eb..b6a0211dc75c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -755,6 +755,7 @@ static int ovl_check_namelen(struct path *path, struct ovl_fs *ofs,
 static int ovl_lower_dir(const char *name, struct path *path,
 			 struct ovl_fs *ofs, int *stack_depth, bool *remote)
 {
+	struct super_block *real_sb;
 	int fh_type;
 	int err;
 
@@ -775,7 +776,8 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	 * The inodes index feature and NFS export need to encode and decode
 	 * file handles, so they require that all layers support them.
 	 */
-	fh_type = ovl_can_decode_fh(path->dentry->d_sb);
+	real_sb = path->dentry->d_sb;
+	fh_type = ovl_can_decode_fh(real_sb);
 	if ((ofs->config.nfs_export ||
 	     (ofs->config.index && ofs->config.upperdir)) && !fh_type) {
 		ofs->config.index = false;
@@ -784,6 +786,17 @@ static int ovl_lower_dir(const char *name, struct path *path,
 			name);
 	}
 
+	/* Are all layers of nested overlay on same sb? */
+	if (ofs->config.xino != OVL_XINO_OFF && ovl_is_overlay_fs(real_sb)) {
+		real_sb = ovl_same_sb(real_sb);
+		if (real_sb) {
+			fh_type = ovl_can_decode_fh(real_sb);
+		} else {
+			pr_warn("overlayfs: nested xino not supported, falling back to xino=off.\n");
+			ofs->config.xino = OVL_XINO_OFF;
+		}
+	}
+
 	/* Check if lower fs has 32bit inode numbers */
 	if (fh_type != FILEID_INO32_GEN)
 		ofs->xino_bits = 0;
@@ -1183,6 +1196,14 @@ static int ovl_get_fsid(struct ovl_fs *ofs, struct vfsmount *mnt)
 	bool need_uuid = ofs->config.nfs_export ||
 			 (ofs->config.index && ofs->upper_mnt);
 
+	/* Are all layers of nested overlay on same sb? */
+	if (ovl_is_overlay_fs(sb)) {
+		struct super_block *real_sb = ovl_same_sb(sb);
+
+		if (real_sb)
+			sb = real_sb;
+	}
+
 	/* fsid 0 is reserved for upper fs even with non upper overlay */
 	if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
 		return 0;
@@ -1535,7 +1556,7 @@ static struct dentry *ovl_mount(struct file_system_type *fs_type, int flags,
 	return mount_nodev(fs_type, flags, raw_data, ovl_fill_super);
 }
 
-static struct file_system_type ovl_fs_type = {
+struct file_system_type ovl_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "overlay",
 	.mount		= ovl_mount,
-- 
2.17.1

[PATCH 3/3] ovl: compact nested ovl_fh

[Amir Goldstein]

When encoding a file handle from lower nested overlayfs, prepending
another ovl_fh header for nested overlay file handle adds no new
information for decoding.  Instead, we just set a 'nested' flag in ovl_fh
header for nested file handle, so we know to distinguish between real
upper file handle that should be decoded from real upper layer and
nested upper file handle that should be decoded from nested lower layer.
For the maximum allowed overlay nesting depth of 1, one bit is enough.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 20 +++++++++++++++++++-
 fs/overlayfs/export.c    | 15 +++++++++++++--
 fs/overlayfs/namei.c     | 34 +++++++++++++++++++++++++---------
 fs/overlayfs/overlayfs.h |  8 +++++---
 fs/overlayfs/super.c     |  6 +++---
 fs/overlayfs/util.c      | 11 ++++++++++-
 6 files changed, 75 insertions(+), 19 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 296037afecdb..8330a982b3ca 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -254,6 +254,24 @@ struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper)
 	    WARN_ON(fh_type == FILEID_INVALID))
 		goto out;
 
+	/*
+	 * Prepending another ovl_fh header for nested overlay file handle adds
+	 * no new information for decoding. If we got a valid ovl_fh from lower
+	 * overlay, pass it up to nested overlay with the 'nested' flag.
+	 */
+	fh = ERR_PTR(-EINVAL);
+	if (ovl_is_overlay_fs(real->d_sb)) {
+		fh = buf;
+		if (WARN_ON(is_upper) ||
+		    WARN_ON(fh_type != OVL_FILEID) ||
+		    WARN_ON(fh->flags & OVL_FH_FLAG_PATH_NESTED) ||
+		    WARN_ON(ovl_check_fh_len(buf, buflen)))
+			goto out;
+
+		fh->flags |= OVL_FH_FLAG_PATH_NESTED;
+		return fh;
+	}
+
 	BUILD_BUG_ON(MAX_HANDLE_SZ + offsetof(struct ovl_fh, fid) > 255);
 	fh_len = offsetof(struct ovl_fh, fid) + buflen;
 	fh = kmalloc(fh_len, GFP_KERNEL);
@@ -294,7 +312,7 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 	 * so we can use the overlay.origin xattr to distignuish between a copy
 	 * up and a pure upper inode.
 	 */
-	if (ovl_can_decode_fh(lower->d_sb)) {
+	if (ovl_can_decode_real_fh(lower->d_sb)) {
 		fh = ovl_encode_real_fh(lower, false);
 		if (IS_ERR(fh))
 			return PTR_ERR(fh);
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 8fa37cd7818a..90411c221519 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -691,7 +691,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
 	if (!ofs->upper_mnt)
 		return ERR_PTR(-EACCES);
 
-	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true, false);
 	if (IS_ERR_OR_NULL(upper))
 		return upper;
 
@@ -788,6 +788,7 @@ static struct dentry *ovl_fh_to_dentry(struct super_block *sb, struct fid *fid,
 	struct ovl_fh *fh = (struct ovl_fh *) fid;
 	int len = fh_len << 2;
 	unsigned int flags = 0;
+	bool nested;
 	int err;
 
 	err = -EINVAL;
@@ -798,8 +799,18 @@ static struct dentry *ovl_fh_to_dentry(struct super_block *sb, struct fid *fid,
 	if (err)
 		goto out_err;
 
+	/*
+	 * Do not try to decode nested upper from upper_mnt. Decode nested file
+	 * handle only from lower overlayfs.
+	 */
 	flags = fh->flags;
-	dentry = (flags & OVL_FH_FLAG_PATH_UPPER) ?
+	nested = flags & OVL_FH_FLAG_PATH_NESTED;
+
+	err = -ESTALE;
+	if (nested && !ovl_is_overlay_fs(ovl_dentry_lower(sb->s_root)->d_sb))
+		goto out_err;
+
+	dentry = ((flags & OVL_FH_FLAG_PATH_UPPER) && !nested) ?
 		 ovl_upper_fh_to_d(sb, fh) :
 		 ovl_lower_fh_to_d(sb, fh);
 	err = PTR_ERR(dentry);
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index f28711846dd6..8d605fc16557 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -151,21 +151,33 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 }
 
 struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
-				  bool connected)
+				  bool connected, bool nested)
 {
 	struct dentry *real;
-	int bytes;
+	int bytes, type;
+	void *fid;
 
 	/*
 	 * Make sure that the stored uuid matches the uuid of the lower
-	 * layer where file handle will be decoded.
+	 * layer where file handle will be decoded. Pass nested ovl_fh to
+	 * lower overlay that will match uuid to its own layers.
 	 */
-	if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
+	if (ovl_is_overlay_fs(mnt->mnt_sb)) {
+		if (!nested)
+			return NULL;
+		fid = fh;
+		type = OVL_FILEID;
+		bytes = fh->len;
+		fh->flags &= ~OVL_FH_FLAG_PATH_NESTED;
+	} else if (uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid)) {
+		fid = fh->fid;
+		type = fh->type;
+		bytes = (fh->len - offsetof(struct ovl_fh, fid));
+	} else {
 		return NULL;
+	}
 
-	bytes = (fh->len - offsetof(struct ovl_fh, fid));
-	real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
-				  bytes >> 2, (int)fh->type,
+	real = exportfs_decode_fh(mnt, fid, (bytes + 3) >> 2, type,
 				  connected ? ovl_acceptable : NULL, mnt);
 	if (IS_ERR(real)) {
 		/*
@@ -319,10 +331,11 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 {
 	struct dentry *origin = NULL;
 	int i;
+	u8 nested = fh->flags & OVL_FH_FLAG_PATH_NESTED;
 
 	for (i = 0; i < ofs->numlower; i++) {
 		origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
-					    connected);
+					    connected, nested);
 		if (origin)
 			break;
 	}
@@ -347,6 +360,9 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 		.layer = &ofs->lower_layers[i]
 	};
 
+	/* Restore nested flag - ovl_lower_fh_to_d() is not done with fh */
+	fh->flags |= nested;
+
 	return 0;
 
 invalid:
@@ -456,7 +472,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
 	if (IS_ERR_OR_NULL(fh))
 		return ERR_CAST(fh);
 
-	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true, false);
 	kfree(fh);
 
 	if (IS_ERR_OR_NULL(upper))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d32fe09a222f..0664a0ca3423 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -62,9 +62,11 @@ enum ovl_entry_flag {
 #define OVL_FH_FLAG_ANY_ENDIAN	(1 << 1)
 /* Is the real inode encoded in fid an upper inode? */
 #define OVL_FH_FLAG_PATH_UPPER	(1 << 2)
+/* File handle from nested (lower) overlayfs */
+#define OVL_FH_FLAG_PATH_NESTED	(1 << 3)
 
 #define OVL_FH_FLAG_ALL (OVL_FH_FLAG_BIG_ENDIAN | OVL_FH_FLAG_ANY_ENDIAN | \
-			 OVL_FH_FLAG_PATH_UPPER)
+			 OVL_FH_FLAG_PATH_UPPER | OVL_FH_FLAG_PATH_NESTED)
 
 #if defined(__LITTLE_ENDIAN)
 #define OVL_FH_FLAG_CPU_ENDIAN 0
@@ -209,7 +211,7 @@ 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_sb(struct super_block *sb);
-int ovl_can_decode_fh(struct super_block *sb);
+int ovl_can_decode_real_fh(struct super_block *sb);
 struct dentry *ovl_indexdir(struct super_block *sb);
 bool ovl_index_all(struct super_block *sb);
 bool ovl_verify_lower(struct super_block *sb);
@@ -294,7 +296,7 @@ static inline unsigned int ovl_xino_bits(struct super_block *sb)
 /* namei.c */
 int ovl_check_fh_len(struct ovl_fh *fh, int fh_len);
 struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
-				  bool connected);
+				  bool connected, bool nested);
 int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 			struct dentry *upperdentry, struct ovl_path **stackp);
 int ovl_verify_set_fh(struct dentry *dentry, const char *name,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b6a0211dc75c..60f33705fe4e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -777,7 +777,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	 * file handles, so they require that all layers support them.
 	 */
 	real_sb = path->dentry->d_sb;
-	fh_type = ovl_can_decode_fh(real_sb);
+	fh_type = ovl_can_decode_real_fh(real_sb);
 	if ((ofs->config.nfs_export ||
 	     (ofs->config.index && ofs->config.upperdir)) && !fh_type) {
 		ofs->config.index = false;
@@ -790,7 +790,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	if (ofs->config.xino != OVL_XINO_OFF && ovl_is_overlay_fs(real_sb)) {
 		real_sb = ovl_same_sb(real_sb);
 		if (real_sb) {
-			fh_type = ovl_can_decode_fh(real_sb);
+			fh_type = ovl_can_decode_real_fh(real_sb);
 		} else {
 			pr_warn("overlayfs: nested xino not supported, falling back to xino=off.\n");
 			ofs->config.xino = OVL_XINO_OFF;
@@ -1075,7 +1075,7 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	}
 
 	/* Check if upper/work fs supports file handles */
-	fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
+	fh_type = ovl_can_decode_real_fh(ofs->workdir->d_sb);
 	if (ofs->config.index && !fh_type) {
 		ofs->config.index = false;
 		pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off.\n");
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 6eabe7cf9652..7f81c771bc63 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -63,11 +63,20 @@ struct super_block *ovl_same_sb(struct super_block *sb)
  * Return 1 (FILEID_INO32_GEN) if fs uses the default 32bit inode encoding.
  * Return -1 if fs uses a non default encoding with unknown inode size.
  */
-int ovl_can_decode_fh(struct super_block *sb)
+int ovl_can_decode_real_fh(struct super_block *sb)
 {
 	if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry)
 		return 0;
 
+	/*
+	 * In case FILESYSTEM_MAX_STACK_DEPTH ever grows, we only support
+	 * one level of nesting in ovl_fh. This test should be optimized out
+	 * at build time.
+	 */
+	if (FILESYSTEM_MAX_STACK_DEPTH > 2 &&
+	    ovl_is_overlay_fs(sb) && sb->s_stack_depth > 1)
+		return 0;
+
 	return sb->s_export_op->encode_fh ? -1 : FILEID_INO32_GEN;
 }
 
-- 
2.17.1

Re: [PATCH 1/3] ovl: relax requirement for non null uuid of lower fs

[Miklos Szeredi]

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

On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> We use uuid to associate an overlay lower file handle with a lower layer,
> so we can accept lower fs with null uuid as long as all lower layers with
> null uuid are on the same fs.
>
> This change allows enabling index and nfs_export features for the setup of
> single lower fs of type squashfs - squashfs supports file handles, but has
> a null uuid. This change also allows enabling index and nfs_export
> features for nested overlayfs, where the lower overlay has nfs_export
> enabled.

Shouldn't overlayfs generate an uuid for the nested case?  Not quite
sure what's the best way to do that, though...

>
> Enabling the index feature with single lower squashfs fixes the
> unionmount-testsuite test:
>   ./run --ov --squashfs --verify
>
> As a by-product, if, like the lower squashfs, upper fs also uses the
> generic export_encode_fh() implementation to export 32bit inode file
> handles (e.g. ext4), then the xino_auto config/module/mount option will
> enable unique overlay inode numbers.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 23 +++++++++++++++++++++--
>  fs/overlayfs/util.c  |  3 +--
>  2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 2e0fc93c2c06..069e441e24eb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1174,11 +1174,14 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
>  }
>
>  /* Get a unique fsid for the layer */
> -static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
> +static int ovl_get_fsid(struct ovl_fs *ofs, struct vfsmount *mnt)
>  {
> +       struct super_block *sb = mnt->mnt_sb;
>         unsigned int i;
>         dev_t dev;
>         int err;
> +       bool need_uuid = ofs->config.nfs_export ||
> +                        (ofs->config.index && ofs->upper_mnt);
>
>         /* fsid 0 is reserved for upper fs even with non upper overlay */
>         if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> @@ -1187,6 +1190,22 @@ static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
>         for (i = 0; i < ofs->numlowerfs; i++) {
>                 if (ofs->lower_fs[i].sb == sb)
>                         return i + 1;
> +
> +               /*
> +                * We use uuid to associate an overlay lower file handle with a
> +                * lower layer, so we can accept lower fs with null uuid as long
> +                * as all lower layers with null uuid are on the same fs.
> +                */
> +               if (need_uuid &&
> +                   uuid_equal(&ofs->lower_fs[i].sb->s_uuid, &sb->s_uuid)) {
> +                       need_uuid = false;
> +                       ofs->config.index = false;
> +                       ofs->config.nfs_export = false;
> +                       pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> +                               uuid_is_null(&sb->s_uuid) ? "null" :
> +                                                           "conflicting",
> +                               mnt->mnt_root);

mnt->mnt_root is wrong here: this is path->mnt resolved from the
lowerdir, not the one cloned from that path.

Fixed so ovl_get_lower_layers() passes in the path, not just the mount.

Also made this a separate loop from the one finding a matching fs, as
it's logically a different step and not needed in the case when the fs
is found.

Updated but untested patch attached.

Thanks,
Miklos

[-- Attachment #2: ovl-relax-requirement-for-non-null-uuid-of-lower-fs.patch --]
[-- Type: text/x-patch, Size: 3262 bytes --]

From: Amir Goldstein <amir73il@gmail.com>
Subject: ovl: relax requirement for non null uuid of lower fs
Date: Mon,  3 Sep 2018 09:12:09 +0300

We use uuid to associate an overlay lower file handle with a lower layer,
so we can accept lower fs with null uuid as long as all lower layers with
null uuid are on the same fs.

This change allows enabling index and nfs_export features for the setup of
single lower fs of type squashfs - squashfs supports file handles, but has
a null uuid. This change also allows enabling index and nfs_export
features for nested overlayfs, where the lower overlay has nfs_export
enabled.

Enabling the index feature with single lower squashfs fixes the
unionmount-testsuite test:
  ./run --ov --squashfs --verify

As a by-product, if, like the lower squashfs, upper fs also uses the
generic export_encode_fh() implementation to export 32bit inode file
handles (e.g. ext4), then the xino_auto config/module/mount option will
enable unique overlay inode numbers.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c |   32 ++++++++++++++++++++++++++++++--
 fs/overlayfs/util.c  |    3 +--
 2 files changed, 31 insertions(+), 4 deletions(-)

--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1175,9 +1175,29 @@ static int ovl_get_indexdir(struct ovl_f
 	return err;
 }
 
+static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
+{
+	unsigned int i;
+
+	if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
+		return true;
+
+	for (i = 0; i < ofs->numlowerfs; i++) {
+		/*
+		 * We use uuid to associate an overlay lower file handle with a
+		 * lower layer, so we can accept lower fs with null uuid as long
+		 * as all lower layers with null uuid are on the same fs.
+		 */
+		if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
+			return false;
+	}
+	return true;
+}
+
 /* Get a unique fsid for the layer */
-static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
+static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 {
+	struct super_block *sb = path->mnt->mnt_sb;
 	unsigned int i;
 	dev_t dev;
 	int err;
@@ -1191,6 +1211,14 @@ static int ovl_get_fsid(struct ovl_fs *o
 			return i + 1;
 	}
 
+	if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
+		ofs->config.index = false;
+		ofs->config.nfs_export = false;
+		pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
+			uuid_is_null(&sb->s_uuid) ? "null" : "conflicting",
+			path->dentry);
+	}
+
 	err = get_anon_bdev(&dev);
 	if (err) {
 		pr_err("overlayfs: failed to get anonymous bdev for lowerpath\n");
@@ -1225,7 +1253,7 @@ static int ovl_get_lower_layers(struct o
 		struct vfsmount *mnt;
 		int fsid;
 
-		err = fsid = ovl_get_fsid(ofs, stack[i].mnt->mnt_sb);
+		err = fsid = ovl_get_fsid(ofs, &stack[i]);
 		if (err < 0)
 			goto out;
 
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -65,8 +65,7 @@ struct super_block *ovl_same_sb(struct s
  */
 int ovl_can_decode_fh(struct super_block *sb)
 {
-	if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry ||
-	    uuid_is_null(&sb->s_uuid))
+	if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry)
 		return 0;
 
 	return sb->s_export_op->encode_fh ? -1 : FILEID_INO32_GEN;

Re: [PATCH 2/3] ovl: disable xino for some nested overlay cases

[Miklos Szeredi]

On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> There are three possible cases when overlayfs is used as lower
> layer for a nested overlay mount:
>
> 1. lower overlay is non-samefs with xino enabled
> 2. lower overlay is non-samefs with xino disabled
> 3. lower overlay is samefs
>
> In the first case, lower layer uses high inode number bits, so they are
> not available for the nested overlay and xino should be disabled.
>
> In the second case, inode numbers of lower layer are not in a single
> address space, so there is no use enabling xino in nested overlay.
>
> In the last case (samefs) the lower layer inode number address space
> is that of the underlying real fs, so we can assign fsid of the lower
> layer according the the real underlying fs.
>
> In the private case of all lower overlay layers on the same fs, which is
> also the upper fs of the nested overlay, the nested overlay itself is
> treated as "samefs", because inode numbers in all layers are from the
> same address space.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/overlayfs.h |  8 ++++++++
>  fs/overlayfs/super.c     | 25 +++++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index f61839e1054c..d32fe09a222f 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -418,3 +418,11 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>
>  /* export.c */
>  extern const struct export_operations ovl_export_operations;
> +
> +/* super.c */
> +extern struct file_system_type ovl_fs_type;
> +
> +static inline bool ovl_is_overlay_fs(struct super_block *sb)
> +{
> +       return sb->s_type == &ovl_fs_type;
> +}

Special casing like that is, well, ugly.

Generic solution (which would work in the face of other stacking fs)
would be to have a "ino-space-id" in the super block that would be
assigned to a unique number for all distinct filesystems, but stacking
fs would be able to reset it to the origin filesystem's id if
appropriate.

What do you think about that?

Thanks,
Miklos

Re: [PATCH 3/3] ovl: compact nested ovl_fh

[Miklos Szeredi]

On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> When encoding a file handle from lower nested overlayfs, prepending
> another ovl_fh header for nested overlay file handle adds no new
> information for decoding.  Instead, we just set a 'nested' flag in ovl_fh
> header for nested file handle, so we know to distinguish between real
> upper file handle that should be decoded from real upper layer and
> nested upper file handle that should be decoded from nested lower layer.
> For the maximum allowed overlay nesting depth of 1, one bit is enough.

This is an optimization, right?

What are the user-visible advantages that would justify the added complexity?

Thanks,
Miklos

Re: [PATCH 2/3] ovl: disable xino for some nested overlay cases

[Amir Goldstein]

On Wed, Oct 24, 2018 at 5:30 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > There are three possible cases when overlayfs is used as lower
> > layer for a nested overlay mount:
> >
> > 1. lower overlay is non-samefs with xino enabled
> > 2. lower overlay is non-samefs with xino disabled
> > 3. lower overlay is samefs
> >
> > In the first case, lower layer uses high inode number bits, so they are
> > not available for the nested overlay and xino should be disabled.
> >
> > In the second case, inode numbers of lower layer are not in a single
> > address space, so there is no use enabling xino in nested overlay.
> >
> > In the last case (samefs) the lower layer inode number address space
> > is that of the underlying real fs, so we can assign fsid of the lower
> > layer according the the real underlying fs.
> >
> > In the private case of all lower overlay layers on the same fs, which is
> > also the upper fs of the nested overlay, the nested overlay itself is
> > treated as "samefs", because inode numbers in all layers are from the
> > same address space.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/overlayfs.h |  8 ++++++++
> >  fs/overlayfs/super.c     | 25 +++++++++++++++++++++++--
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index f61839e1054c..d32fe09a222f 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -418,3 +418,11 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> >
> >  /* export.c */
> >  extern const struct export_operations ovl_export_operations;
> > +
> > +/* super.c */
> > +extern struct file_system_type ovl_fs_type;
> > +
> > +static inline bool ovl_is_overlay_fs(struct super_block *sb)
> > +{
> > +       return sb->s_type == &ovl_fs_type;
> > +}
>
> Special casing like that is, well, ugly.
>
> Generic solution (which would work in the face of other stacking fs)
> would be to have a "ino-space-id" in the super block that would be
> assigned to a unique number for all distinct filesystems, but stacking
> fs would be able to reset it to the origin filesystem's id if
> appropriate.
>
> What do you think about that?
>

I either did not understand your suggestion or have not explained the
purpose of this patch properly.

We wanted to have s_maxinobits, but xfs refused to commit to its
current effective maxinobits. So we gave the power to the users
(who allegedly studied their undelying filesystem ino address space) to tell
overlayfs using the xino=on option that underlying filesystem does NOT use
the highest ino bits.

Generally, we have no way of knowing if the user research was wrong, expect
for one particular case - upderlying fs is overlayfs and underlying overlayfs
has xino enabled.

*This* is why I have the "special casing" here. I am not interested to know
if this is a stacked filesystem.

So maybe what we can do is declare s_maxinobits.
Have some filesystems commit to s_maxinobits = 32 instead of the encode_fh
trick. I filesystems leave s_maxinobits 0 then user is allowed to
force xino = on.
Overlayfs with xino = on will set s_maxinobits to max of all layers s_maxinobits
+ the bits needed to identify the layer.
If overlay doesn't know the underlying maxinobits, it will used the highest bits
for layer id and set s_maxinobits = 64.

Does that make sense?

Thanks,
Amir.

Re: [PATCH 2/3] ovl: disable xino for some nested overlay cases

[Miklos Szeredi]

On Wed, Oct 24, 2018 at 5:03 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Oct 24, 2018 at 5:30 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > There are three possible cases when overlayfs is used as lower
>> > layer for a nested overlay mount:
>> >
>> > 1. lower overlay is non-samefs with xino enabled
>> > 2. lower overlay is non-samefs with xino disabled
>> > 3. lower overlay is samefs
>> >
>> > In the first case, lower layer uses high inode number bits, so they are
>> > not available for the nested overlay and xino should be disabled.
>> >
>> > In the second case, inode numbers of lower layer are not in a single
>> > address space, so there is no use enabling xino in nested overlay.
>> >
>> > In the last case (samefs) the lower layer inode number address space
>> > is that of the underlying real fs, so we can assign fsid of the lower
>> > layer according the the real underlying fs.
>> >
>> > In the private case of all lower overlay layers on the same fs, which is
>> > also the upper fs of the nested overlay, the nested overlay itself is
>> > treated as "samefs", because inode numbers in all layers are from the
>> > same address space.
>> >
>> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> > ---
>> >  fs/overlayfs/overlayfs.h |  8 ++++++++
>> >  fs/overlayfs/super.c     | 25 +++++++++++++++++++++++--
>> >  2 files changed, 31 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> > index f61839e1054c..d32fe09a222f 100644
>> > --- a/fs/overlayfs/overlayfs.h
>> > +++ b/fs/overlayfs/overlayfs.h
>> > @@ -418,3 +418,11 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>> >
>> >  /* export.c */
>> >  extern const struct export_operations ovl_export_operations;
>> > +
>> > +/* super.c */
>> > +extern struct file_system_type ovl_fs_type;
>> > +
>> > +static inline bool ovl_is_overlay_fs(struct super_block *sb)
>> > +{
>> > +       return sb->s_type == &ovl_fs_type;
>> > +}
>>
>> Special casing like that is, well, ugly.
>>
>> Generic solution (which would work in the face of other stacking fs)
>> would be to have a "ino-space-id" in the super block that would be
>> assigned to a unique number for all distinct filesystems, but stacking
>> fs would be able to reset it to the origin filesystem's id if
>> appropriate.
>>
>> What do you think about that?
>>
>
> I either did not understand your suggestion or have not explained the
> purpose of this patch properly.
>
> We wanted to have s_maxinobits, but xfs refused to commit to its
> current effective maxinobits. So we gave the power to the users
> (who allegedly studied their undelying filesystem ino address space) to tell
> overlayfs using the xino=on option that underlying filesystem does NOT use
> the highest ino bits.
>
> Generally, we have no way of knowing if the user research was wrong, expect
> for one particular case - upderlying fs is overlayfs and underlying overlayfs
> has xino enabled.
>
> *This* is why I have the "special casing" here. I am not interested to know
> if this is a stacked filesystem.
>
> So maybe what we can do is declare s_maxinobits.
> Have some filesystems commit to s_maxinobits = 32 instead of the encode_fh
> trick. I filesystems leave s_maxinobits 0 then user is allowed to
> force xino = on.
> Overlayfs with xino = on will set s_maxinobits to max of all layers s_maxinobits
> + the bits needed to identify the layer.
> If overlay doesn't know the underlying maxinobits, it will used the highest bits
> for layer id and set s_maxinobits = 64.
>
> Does that make sense?

Certainly.

I was thinking of the special case of all - real underlying layers of
nested and outer overlay - being on the same fs.  I thought this patch
was about using the samefs info from the nested overlay to allow using
the samefs logics for the outer overlay as well.

Thanks,
Miklos

Re: [PATCH 1/3] ovl: relax requirement for non null uuid of lower fs

[Amir Goldstein]

On Wed, Oct 24, 2018 at 5:16 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > We use uuid to associate an overlay lower file handle with a lower layer,
> > so we can accept lower fs with null uuid as long as all lower layers with
> > null uuid are on the same fs.
> >
> > This change allows enabling index and nfs_export features for the setup of
> > single lower fs of type squashfs - squashfs supports file handles, but has
> > a null uuid. This change also allows enabling index and nfs_export
> > features for nested overlayfs, where the lower overlay has nfs_export
> > enabled.
>
> Shouldn't overlayfs generate an uuid for the nested case?  Not quite
> sure what's the best way to do that, though...
>

They way I chose to handle that is patch 3/3.
Overlay does not generate a uuid for the nested case, but it encodes file
handles with uuid of nested underlying, so for example, when verifying
lower origin on mount we don't know for sure that last mount use exact
same overlay configuration, but we do know if used the same upper root
dir (because root dir is a special case that encodes an upper file handle).

> >
> > Enabling the index feature with single lower squashfs fixes the
> > unionmount-testsuite test:
> >   ./run --ov --squashfs --verify
> >
> > As a by-product, if, like the lower squashfs, upper fs also uses the
> > generic export_encode_fh() implementation to export 32bit inode file
> > handles (e.g. ext4), then the xino_auto config/module/mount option will
> > enable unique overlay inode numbers.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/super.c | 23 +++++++++++++++++++++--
> >  fs/overlayfs/util.c  |  3 +--
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 2e0fc93c2c06..069e441e24eb 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1174,11 +1174,14 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
> >  }
> >
> >  /* Get a unique fsid for the layer */
> > -static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
> > +static int ovl_get_fsid(struct ovl_fs *ofs, struct vfsmount *mnt)
> >  {
> > +       struct super_block *sb = mnt->mnt_sb;
> >         unsigned int i;
> >         dev_t dev;
> >         int err;
> > +       bool need_uuid = ofs->config.nfs_export ||
> > +                        (ofs->config.index && ofs->upper_mnt);
> >
> >         /* fsid 0 is reserved for upper fs even with non upper overlay */
> >         if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> > @@ -1187,6 +1190,22 @@ static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb)
> >         for (i = 0; i < ofs->numlowerfs; i++) {
> >                 if (ofs->lower_fs[i].sb == sb)
> >                         return i + 1;
> > +
> > +               /*
> > +                * We use uuid to associate an overlay lower file handle with a
> > +                * lower layer, so we can accept lower fs with null uuid as long
> > +                * as all lower layers with null uuid are on the same fs.
> > +                */
> > +               if (need_uuid &&
> > +                   uuid_equal(&ofs->lower_fs[i].sb->s_uuid, &sb->s_uuid)) {
> > +                       need_uuid = false;
> > +                       ofs->config.index = false;
> > +                       ofs->config.nfs_export = false;
> > +                       pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> > +                               uuid_is_null(&sb->s_uuid) ? "null" :
> > +                                                           "conflicting",
> > +                               mnt->mnt_root);
>
> mnt->mnt_root is wrong here: this is path->mnt resolved from the
> lowerdir, not the one cloned from that path.
>
> Fixed so ovl_get_lower_layers() passes in the path, not just the mount.
>
> Also made this a separate loop from the one finding a matching fs, as
> it's logically a different step and not needed in the case when the fs
> is found.
>
> Updated but untested patch attached.
>

Tested. looks good.

Thanks,
Amir.

Re: [PATCH 2/3] ovl: disable xino for some nested overlay cases

[Amir Goldstein]

On Wed, Oct 24, 2018 at 6:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 24, 2018 at 5:03 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Oct 24, 2018 at 5:30 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >>
> >> On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> >> > There are three possible cases when overlayfs is used as lower
> >> > layer for a nested overlay mount:
> >> >
> >> > 1. lower overlay is non-samefs with xino enabled
> >> > 2. lower overlay is non-samefs with xino disabled
> >> > 3. lower overlay is samefs
> >> >
> >> > In the first case, lower layer uses high inode number bits, so they are
> >> > not available for the nested overlay and xino should be disabled.
> >> >
> >> > In the second case, inode numbers of lower layer are not in a single
> >> > address space, so there is no use enabling xino in nested overlay.
> >> >
> >> > In the last case (samefs) the lower layer inode number address space
> >> > is that of the underlying real fs, so we can assign fsid of the lower
> >> > layer according the the real underlying fs.
> >> >
> >> > In the private case of all lower overlay layers on the same fs, which is
> >> > also the upper fs of the nested overlay, the nested overlay itself is
> >> > treated as "samefs", because inode numbers in all layers are from the
> >> > same address space.
> >> >
> >> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> > ---
> >> >  fs/overlayfs/overlayfs.h |  8 ++++++++
> >> >  fs/overlayfs/super.c     | 25 +++++++++++++++++++++++--
> >> >  2 files changed, 31 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> >> > index f61839e1054c..d32fe09a222f 100644
> >> > --- a/fs/overlayfs/overlayfs.h
> >> > +++ b/fs/overlayfs/overlayfs.h
> >> > @@ -418,3 +418,11 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> >> >
> >> >  /* export.c */
> >> >  extern const struct export_operations ovl_export_operations;
> >> > +
> >> > +/* super.c */
> >> > +extern struct file_system_type ovl_fs_type;
> >> > +
> >> > +static inline bool ovl_is_overlay_fs(struct super_block *sb)
> >> > +{
> >> > +       return sb->s_type == &ovl_fs_type;
> >> > +}
> >>
> >> Special casing like that is, well, ugly.
> >>
> >> Generic solution (which would work in the face of other stacking fs)
> >> would be to have a "ino-space-id" in the super block that would be
> >> assigned to a unique number for all distinct filesystems, but stacking
> >> fs would be able to reset it to the origin filesystem's id if
> >> appropriate.
> >>
> >> What do you think about that?
> >>
[...]
>
> I was thinking of the special case of all - real underlying layers of
> nested and outer overlay - being on the same fs.  I thought this patch
> was about using the samefs info from the nested overlay to allow using
> the samefs logics for the outer overlay as well.
>

Ah. Is it really worth the trouble?
What are the chances of use case other than nested overlayfs
will really present itself?
IMO it is not *that* ugly to "special case" your own fs type, but
it's your definition of "ugly" that counts.

Let me know if you want me to add s_ino_dev at this time.
When non-zero, it is meant to fill stat->dev instead of s_dev,
which is what we do in samefs case, so in theory we could
replace most current callers of ovl_same_sb() with:

dev_t ovl_same_dev(struct super_block *sb)
{
    return sb->s_ino_dev;
}

Thanks,
Amir.

Re: [PATCH 3/3] ovl: compact nested ovl_fh

[Amir Goldstein]

On Wed, Oct 24, 2018 at 5:34 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Sep 3, 2018 at 8:12 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > When encoding a file handle from lower nested overlayfs, prepending
> > another ovl_fh header for nested overlay file handle adds no new
> > information for decoding.  Instead, we just set a 'nested' flag in ovl_fh
> > header for nested file handle, so we know to distinguish between real
> > upper file handle that should be decoded from real upper layer and
> > nested upper file handle that should be decoded from nested lower layer.
> > For the maximum allowed overlay nesting depth of 1, one bit is enough.
>
> This is an optimization, right?
>

Optimization was not my motivation.
It started off as a patch to enable NFS export for nested overlay,
which I needed for snapshots, but then I relaxed uuid requirement
and nested overlay NFS export just worked (I think - need to re-check)
Then it felt a bit silly to double encode, so posted this patch with its
current title, because it did not change functionality anymore, but...

> What are the user-visible advantages that would justify the added complexity?
>

...When you put it this way, it's really hard to justify the patch on its own.
One thing that comes to mind w.r.t. advantages of compact
representation of ovl_fh
is in-inode xattrs space, but that is quite a far fetched justification.
If we wanted more compact xattrs we should have started with shorter names...

So feel free to drop this patch.
If I ever come with a better reason, I will re-post it.

Thanks,
Amir.