[PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

Miklos,

This v2 combines the prep patch set [1] and lazy lookup patch set [2].

This work is motivated by Alexander's composefs use case.
Alexander has been developing and testing his fsverity patches over
my lazy-lowerdata-lookup branch [3].

Alexander has also written tests for lazy lowerdata lookup [4].

Note that patch #1 is a Fixes patch for stable.
Gao commented that the fix may not be complete, but I think it is better
than no fix at all.

Regarding lazy lookup in d_real(), I am not sure if the best effort
lookup is the best solution, but in any case, none of this code kicks in
without explicit opt-in to data-only layers, so the risk of breaking
existing setups is quite low.

Thanks,
Amir.

Changes since v1:
- Include the prep patch set
- Split remove lowerdata from add lowerdata_redirect patch
- Remove embedded ovl_entry stack optimization
- Add lazy lookup and comment in d_real_inode()
- Improve documentation of :: data-only layers syntax
- Added RVBs

[1] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-unionfs/20230412135412.1684197-1-amir73il@gmail.com/
[3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
[4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata

Amir Goldstein (13):
  ovl: update of dentry revalidate flags after copy up
  ovl: use OVL_E() and OVL_E_FLAGS() accessors
  ovl: use ovl_numlower() and ovl_lowerstack() accessors
  ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
  ovl: move ovl_entry into ovl_inode
  ovl: deduplicate lowerpath and lowerstack[]
  ovl: deduplicate lowerdata and lowerstack[]
  ovl: remove unneeded goto instructions
  ovl: introduce data-only lower layers
  ovl: implement lookup in data-only layers
  ovl: prepare to store lowerdata redirect for lazy lowerdata lookup
  ovl: prepare for lazy lookup of lowerdata inode
  ovl: implement lazy lookup of lowerdata in data-only layers

 Documentation/filesystems/overlayfs.rst |  36 +++++
 fs/overlayfs/copy_up.c                  |  11 ++
 fs/overlayfs/dir.c                      |   3 +-
 fs/overlayfs/export.c                   |  41 +++---
 fs/overlayfs/file.c                     |  21 ++-
 fs/overlayfs/inode.c                    |  38 +++--
 fs/overlayfs/namei.c                    | 185 +++++++++++++++++++-----
 fs/overlayfs/overlayfs.h                |  20 ++-
 fs/overlayfs/ovl_entry.h                |  73 ++++++++--
 fs/overlayfs/super.c                    | 132 ++++++++++-------
 fs/overlayfs/util.c                     | 165 ++++++++++++++++-----
 11 files changed, 534 insertions(+), 191 deletions(-)

-- 
2.34.1

[PATCH v2 01/13] ovl: update of dentry revalidate flags after copy up

[Amir Goldstein]

After copy up, we may need to update d_flags if upper dentry is on a
remote fs and lower dentries are not.

Add helpers to allow incremental update of the revalidate flags.

Fixes: bccece1ead36 ("ovl: allow remote upper")
Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  2 ++
 fs/overlayfs/dir.c       |  3 +--
 fs/overlayfs/export.c    |  3 +--
 fs/overlayfs/namei.c     |  3 +--
 fs/overlayfs/overlayfs.h |  6 ++++--
 fs/overlayfs/super.c     |  2 +-
 fs/overlayfs/util.c      | 24 ++++++++++++++++++++----
 7 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index c14e90764e35..7bf101e756c8 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -576,6 +576,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 			/* Restore timestamps on parent (best effort) */
 			ovl_set_timestamps(ofs, upperdir, &c->pstat);
 			ovl_dentry_set_upper_alias(c->dentry);
+			ovl_dentry_update_reval(c->dentry, upper);
 		}
 	}
 	inode_unlock(udir);
@@ -895,6 +896,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		inode_unlock(udir);
 
 		ovl_dentry_set_upper_alias(c->dentry);
+		ovl_dentry_update_reval(c->dentry, ovl_dentry_upper(c->dentry));
 	}
 
 out:
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index fc25fb95d5fc..9be52d8013c8 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -269,8 +269,7 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 
 	ovl_dir_modified(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
-	ovl_dentry_update_reval(dentry, newdentry,
-			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+	ovl_dentry_init_reval(dentry, newdentry);
 
 	if (!hardlink) {
 		/*
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index defd4e231ad2..5c36fb3a7bab 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -326,8 +326,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	if (upper_alias)
 		ovl_dentry_set_upper_alias(dentry);
 
-	ovl_dentry_update_reval(dentry, upper,
-			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+	ovl_dentry_init_reval(dentry, upper);
 
 	return d_instantiate_anon(dentry, inode);
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index cfb3420b7df0..100a492d2b2a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1122,8 +1122,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
-	ovl_dentry_update_reval(dentry, upperdentry,
-			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+	ovl_dentry_init_reval(dentry, upperdentry);
 
 	revert_creds(old_cred);
 	if (origin_path) {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4d0b278f5630..e100c55bb924 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -375,8 +375,10 @@ bool ovl_index_all(struct super_block *sb);
 bool ovl_verify_lower(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 bool ovl_dentry_remote(struct dentry *dentry);
-void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
-			     unsigned int mask);
+void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *realdentry);
+void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry);
+void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
+			   unsigned int mask);
 bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f1d9f75f8786..49b6956468f9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1885,7 +1885,7 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 	ovl_dentry_set_flag(OVL_E_CONNECTED, root);
 	ovl_set_upperdata(d_inode(root));
 	ovl_inode_init(d_inode(root), &oip, ino, fsid);
-	ovl_dentry_update_reval(root, upperdentry, DCACHE_OP_WEAK_REVALIDATE);
+	ovl_dentry_init_flags(root, upperdentry, DCACHE_OP_WEAK_REVALIDATE);
 
 	return root;
 }
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 923d66d131c1..6a0652bd51f2 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -94,14 +94,30 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 	return oe;
 }
 
+#define OVL_D_REVALIDATE (DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE)
+
 bool ovl_dentry_remote(struct dentry *dentry)
 {
-	return dentry->d_flags &
-		(DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+	return dentry->d_flags & OVL_D_REVALIDATE;
+}
+
+void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *realdentry)
+{
+	if (!ovl_dentry_remote(realdentry))
+		return;
+
+	spin_lock(&dentry->d_lock);
+	dentry->d_flags |= realdentry->d_flags & OVL_D_REVALIDATE;
+	spin_unlock(&dentry->d_lock);
+}
+
+void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry)
+{
+	return ovl_dentry_init_flags(dentry, upperdentry, OVL_D_REVALIDATE);
 }
 
-void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
-			     unsigned int mask)
+void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
+			   unsigned int mask)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 	unsigned int i, flags = 0;
-- 
2.34.1

[PATCH v2 02/13] ovl: use OVL_E() and OVL_E_FLAGS() accessors

[Amir Goldstein]

Instead of open coded instances, because we are about to split
the two apart.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/export.c    |  2 +-
 fs/overlayfs/namei.c     |  8 ++++----
 fs/overlayfs/ovl_entry.h |  5 +++++
 fs/overlayfs/super.c     |  2 +-
 fs/overlayfs/util.c      | 20 ++++++++++----------
 5 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 5c36fb3a7bab..2cfdfcca5659 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -341,7 +341,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 /* Get the upper or lower dentry in stack whose on layer @idx */
 static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 	int i;
 
 	if (!idx)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 100a492d2b2a..e66352f19755 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -790,7 +790,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
  */
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	BUG_ON(idx < 0);
 	if (idx == 0) {
@@ -833,8 +833,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct ovl_entry *oe;
 	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 ovl_entry *poe = OVL_E(dentry->d_parent);
+	struct ovl_entry *roe = OVL_E(dentry->d_sb->s_root);
 	struct ovl_path *stack = NULL, *origin_path = NULL;
 	struct dentry *upperdir, *upperdentry = NULL;
 	struct dentry *origin = NULL;
@@ -1157,7 +1157,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 bool ovl_lower_positive(struct dentry *dentry)
 {
-	struct ovl_entry *poe = dentry->d_parent->d_fsdata;
+	struct ovl_entry *poe = OVL_E(dentry->d_parent);
 	const struct qstr *name = &dentry->d_name;
 	const struct cred *old_cred;
 	unsigned int i;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index fd11fe6d6d45..4c7312126b3b 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -124,6 +124,11 @@ static inline struct ovl_entry *OVL_E(struct dentry *dentry)
 	return (struct ovl_entry *) dentry->d_fsdata;
 }
 
+static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
+{
+	return &OVL_E(dentry)->flags;
+}
+
 struct ovl_inode {
 	union {
 		struct ovl_dir_cache *cache;	/* directory */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 49b6956468f9..108824b359e6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -138,7 +138,7 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 static int ovl_dentry_revalidate_common(struct dentry *dentry,
 					unsigned int flags, bool weak)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 	struct inode *inode = d_inode_rcu(dentry);
 	struct dentry *upper;
 	unsigned int i;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 6a0652bd51f2..01e6b4ec3074 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -143,7 +143,7 @@ bool ovl_dentry_weird(struct dentry *dentry)
 
 enum ovl_path_type ovl_path_type(struct dentry *dentry)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 	enum ovl_path_type type = 0;
 
 	if (ovl_dentry_upper(dentry)) {
@@ -176,7 +176,7 @@ void ovl_path_upper(struct dentry *dentry, struct path *path)
 
 void ovl_path_lower(struct dentry *dentry, struct path *path)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	if (oe->numlower) {
 		path->mnt = oe->lowerstack[0].layer->mnt;
@@ -188,7 +188,7 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	if (oe->numlower) {
 		path->mnt = oe->lowerstack[oe->numlower - 1].layer->mnt;
@@ -231,14 +231,14 @@ struct dentry *ovl_dentry_upper(struct dentry *dentry)
 
 struct dentry *ovl_dentry_lower(struct dentry *dentry)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 
 const struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	return oe->numlower ? oe->lowerstack[0].layer : NULL;
 }
@@ -251,7 +251,7 @@ const struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
  */
 struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	return oe->numlower ? oe->lowerstack[oe->numlower - 1].dentry : NULL;
 }
@@ -329,17 +329,17 @@ void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache)
 
 void ovl_dentry_set_flag(unsigned long flag, struct dentry *dentry)
 {
-	set_bit(flag, &OVL_E(dentry)->flags);
+	set_bit(flag, OVL_E_FLAGS(dentry));
 }
 
 void ovl_dentry_clear_flag(unsigned long flag, struct dentry *dentry)
 {
-	clear_bit(flag, &OVL_E(dentry)->flags);
+	clear_bit(flag, OVL_E_FLAGS(dentry));
 }
 
 bool ovl_dentry_test_flag(unsigned long flag, struct dentry *dentry)
 {
-	return test_bit(flag, &OVL_E(dentry)->flags);
+	return test_bit(flag, OVL_E_FLAGS(dentry));
 }
 
 bool ovl_dentry_is_opaque(struct dentry *dentry)
@@ -1015,7 +1015,7 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path)
 
 bool ovl_is_metacopy_dentry(struct dentry *dentry)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	if (!d_is_reg(dentry))
 		return false;
-- 
2.34.1

[PATCH v2 03/13] ovl: use ovl_numlower() and ovl_lowerstack() accessors

[Amir Goldstein]

This helps fortify against dereferencing a NULL ovl_entry,
before we move the ovl_entry reference into ovl_inode.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/export.c    | 18 ++++++++++--------
 fs/overlayfs/namei.c     | 34 ++++++++++++++++++----------------
 fs/overlayfs/ovl_entry.h | 14 ++++++++++++--
 fs/overlayfs/super.c     | 21 ++++++++++++---------
 fs/overlayfs/util.c      | 36 ++++++++++++++++++++----------------
 5 files changed, 72 insertions(+), 51 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 2cfdfcca5659..ddb546627749 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -80,7 +80,7 @@ static int ovl_connectable_layer(struct dentry *dentry)
 
 	/* We can get overlay root from root of any layer */
 	if (dentry == dentry->d_sb->s_root)
-		return oe->numlower;
+		return ovl_numlower(oe);
 
 	/*
 	 * If it's an unindexed merge dir, then it's not connectable with any
@@ -91,7 +91,7 @@ static int ovl_connectable_layer(struct dentry *dentry)
 		return 0;
 
 	/* We can get upper/overlay path from indexed/lower dentry */
-	return oe->lowerstack[0].layer->idx;
+	return ovl_lowerstack(oe)->layer->idx;
 }
 
 /*
@@ -105,6 +105,7 @@ static int ovl_connectable_layer(struct dentry *dentry)
 static int ovl_connect_layer(struct dentry *dentry)
 {
 	struct dentry *next, *parent = NULL;
+	struct ovl_entry *oe = OVL_E(dentry);
 	int origin_layer;
 	int err = 0;
 
@@ -112,7 +113,7 @@ static int ovl_connect_layer(struct dentry *dentry)
 	    WARN_ON(!ovl_dentry_lower(dentry)))
 		return -EIO;
 
-	origin_layer = OVL_E(dentry)->lowerstack[0].layer->idx;
+	origin_layer = ovl_lowerstack(oe)->layer->idx;
 	if (ovl_dentry_test_flag(OVL_E_CONNECTED, dentry))
 		return origin_layer;
 
@@ -319,8 +320,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 		goto nomem;
 
 	if (lower) {
-		oe->lowerstack->dentry = dget(lower);
-		oe->lowerstack->layer = lowerpath->layer;
+		ovl_lowerstack(oe)->dentry = dget(lower);
+		ovl_lowerstack(oe)->layer = lowerpath->layer;
 	}
 	dentry->d_fsdata = oe;
 	if (upper_alias)
@@ -342,14 +343,15 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 	int i;
 
 	if (!idx)
 		return ovl_dentry_upper(dentry);
 
-	for (i = 0; i < oe->numlower; i++) {
-		if (oe->lowerstack[i].layer->idx == idx)
-			return oe->lowerstack[i].dentry;
+	for (i = 0; i < ovl_numlower(oe); i++) {
+		if (lowerstack[i].layer->idx == idx)
+			return lowerstack[i].dentry;
 	}
 
 	return NULL;
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e66352f19755..31f889d27083 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -791,19 +791,20 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 
 	BUG_ON(idx < 0);
 	if (idx == 0) {
 		ovl_path_upper(dentry, path);
 		if (path->dentry)
-			return oe->numlower ? 1 : -1;
+			return ovl_numlower(oe) ? 1 : -1;
 		idx++;
 	}
-	BUG_ON(idx > oe->numlower);
-	path->dentry = oe->lowerstack[idx - 1].dentry;
-	path->mnt = oe->lowerstack[idx - 1].layer->mnt;
+	BUG_ON(idx > ovl_numlower(oe));
+	path->dentry = lowerstack[idx - 1].dentry;
+	path->mnt = lowerstack[idx - 1].layer->mnt;
 
-	return (idx < oe->numlower) ? idx + 1 : -1;
+	return (idx < ovl_numlower(oe)) ? idx + 1 : -1;
 }
 
 /* Fix missing 'origin' xattr */
@@ -853,7 +854,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.is_dir = false,
 		.opaque = false,
 		.stop = false,
-		.last = ofs->config.redirect_follow ? false : !poe->numlower,
+		.last = ofs->config.redirect_follow ? false : !ovl_numlower(poe),
 		.redirect = NULL,
 		.metacopy = false,
 	};
@@ -904,7 +905,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		upperopaque = d.opaque;
 	}
 
-	if (!d.stop && poe->numlower) {
+	if (!d.stop && ovl_numlower(poe)) {
 		err = -ENOMEM;
 		stack = kcalloc(ofs->numlayer - 1, sizeof(struct ovl_path),
 				GFP_KERNEL);
@@ -912,13 +913,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put_upper;
 	}
 
-	for (i = 0; !d.stop && i < poe->numlower; i++) {
-		struct ovl_path lower = poe->lowerstack[i];
+	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
+		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
 		if (!ofs->config.redirect_follow)
-			d.last = i == poe->numlower - 1;
+			d.last = i == ovl_numlower(poe) - 1;
 		else
-			d.last = lower.layer->idx == roe->numlower;
+			d.last = lower.layer->idx == ovl_numlower(roe);
 
 		d.mnt = lower.layer->mnt;
 		err = ovl_lookup_layer(lower.dentry, &d, &this, false);
@@ -1072,7 +1073,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (!oe)
 		goto out_put;
 
-	memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
+	memcpy(ovl_lowerstack(oe), stack, sizeof(struct ovl_path) * ctr);
 	dentry->d_fsdata = oe;
 
 	if (upperopaque)
@@ -1177,12 +1178,13 @@ bool ovl_lower_positive(struct dentry *dentry)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	/* Positive upper -> have to look up lower to see whether it exists */
-	for (i = 0; !done && !positive && i < poe->numlower; i++) {
+	for (i = 0; !done && !positive && i < ovl_numlower(poe); i++) {
 		struct dentry *this;
-		struct dentry *lowerdir = poe->lowerstack[i].dentry;
+		struct ovl_path *parentpath = &ovl_lowerstack(poe)[i];
 
-		this = lookup_one_positive_unlocked(mnt_idmap(poe->lowerstack[i].layer->mnt),
-						   name->name, lowerdir, name->len);
+		this = lookup_one_positive_unlocked(
+				mnt_idmap(parentpath->layer->mnt),
+				name->name, parentpath->dentry, name->len);
 		if (IS_ERR(this)) {
 			switch (PTR_ERR(this)) {
 			case -ENOENT:
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 4c7312126b3b..0e009fa39d54 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -113,10 +113,20 @@ struct ovl_entry {
 		};
 		struct rcu_head rcu;
 	};
-	unsigned numlower;
-	struct ovl_path lowerstack[];
+	unsigned int __numlower;
+	struct ovl_path __lowerstack[];
 };
 
+static inline unsigned int ovl_numlower(struct ovl_entry *oe)
+{
+	return oe ? oe->__numlower : 0;
+}
+
+static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
+{
+	return ovl_numlower(oe) ? oe->__lowerstack : NULL;
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 
 static inline struct ovl_entry *OVL_E(struct dentry *dentry)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 108824b359e6..89e9843bd2de 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -56,10 +56,11 @@ MODULE_PARM_DESC(xino_auto,
 
 static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 	unsigned int i;
 
-	for (i = 0; i < oe->numlower; i++)
-		dput(oe->lowerstack[i].dentry);
+	for (i = 0; i < ovl_numlower(oe); i++)
+		dput(lowerstack[i].dentry);
 }
 
 static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
@@ -139,6 +140,7 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 					unsigned int flags, bool weak)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 	struct inode *inode = d_inode_rcu(dentry);
 	struct dentry *upper;
 	unsigned int i;
@@ -152,9 +154,8 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 	if (upper)
 		ret = ovl_revalidate_real(upper, flags, weak);
 
-	for (i = 0; ret > 0 && i < oe->numlower; i++) {
-		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
-					  weak);
+	for (i = 0; ret > 0 && i < ovl_numlower(oe); i++) {
+		ret = ovl_revalidate_real(lowerstack[i].dentry, flags, weak);
 	}
 	return ret;
 }
@@ -1462,7 +1463,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
 
 	/* Verify lower root is upper root origin */
 	err = ovl_verify_origin(ofs, upperpath->dentry,
-				oe->lowerstack[0].dentry, true);
+				ovl_lowerstack(oe)->dentry, true);
 	if (err) {
 		pr_err("failed to verify upper root origin\n");
 		goto out;
@@ -1725,6 +1726,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 {
 	int err;
 	struct path *stack = NULL;
+	struct ovl_path *lowerstack;
 	unsigned int i;
 	struct ovl_entry *oe;
 
@@ -1762,9 +1764,10 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (!oe)
 		goto out_err;
 
+	lowerstack = ovl_lowerstack(oe);
 	for (i = 0; i < numlower; i++) {
-		oe->lowerstack[i].dentry = dget(stack[i].dentry);
-		oe->lowerstack[i].layer = &ofs->layers[i+1];
+		lowerstack[i].dentry = dget(stack[i].dentry);
+		lowerstack[i].layer = &ofs->layers[i+1];
 	}
 
 out:
@@ -1857,7 +1860,7 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 				   struct ovl_entry *oe)
 {
 	struct dentry *root;
-	struct ovl_path *lowerpath = &oe->lowerstack[0];
+	struct ovl_path *lowerpath = ovl_lowerstack(oe);
 	unsigned long ino = d_inode(lowerpath->dentry)->i_ino;
 	int fsid = lowerpath->layer->fsid;
 	struct ovl_inode_params oip = {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 01e6b4ec3074..a139eb581093 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -85,11 +85,11 @@ bool ovl_verify_lower(struct super_block *sb)
 
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
-	size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
+	size_t size = offsetof(struct ovl_entry, __lowerstack[numlower]);
 	struct ovl_entry *oe = kzalloc(size, GFP_KERNEL);
 
 	if (oe)
-		oe->numlower = numlower;
+		oe->__numlower = numlower;
 
 	return oe;
 }
@@ -120,12 +120,13 @@ void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
 			   unsigned int mask)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 	unsigned int i, flags = 0;
 
 	if (upperdentry)
 		flags |= upperdentry->d_flags;
-	for (i = 0; i < oe->numlower; i++)
-		flags |= oe->lowerstack[i].dentry->d_flags;
+	for (i = 0; i < ovl_numlower(oe); i++)
+		flags |= lowerstack[i].dentry->d_flags;
 
 	spin_lock(&dentry->d_lock);
 	dentry->d_flags &= ~mask;
@@ -152,7 +153,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 		/*
 		 * Non-dir dentry can hold lower dentry of its copy up origin.
 		 */
-		if (oe->numlower) {
+		if (ovl_numlower(oe)) {
 			if (ovl_test_flag(OVL_CONST_INO, d_inode(dentry)))
 				type |= __OVL_PATH_ORIGIN;
 			if (d_is_dir(dentry) ||
@@ -160,7 +161,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 				type |= __OVL_PATH_MERGE;
 		}
 	} else {
-		if (oe->numlower > 1)
+		if (ovl_numlower(oe) > 1)
 			type |= __OVL_PATH_MERGE;
 	}
 	return type;
@@ -177,10 +178,11 @@ void ovl_path_upper(struct dentry *dentry, struct path *path)
 void ovl_path_lower(struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerpath = ovl_lowerstack(oe);
 
-	if (oe->numlower) {
-		path->mnt = oe->lowerstack[0].layer->mnt;
-		path->dentry = oe->lowerstack[0].dentry;
+	if (ovl_numlower(oe)) {
+		path->mnt = lowerpath->layer->mnt;
+		path->dentry = lowerpath->dentry;
 	} else {
 		*path = (struct path) { };
 	}
@@ -189,10 +191,11 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 
-	if (oe->numlower) {
-		path->mnt = oe->lowerstack[oe->numlower - 1].layer->mnt;
-		path->dentry = oe->lowerstack[oe->numlower - 1].dentry;
+	if (ovl_numlower(oe)) {
+		path->mnt = lowerstack[ovl_numlower(oe) - 1].layer->mnt;
+		path->dentry = lowerstack[ovl_numlower(oe) - 1].dentry;
 	} else {
 		*path = (struct path) { };
 	}
@@ -233,14 +236,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 
-	return oe->numlower ? oe->lowerstack[0].dentry : NULL;
+	return ovl_numlower(oe) ? ovl_lowerstack(oe)->dentry : NULL;
 }
 
 const struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 
-	return oe->numlower ? oe->lowerstack[0].layer : NULL;
+	return ovl_numlower(oe) ? ovl_lowerstack(oe)->layer : NULL;
 }
 
 /*
@@ -253,7 +256,8 @@ struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 
-	return oe->numlower ? oe->lowerstack[oe->numlower - 1].dentry : NULL;
+	return ovl_numlower(oe) ?
+		ovl_lowerstack(oe)[ovl_numlower(oe) - 1].dentry : NULL;
 }
 
 struct dentry *ovl_dentry_real(struct dentry *dentry)
@@ -1026,7 +1030,7 @@ bool ovl_is_metacopy_dentry(struct dentry *dentry)
 		return false;
 	}
 
-	return (oe->numlower > 1);
+	return (ovl_numlower(oe) > 1);
 }
 
 char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding)
-- 
2.34.1

[PATCH v2 04/13] ovl: factor out ovl_free_entry() and ovl_stack_*() helpers

[Amir Goldstein]

In preparation for moving lowerstack into ovl_inode.

Note that in ovl_lookup() the temp stack dentry refs are now cloned
into the final ovl_lowerstack instead of being transferred, so cleanup
always needs to call ovl_stack_free(stack).

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 13 +++++--------
 fs/overlayfs/overlayfs.h |  5 +++++
 fs/overlayfs/ovl_entry.h |  2 --
 fs/overlayfs/super.c     | 14 ++------------
 fs/overlayfs/util.c      | 34 ++++++++++++++++++++++++++++++++++
 5 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 31f889d27083..c237c8dbff09 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -907,8 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	if (!d.stop && ovl_numlower(poe)) {
 		err = -ENOMEM;
-		stack = kcalloc(ofs->numlayer - 1, sizeof(struct ovl_path),
-				GFP_KERNEL);
+		stack = ovl_stack_alloc(ofs->numlayer - 1);
 		if (!stack)
 			goto out_put_upper;
 	}
@@ -1073,7 +1072,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (!oe)
 		goto out_put;
 
-	memcpy(ovl_lowerstack(oe), stack, sizeof(struct ovl_path) * ctr);
+	ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
 	dentry->d_fsdata = oe;
 
 	if (upperopaque)
@@ -1131,18 +1130,16 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		kfree(origin_path);
 	}
 	dput(index);
-	kfree(stack);
+	ovl_stack_free(stack, ctr);
 	kfree(d.redirect);
 	return d_splice_alias(inode, dentry);
 
 out_free_oe:
 	dentry->d_fsdata = NULL;
-	kfree(oe);
+	ovl_free_entry(oe);
 out_put:
 	dput(index);
-	for (i = 0; i < ctr; i++)
-		dput(stack[i].dentry);
-	kfree(stack);
+	ovl_stack_free(stack, ctr);
 out_put_upper:
 	if (origin_path) {
 		dput(origin_path->dentry);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e100c55bb924..6a50296fef8f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -373,7 +373,12 @@ int ovl_can_decode_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);
+struct ovl_path *ovl_stack_alloc(unsigned int n);
+void ovl_stack_cpy(struct ovl_path *dst, struct ovl_path *src, unsigned int n);
+void ovl_stack_put(struct ovl_path *stack, unsigned int n);
+void ovl_stack_free(struct ovl_path *stack, unsigned int n);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
+void ovl_free_entry(struct ovl_entry *oe);
 bool ovl_dentry_remote(struct dentry *dentry);
 void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *realdentry);
 void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 0e009fa39d54..608742f60037 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -127,8 +127,6 @@ static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
 	return ovl_numlower(oe) ? oe->__lowerstack : NULL;
 }
 
-struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
-
 static inline struct ovl_entry *OVL_E(struct dentry *dentry)
 {
 	return (struct ovl_entry *) dentry->d_fsdata;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 89e9843bd2de..d8fe857bd7e1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -54,15 +54,6 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
 MODULE_PARM_DESC(xino_auto,
 		 "Auto enable xino feature");
 
-static void ovl_entry_stack_free(struct ovl_entry *oe)
-{
-	struct ovl_path *lowerstack = ovl_lowerstack(oe);
-	unsigned int i;
-
-	for (i = 0; i < ovl_numlower(oe); i++)
-		dput(lowerstack[i].dentry);
-}
-
 static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
 module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
 MODULE_PARM_DESC(metacopy,
@@ -73,7 +64,7 @@ static void ovl_dentry_release(struct dentry *dentry)
 	struct ovl_entry *oe = dentry->d_fsdata;
 
 	if (oe) {
-		ovl_entry_stack_free(oe);
+		ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
 		kfree_rcu(oe, rcu);
 	}
 }
@@ -2078,8 +2069,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	return 0;
 
 out_free_oe:
-	ovl_entry_stack_free(oe);
-	kfree(oe);
+	ovl_free_entry(oe);
 out_err:
 	kfree(splitlower);
 	path_put(&upperpath);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index a139eb581093..1ba6dbea808c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -83,6 +83,34 @@ bool ovl_verify_lower(struct super_block *sb)
 	return ofs->config.nfs_export && ofs->config.index;
 }
 
+struct ovl_path *ovl_stack_alloc(unsigned int n)
+{
+	return kcalloc(n, sizeof(struct ovl_path), GFP_KERNEL);
+}
+
+void ovl_stack_cpy(struct ovl_path *dst, struct ovl_path *src, unsigned int n)
+{
+	unsigned int i;
+
+	memcpy(dst, src, sizeof(struct ovl_path) * n);
+	for (i = 0; i < n; i++)
+		dget(src[i].dentry);
+}
+
+void ovl_stack_put(struct ovl_path *stack, unsigned int n)
+{
+	unsigned int i;
+
+	for (i = 0; stack && i < n; i++)
+		dput(stack[i].dentry);
+}
+
+void ovl_stack_free(struct ovl_path *stack, unsigned int n)
+{
+	ovl_stack_put(stack, n);
+	kfree(stack);
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
 	size_t size = offsetof(struct ovl_entry, __lowerstack[numlower]);
@@ -94,6 +122,12 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 	return oe;
 }
 
+void ovl_free_entry(struct ovl_entry *oe)
+{
+	ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
+	kfree(oe);
+}
+
 #define OVL_D_REVALIDATE (DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE)
 
 bool ovl_dentry_remote(struct dentry *dentry)
-- 
2.34.1

[PATCH v2 05/13] ovl: move ovl_entry into ovl_inode

[Amir Goldstein]

The lower stacks of all the ovl inode aliases should be identical
and there is redundant information in ovl_entry and ovl_inode.

Move lowerstack into ovl_inode and keep only the OVL_E_FLAGS
per overlay dentry.

Following patches will deduplicate redundant ovl_inode fields.

Note that for a negative dentry, OVL_E(dentry) can now be NULL,
so it is imporatnt to use the ovl_numlower() accessor.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       |  2 +-
 fs/overlayfs/export.c    | 22 ++++++++++++----------
 fs/overlayfs/inode.c     |  8 ++++----
 fs/overlayfs/namei.c     |  5 ++---
 fs/overlayfs/overlayfs.h |  6 ++++--
 fs/overlayfs/ovl_entry.h | 36 ++++++++++++++++++------------------
 fs/overlayfs/super.c     | 18 ++++--------------
 fs/overlayfs/util.c      |  8 ++++----
 8 files changed, 49 insertions(+), 56 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 9be52d8013c8..92bdcedfaaec 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -269,7 +269,7 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 
 	ovl_dir_modified(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
-	ovl_dentry_init_reval(dentry, newdentry);
+	ovl_dentry_init_reval(dentry, newdentry, NULL);
 
 	if (!hardlink) {
 		/*
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index ddb546627749..be142ea73fad 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -286,7 +286,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	struct dentry *lower = lowerpath ? lowerpath->dentry : NULL;
 	struct dentry *upper = upper_alias ?: index;
 	struct dentry *dentry;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	struct ovl_entry *oe;
 	struct ovl_inode_params oip = {
 		.lowerpath = lowerpath,
@@ -298,9 +298,19 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	if (d_is_dir(upper ?: lower))
 		return ERR_PTR(-EIO);
 
+	oe = ovl_alloc_entry(!!lower);
+	if (!oe)
+		return ERR_PTR(-ENOMEM);
+
 	oip.upperdentry = dget(upper);
+	if (lower) {
+		ovl_lowerstack(oe)->dentry = dget(lower);
+		ovl_lowerstack(oe)->layer = lowerpath->layer;
+	}
+	oip.oe = oe;
 	inode = ovl_get_inode(sb, &oip);
 	if (IS_ERR(inode)) {
+		ovl_free_entry(oe);
 		dput(upper);
 		return ERR_CAST(inode);
 	}
@@ -315,19 +325,11 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	dentry = d_alloc_anon(inode->i_sb);
 	if (unlikely(!dentry))
 		goto nomem;
-	oe = ovl_alloc_entry(lower ? 1 : 0);
-	if (!oe)
-		goto nomem;
 
-	if (lower) {
-		ovl_lowerstack(oe)->dentry = dget(lower);
-		ovl_lowerstack(oe)->layer = lowerpath->layer;
-	}
-	dentry->d_fsdata = oe;
 	if (upper_alias)
 		ovl_dentry_set_upper_alias(dentry);
 
-	ovl_dentry_init_reval(dentry, upper);
+	ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));
 
 	return d_instantiate_anon(dentry, inode);
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..c296bd656858 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1003,8 +1003,9 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 	struct inode *realinode;
 	struct ovl_inode *oi = OVL_I(inode);
 
-	if (oip->upperdentry)
-		oi->__upperdentry = oip->upperdentry;
+	oi->__upperdentry = oip->upperdentry;
+	oi->oe = oip->oe;
+	oi->redirect = oip->redirect;
 	if (oip->lowerpath && oip->lowerpath->dentry) {
 		oi->lowerpath.dentry = dget(oip->lowerpath->dentry);
 		oi->lowerpath.layer = oip->lowerpath->layer;
@@ -1369,6 +1370,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			}
 
 			dput(upperdentry);
+			ovl_free_entry(oip->oe);
 			kfree(oip->redirect);
 			goto out;
 		}
@@ -1398,8 +1400,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	if (oip->index)
 		ovl_set_flag(OVL_INDEX, inode);
 
-	OVL_I(inode)->redirect = oip->redirect;
-
 	if (bylower)
 		ovl_set_flag(OVL_CONST_INO, inode);
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index c237c8dbff09..a0a1c498dbd1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1073,7 +1073,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		goto out_put;
 
 	ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
-	dentry->d_fsdata = oe;
 
 	if (upperopaque)
 		ovl_dentry_set_opaque(dentry);
@@ -1107,6 +1106,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		struct ovl_inode_params oip = {
 			.upperdentry = upperdentry,
 			.lowerpath = stack,
+			.oe = oe,
 			.index = index,
 			.numlower = ctr,
 			.redirect = upperredirect,
@@ -1122,7 +1122,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
-	ovl_dentry_init_reval(dentry, upperdentry);
+	ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
 
 	revert_creds(old_cred);
 	if (origin_path) {
@@ -1135,7 +1135,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	return d_splice_alias(inode, dentry);
 
 out_free_oe:
-	dentry->d_fsdata = NULL;
 	ovl_free_entry(oe);
 out_put:
 	dput(index);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6a50296fef8f..e14ca0fd1063 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -381,9 +381,10 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 void ovl_free_entry(struct ovl_entry *oe);
 bool ovl_dentry_remote(struct dentry *dentry);
 void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *realdentry);
-void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry);
+void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry,
+			   struct ovl_entry *oe);
 void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
-			   unsigned int mask);
+			   struct ovl_entry *oe, unsigned int mask);
 bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
@@ -653,6 +654,7 @@ struct ovl_inode_params {
 	struct inode *newinode;
 	struct dentry *upperdentry;
 	struct ovl_path *lowerpath;
+	struct ovl_entry *oe;
 	bool index;
 	unsigned int numlower;
 	char *redirect;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 608742f60037..f511ac78c5bd 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -47,6 +47,11 @@ struct ovl_path {
 	struct dentry *dentry;
 };
 
+struct ovl_entry {
+	unsigned int __numlower;
+	struct ovl_path __lowerstack[];
+};
+
 /* private information held for overlayfs's superblock */
 struct ovl_fs {
 	unsigned int numlayer;
@@ -105,18 +110,6 @@ static inline bool ovl_should_sync(struct ovl_fs *ofs)
 	return !ofs->config.ovl_volatile;
 }
 
-/* private information held for every overlayfs dentry */
-struct ovl_entry {
-	union {
-		struct {
-			unsigned long flags;
-		};
-		struct rcu_head rcu;
-	};
-	unsigned int __numlower;
-	struct ovl_path __lowerstack[];
-};
-
 static inline unsigned int ovl_numlower(struct ovl_entry *oe)
 {
 	return oe ? oe->__numlower : 0;
@@ -127,14 +120,10 @@ static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
 	return ovl_numlower(oe) ? oe->__lowerstack : NULL;
 }
 
-static inline struct ovl_entry *OVL_E(struct dentry *dentry)
-{
-	return (struct ovl_entry *) dentry->d_fsdata;
-}
-
+/* private information held for every overlayfs dentry */
 static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
 {
-	return &OVL_E(dentry)->flags;
+	return (unsigned long *) &dentry->d_fsdata;
 }
 
 struct ovl_inode {
@@ -148,6 +137,7 @@ struct ovl_inode {
 	struct inode vfs_inode;
 	struct dentry *__upperdentry;
 	struct ovl_path lowerpath;
+	struct ovl_entry *oe;
 
 	/* synchronize copy up and more */
 	struct mutex lock;
@@ -158,6 +148,16 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
 	return container_of(inode, struct ovl_inode, vfs_inode);
 }
 
+static inline struct ovl_entry *OVL_I_E(struct inode *inode)
+{
+	return inode ? OVL_I(inode)->oe : NULL;
+}
+
+static inline struct ovl_entry *OVL_E(struct dentry *dentry)
+{
+	return OVL_I_E(d_inode(dentry));
+}
+
 static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
 {
 	return READ_ONCE(oi->__upperdentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d8fe857bd7e1..b9e62ccd609f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -59,16 +59,6 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
 MODULE_PARM_DESC(metacopy,
 		 "Default to on or off for the metadata only copy up feature");
 
-static void ovl_dentry_release(struct dentry *dentry)
-{
-	struct ovl_entry *oe = dentry->d_fsdata;
-
-	if (oe) {
-		ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
-		kfree_rcu(oe, rcu);
-	}
-}
-
 static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode)
 {
@@ -162,7 +152,6 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
 }
 
 static const struct dentry_operations ovl_dentry_operations = {
-	.d_release = ovl_dentry_release,
 	.d_real = ovl_d_real,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
@@ -182,6 +171,7 @@ static struct inode *ovl_alloc_inode(struct super_block *sb)
 	oi->version = 0;
 	oi->flags = 0;
 	oi->__upperdentry = NULL;
+	oi->oe = NULL;
 	oi->lowerpath.dentry = NULL;
 	oi->lowerpath.layer = NULL;
 	oi->lowerdata = NULL;
@@ -205,6 +195,7 @@ static void ovl_destroy_inode(struct inode *inode)
 
 	dput(oi->__upperdentry);
 	dput(oi->lowerpath.dentry);
+	ovl_free_entry(oi->oe);
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
 	else
@@ -1857,14 +1848,13 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 	struct ovl_inode_params oip = {
 		.upperdentry = upperdentry,
 		.lowerpath = lowerpath,
+		.oe = oe,
 	};
 
 	root = d_make_root(ovl_new_inode(sb, S_IFDIR, 0));
 	if (!root)
 		return NULL;
 
-	root->d_fsdata = oe;
-
 	if (upperdentry) {
 		/* Root inode uses upper st_ino/i_ino */
 		ino = d_inode(upperdentry)->i_ino;
@@ -1879,7 +1869,7 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 	ovl_dentry_set_flag(OVL_E_CONNECTED, root);
 	ovl_set_upperdata(d_inode(root));
 	ovl_inode_init(d_inode(root), &oip, ino, fsid);
-	ovl_dentry_init_flags(root, upperdentry, DCACHE_OP_WEAK_REVALIDATE);
+	ovl_dentry_init_flags(root, upperdentry, oe, DCACHE_OP_WEAK_REVALIDATE);
 
 	return root;
 }
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 1ba6dbea808c..f5e2c70a57f8 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -145,15 +145,15 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *realdentry)
 	spin_unlock(&dentry->d_lock);
 }
 
-void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry)
+void ovl_dentry_init_reval(struct dentry *dentry, struct dentry *upperdentry,
+			   struct ovl_entry *oe)
 {
-	return ovl_dentry_init_flags(dentry, upperdentry, OVL_D_REVALIDATE);
+	return ovl_dentry_init_flags(dentry, upperdentry, oe, OVL_D_REVALIDATE);
 }
 
 void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
-			   unsigned int mask)
+			   struct ovl_entry *oe, unsigned int mask)
 {
-	struct ovl_entry *oe = OVL_E(dentry);
 	struct ovl_path *lowerstack = ovl_lowerstack(oe);
 	unsigned int i, flags = 0;
 
-- 
2.34.1

[PATCH v2 06/13] ovl: deduplicate lowerpath and lowerstack[]

[Amir Goldstein]

The ovl_inode contains a copy of lowerpath in lowerstack[0], so the
lowerpath member can be removed.

Use accessor ovl_lowerpath() to get the lowerpath whereever the member
was accessed directly.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/export.c    |  2 --
 fs/overlayfs/inode.c     |  8 ++------
 fs/overlayfs/namei.c     |  2 --
 fs/overlayfs/overlayfs.h |  2 --
 fs/overlayfs/ovl_entry.h |  6 +++++-
 fs/overlayfs/super.c     |  4 ----
 fs/overlayfs/util.c      | 10 ++++++----
 7 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index be142ea73fad..35680b6e175b 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -289,9 +289,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	struct inode *inode = NULL;
 	struct ovl_entry *oe;
 	struct ovl_inode_params oip = {
-		.lowerpath = lowerpath,
 		.index = index,
-		.numlower = !!lower
 	};
 
 	/* We get overlay directory dentries with ovl_lookup_real() */
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index c296bd656858..f2ea51fac56b 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1006,10 +1006,6 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 	oi->__upperdentry = oip->upperdentry;
 	oi->oe = oip->oe;
 	oi->redirect = oip->redirect;
-	if (oip->lowerpath && oip->lowerpath->dentry) {
-		oi->lowerpath.dentry = dget(oip->lowerpath->dentry);
-		oi->lowerpath.layer = oip->lowerpath->layer;
-	}
 	if (oip->lowerdata)
 		oi->lowerdata = igrab(d_inode(oip->lowerdata));
 
@@ -1326,7 +1322,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 {
 	struct ovl_fs *ofs = OVL_FS(sb);
 	struct dentry *upperdentry = oip->upperdentry;
-	struct ovl_path *lowerpath = oip->lowerpath;
+	struct ovl_path *lowerpath = ovl_lowerpath(oip->oe);
 	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
 	struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
@@ -1405,7 +1401,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 
 	/* Check for non-merge dir that may have whiteouts */
 	if (is_dir) {
-		if (((upperdentry && lowerdentry) || oip->numlower > 1) ||
+		if (((upperdentry && lowerdentry) || ovl_numlower(oip->oe) > 1) ||
 		    ovl_path_check_origin_xattr(ofs, &realpath)) {
 			ovl_set_flag(OVL_WHITEOUTS, inode);
 		}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index a0a1c498dbd1..4f332d3fad37 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1105,10 +1105,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (upperdentry || ctr) {
 		struct ovl_inode_params oip = {
 			.upperdentry = upperdentry,
-			.lowerpath = stack,
 			.oe = oe,
 			.index = index,
-			.numlower = ctr,
 			.redirect = upperredirect,
 			.lowerdata = (ctr > 1 && !d.is_dir) ?
 				      stack[ctr - 1].dentry : NULL,
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e14ca0fd1063..2c61e4383cf6 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -653,10 +653,8 @@ bool ovl_is_private_xattr(struct super_block *sb, const char *name);
 struct ovl_inode_params {
 	struct inode *newinode;
 	struct dentry *upperdentry;
-	struct ovl_path *lowerpath;
 	struct ovl_entry *oe;
 	bool index;
-	unsigned int numlower;
 	char *redirect;
 	struct dentry *lowerdata;
 };
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index f511ac78c5bd..0bb8ab3aa8a7 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -120,6 +120,11 @@ static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
 	return ovl_numlower(oe) ? oe->__lowerstack : NULL;
 }
 
+static inline struct ovl_path *ovl_lowerpath(struct ovl_entry *oe)
+{
+	return ovl_lowerstack(oe);
+}
+
 /* private information held for every overlayfs dentry */
 static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
 {
@@ -136,7 +141,6 @@ struct ovl_inode {
 	unsigned long flags;
 	struct inode vfs_inode;
 	struct dentry *__upperdentry;
-	struct ovl_path lowerpath;
 	struct ovl_entry *oe;
 
 	/* synchronize copy up and more */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b9e62ccd609f..e3976ce279a8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -172,8 +172,6 @@ static struct inode *ovl_alloc_inode(struct super_block *sb)
 	oi->flags = 0;
 	oi->__upperdentry = NULL;
 	oi->oe = NULL;
-	oi->lowerpath.dentry = NULL;
-	oi->lowerpath.layer = NULL;
 	oi->lowerdata = NULL;
 	mutex_init(&oi->lock);
 
@@ -194,7 +192,6 @@ static void ovl_destroy_inode(struct inode *inode)
 	struct ovl_inode *oi = OVL_I(inode);
 
 	dput(oi->__upperdentry);
-	dput(oi->lowerpath.dentry);
 	ovl_free_entry(oi->oe);
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
@@ -1847,7 +1844,6 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 	int fsid = lowerpath->layer->fsid;
 	struct ovl_inode_params oip = {
 		.upperdentry = upperdentry,
-		.lowerpath = lowerpath,
 		.oe = oe,
 	};
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f5e2c70a57f8..21b2f479a46f 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -306,10 +306,12 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
 
 void ovl_i_path_real(struct inode *inode, struct path *path)
 {
+	struct ovl_path *lowerpath = ovl_lowerpath(OVL_I_E(inode));
+
 	path->dentry = ovl_i_dentry_upper(inode);
 	if (!path->dentry) {
-		path->dentry = OVL_I(inode)->lowerpath.dentry;
-		path->mnt = OVL_I(inode)->lowerpath.layer->mnt;
+		path->dentry = lowerpath->dentry;
+		path->mnt = lowerpath->layer->mnt;
 	} else {
 		path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
 	}
@@ -324,9 +326,9 @@ struct inode *ovl_inode_upper(struct inode *inode)
 
 struct inode *ovl_inode_lower(struct inode *inode)
 {
-	struct dentry *lowerdentry = OVL_I(inode)->lowerpath.dentry;
+	struct ovl_path *lowerpath = ovl_lowerpath(OVL_I_E(inode));
 
-	return lowerdentry ? d_inode(lowerdentry) : NULL;
+	return lowerpath ? d_inode(lowerpath->dentry) : NULL;
 }
 
 struct inode *ovl_inode_real(struct inode *inode)
-- 
2.34.1

[PATCH v2 07/13] ovl: deduplicate lowerdata and lowerstack[]

[Amir Goldstein]

The ovl_inode contains a copy of lowerdata in lowerstack[], so the
lowerdata inode member can be removed.

Use accessors ovl_lowerdata*() to get the lowerdata whereever the member
was accessed directly.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     |  2 --
 fs/overlayfs/namei.c     |  2 --
 fs/overlayfs/overlayfs.h |  1 -
 fs/overlayfs/ovl_entry.h | 16 +++++++++++++++-
 fs/overlayfs/super.c     |  3 ---
 fs/overlayfs/util.c      | 15 +++++++--------
 6 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index f2ea51fac56b..62027e60a936 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1006,8 +1006,6 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 	oi->__upperdentry = oip->upperdentry;
 	oi->oe = oip->oe;
 	oi->redirect = oip->redirect;
-	if (oip->lowerdata)
-		oi->lowerdata = igrab(d_inode(oip->lowerdata));
 
 	realinode = ovl_inode_real(inode);
 	ovl_copyattr(inode);
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 4f332d3fad37..e2b3c8f6753a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1108,8 +1108,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			.oe = oe,
 			.index = index,
 			.redirect = upperredirect,
-			.lowerdata = (ctr > 1 && !d.is_dir) ?
-				      stack[ctr - 1].dentry : NULL,
 		};
 
 		inode = ovl_get_inode(dentry->d_sb, &oip);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 2c61e4383cf6..93ebc0edf4ef 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -656,7 +656,6 @@ struct ovl_inode_params {
 	struct ovl_entry *oe;
 	bool index;
 	char *redirect;
-	struct dentry *lowerdata;
 };
 void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 		    unsigned long ino, int fsid);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 0bb8ab3aa8a7..548c93e030fc 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -125,6 +125,20 @@ static inline struct ovl_path *ovl_lowerpath(struct ovl_entry *oe)
 	return ovl_lowerstack(oe);
 }
 
+static inline struct ovl_path *ovl_lowerdata(struct ovl_entry *oe)
+{
+	struct ovl_path *lowerstack = ovl_lowerstack(oe);
+
+	return lowerstack ? &lowerstack[oe->__numlower - 1] : NULL;
+}
+
+static inline struct dentry *ovl_lowerdata_dentry(struct ovl_entry *oe)
+{
+	struct ovl_path *lowerdata = ovl_lowerdata(oe);
+
+	return lowerdata ? lowerdata->dentry : NULL;
+}
+
 /* private information held for every overlayfs dentry */
 static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
 {
@@ -134,7 +148,7 @@ static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
 struct ovl_inode {
 	union {
 		struct ovl_dir_cache *cache;	/* directory */
-		struct inode *lowerdata;	/* regular file */
+		/* place holder for non-dir */	/* regular file */
 	};
 	const char *redirect;
 	u64 version;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e3976ce279a8..1bfbdce2209a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -172,7 +172,6 @@ static struct inode *ovl_alloc_inode(struct super_block *sb)
 	oi->flags = 0;
 	oi->__upperdentry = NULL;
 	oi->oe = NULL;
-	oi->lowerdata = NULL;
 	mutex_init(&oi->lock);
 
 	return &oi->vfs_inode;
@@ -195,8 +194,6 @@ static void ovl_destroy_inode(struct inode *inode)
 	ovl_free_entry(oi->oe);
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
-	else
-		iput(oi->lowerdata);
 }
 
 static void ovl_free_fs(struct ovl_fs *ofs)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 21b2f479a46f..7495b4d378b0 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -225,11 +225,11 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
-	struct ovl_path *lowerstack = ovl_lowerstack(oe);
+	struct ovl_path *lowerdata = ovl_lowerdata(oe);
 
 	if (ovl_numlower(oe)) {
-		path->mnt = lowerstack[ovl_numlower(oe) - 1].layer->mnt;
-		path->dentry = lowerstack[ovl_numlower(oe) - 1].dentry;
+		path->mnt = lowerdata->layer->mnt;
+		path->dentry = lowerdata->dentry;
 	} else {
 		*path = (struct path) { };
 	}
@@ -288,10 +288,7 @@ const struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
  */
 struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
 {
-	struct ovl_entry *oe = OVL_E(dentry);
-
-	return ovl_numlower(oe) ?
-		ovl_lowerstack(oe)[ovl_numlower(oe) - 1].dentry : NULL;
+	return ovl_lowerdata_dentry(OVL_E(dentry));
 }
 
 struct dentry *ovl_dentry_real(struct dentry *dentry)
@@ -339,10 +336,12 @@ struct inode *ovl_inode_real(struct inode *inode)
 /* Return inode which contains lower data. Do not return metacopy */
 struct inode *ovl_inode_lowerdata(struct inode *inode)
 {
+	struct dentry *lowerdata = ovl_lowerdata_dentry(OVL_I_E(inode));
+
 	if (WARN_ON(!S_ISREG(inode->i_mode)))
 		return NULL;
 
-	return OVL_I(inode)->lowerdata ?: ovl_inode_lower(inode);
+	return lowerdata ? d_inode(lowerdata) : NULL;
 }
 
 /* Return real inode which contains data. Does not return metacopy inode */
-- 
2.34.1

[PATCH v2 08/13] ovl: remove unneeded goto instructions

[Amir Goldstein]

There is nothing in the out goto target of ovl_get_layers().

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 1bfbdce2209a..9b326b857ad6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1583,10 +1583,9 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	int err;
 	unsigned int i;
 
-	err = -ENOMEM;
 	ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
 	if (ofs->fs == NULL)
-		goto out;
+		return -ENOMEM;
 
 	/* idx/fsid 0 are reserved for upper fs even with lower only overlay */
 	ofs->numfs++;
@@ -1600,7 +1599,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	err = get_anon_bdev(&ofs->fs[0].pseudo_dev);
 	if (err) {
 		pr_err("failed to get anonymous bdev for upper fs\n");
-		goto out;
+		return err;
 	}
 
 	if (ovl_upper_mnt(ofs)) {
@@ -1613,9 +1612,9 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		struct inode *trap;
 		int fsid;
 
-		err = fsid = ovl_get_fsid(ofs, &stack[i]);
-		if (err < 0)
-			goto out;
+		fsid = ovl_get_fsid(ofs, &stack[i]);
+		if (fsid < 0)
+			return fsid;
 
 		/*
 		 * Check if lower root conflicts with this overlay layers before
@@ -1626,13 +1625,13 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		 */
 		err = ovl_setup_trap(sb, stack[i].dentry, &trap, "lowerdir");
 		if (err)
-			goto out;
+			return err;
 
 		if (ovl_is_inuse(stack[i].dentry)) {
 			err = ovl_report_in_use(ofs, "lowerdir");
 			if (err) {
 				iput(trap);
-				goto out;
+				return err;
 			}
 		}
 
@@ -1641,7 +1640,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		if (IS_ERR(mnt)) {
 			pr_err("failed to clone lowerpath\n");
 			iput(trap);
-			goto out;
+			return err;
 		}
 
 		/*
@@ -1691,9 +1690,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 			ofs->xino_mode);
 	}
 
-	err = 0;
-out:
-	return err;
+	return 0;
 }
 
 static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
-- 
2.34.1

[PATCH v2 09/13] ovl: introduce data-only lower layers

[Amir Goldstein]

Introduce the format lowerdir=lower1:lower2::lowerdata1:lowerdata2
where the lower layers on the right of the :: separator are not merged
into the overlayfs merge dirs.

The files in those layers are only meant to be accessible via absolute
redirect from metacopy files in lower layers.  Following changes will
implement lookup in the data layers.

This feature was requested for composefs ostree use case, where the
lower data layer should only be accessiable via absolute redirects
from metacopy inodes.

The lower data layers are not required to a have a unique uuid or any
uuid at all, because they are never used to compose the overlayfs inode
st_ino/st_dev.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst | 36 +++++++++++++++++++
 fs/overlayfs/namei.c                    |  2 +-
 fs/overlayfs/ovl_entry.h                |  9 +++++
 fs/overlayfs/super.c                    | 46 +++++++++++++++++++++----
 4 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 4c76fda07645..bc95343bafba 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -371,6 +371,42 @@ conflict with metacopy=on, and will result in an error.
 [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
 given.
 
+
+Data-only lower layers
+----------------------
+
+With "metacopy" feature enabled, an overlayfs regular file may be a composition
+of information from up to three different layers:
+
+ 1) metadata from a file in the upper layer
+
+ 2) st_ino and st_dev object identifier from a file in a lower layer
+
+ 3) data from a file in another lower layer (further below)
+
+The "lower data" file can be on any lower layer, except from the top most
+lower layer.
+
+Below the top most lower layer, any number of lower most layers may be defined
+as "data-only" lower layers, using the double colon ("::") separator.
+The double colon ("::") separator can only occur once and it must have a
+non-empty list of lower directory paths on the left and a non-empty
+list of "data-only" lower directory paths on the right.
+
+
+For example:
+
+  mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1:/do2 /merged
+
+The paths of files in the "data-only" lower layers are not visible in the
+merged overlayfs directories and the metadata and st_ino/st_dev of files
+in the "data-only" lower layers are not visible in overlayfs inodes.
+
+Only the data of the files in the "data-only" lower layers may be visible
+when a "metacopy" file in one of the lower layers above it, has a "redirect"
+to the absolute path of the "lower data" file in the "data-only" lower layer.
+
+
 Sharing and copying layers
 --------------------------
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e2b3c8f6753a..6bb07e1c01ee 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 	struct dentry *origin = NULL;
 	int i;
 
-	for (i = 1; i < ofs->numlayer; i++) {
+	for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
 		/*
 		 * If lower fs uuid is not unique among lower fs we cannot match
 		 * fh->uuid to layer.
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 548c93e030fc..93ff299da0dd 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -57,6 +57,8 @@ struct ovl_fs {
 	unsigned int numlayer;
 	/* Number of unique fs among layers including upper fs */
 	unsigned int numfs;
+	/* Number of data-only lower layers */
+	unsigned int numdatalayer;
 	const struct ovl_layer *layers;
 	struct ovl_sb *fs;
 	/* workbasedir is the path at workdir= mount option */
@@ -90,6 +92,13 @@ struct ovl_fs {
 	errseq_t errseq;
 };
 
+
+/* Number of lower layers, not including data-only layers */
+static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
+{
+	return ofs->numlayer - ofs->numdatalayer - 1;
+}
+
 static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
 {
 	return ofs->layers[0].mnt;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 9b326b857ad6..988edb9e9d23 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1576,6 +1576,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 	return ofs->numfs++;
 }
 
+/*
+ * The fsid after the last lower fsid is used for the data layers.
+ * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
+ */
+static int ovl_get_data_fsid(struct ovl_fs *ofs)
+{
+	return ofs->numfs;
+}
+
+
 static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 			  struct path *stack, unsigned int numlower,
 			  struct ovl_layer *layers)
@@ -1583,11 +1593,14 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	int err;
 	unsigned int i;
 
-	ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
+	ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb), GFP_KERNEL);
 	if (ofs->fs == NULL)
 		return -ENOMEM;
 
-	/* idx/fsid 0 are reserved for upper fs even with lower only overlay */
+	/*
+	 * idx/fsid 0 are reserved for upper fs even with lower only overlay
+	 * and the last fsid is reserved for "null fs" of the data layers.
+	 */
 	ofs->numfs++;
 
 	/*
@@ -1612,7 +1625,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		struct inode *trap;
 		int fsid;
 
-		fsid = ovl_get_fsid(ofs, &stack[i]);
+		if (i < numlower - ofs->numdatalayer)
+			fsid = ovl_get_fsid(ofs, &stack[i]);
+		else
+			fsid = ovl_get_data_fsid(ofs);
 		if (fsid < 0)
 			return fsid;
 
@@ -1700,6 +1716,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	int err;
 	struct path *stack = NULL;
 	struct ovl_path *lowerstack;
+	unsigned int numlowerdata = 0;
 	unsigned int i;
 	struct ovl_entry *oe;
 
@@ -1712,13 +1729,27 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (!stack)
 		return ERR_PTR(-ENOMEM);
 
-	err = -EINVAL;
-	for (i = 0; i < numlower; i++) {
+	for (i = 0; i < numlower;) {
 		err = ovl_lower_dir(lower, &stack[i], ofs, &sb->s_stack_depth);
 		if (err)
 			goto out_err;
 
 		lower = strchr(lower, '\0') + 1;
+
+		i++;
+		err = -EINVAL;
+		/* :: separator indicates the start of lower data layers */
+		if (!*lower && i < numlower && !numlowerdata) {
+			if (!ofs->config.metacopy) {
+				pr_err("lower data-only dirs require metacopy support.\n");
+				goto out_err;
+			}
+			lower++;
+			numlower--;
+			ofs->numdatalayer = numlowerdata = numlower - i;
+			pr_info("using the lowest %d of %d lowerdirs as data layers\n",
+				numlowerdata, numlower);
+		}
 	}
 
 	err = -EINVAL;
@@ -1733,12 +1764,13 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		goto out_err;
 
 	err = -ENOMEM;
-	oe = ovl_alloc_entry(numlower);
+	/* Data-only layers are not merged in root directory */
+	oe = ovl_alloc_entry(numlower - numlowerdata);
 	if (!oe)
 		goto out_err;
 
 	lowerstack = ovl_lowerstack(oe);
-	for (i = 0; i < numlower; i++) {
+	for (i = 0; i < numlower - numlowerdata; i++) {
 		lowerstack[i].dentry = dget(stack[i].dentry);
 		lowerstack[i].layer = &ofs->layers[i+1];
 	}
-- 
2.34.1

[PATCH v2 10/13] ovl: implement lookup in data-only layers

[Amir Goldstein]

Lookup in data-only layers only for a lower metacopy with an absolute
redirect xattr.

The metacopy xattr is not checked on files found in the data-only layers
and redirect xattr are not followed in the data-only layers.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 73 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6bb07e1c01ee..1ed64ff129d9 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -14,6 +14,8 @@
 #include <linux/exportfs.h>
 #include "overlayfs.h"
 
+#include "../internal.h"	/* for vfs_path_lookup */
+
 struct ovl_lookup_data {
 	struct super_block *sb;
 	struct vfsmount *mnt;
@@ -24,6 +26,8 @@ struct ovl_lookup_data {
 	bool last;
 	char *redirect;
 	bool metacopy;
+	/* Referring to last redirect xattr */
+	bool absolute_redirect;
 };
 
 static int ovl_check_redirect(const struct path *path, struct ovl_lookup_data *d,
@@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path *path, struct ovl_lookup_data *d
 	char *buf;
 	struct ovl_fs *ofs = OVL_FS(d->sb);
 
+	d->absolute_redirect = false;
 	buf = ovl_get_redirect_xattr(ofs, path, prelen + strlen(post));
 	if (IS_ERR_OR_NULL(buf))
 		return PTR_ERR(buf);
 
 	if (buf[0] == '/') {
+		d->absolute_redirect = true;
 		/*
 		 * One of the ancestor path elements in an absolute path
 		 * lookup in ovl_lookup_layer() could have been opaque and
@@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 	return 0;
 }
 
+static int ovl_lookup_data_layer(struct dentry *dentry, const char *redirect,
+				 const struct ovl_layer *layer,
+				 struct path *datapath)
+{
+	int err;
+
+	err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt, redirect,
+			LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS | LOOKUP_NO_XDEV,
+			datapath);
+	pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d, err=%i)\n",
+		 dentry, redirect, layer->idx, err);
+
+	if (err)
+		return err;
+
+	err = -EREMOTE;
+	if (ovl_dentry_weird(datapath->dentry))
+		goto out_path_put;
+
+	err = -ENOENT;
+	/* Only regular file is acceptable as lower data */
+	if (!d_is_reg(datapath->dentry))
+		goto out_path_put;
+
+	return 0;
+
+out_path_put:
+	path_put(datapath);
+
+	return err;
+}
+
+/* Lookup in data-only layers by absolute redirect to layer root */
+static int ovl_lookup_data_layers(struct dentry *dentry, const char *redirect,
+				  struct ovl_path *lowerdata)
+{
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
+	const struct ovl_layer *layer;
+	struct path datapath;
+	int err = -ENOENT;
+	int i;
+
+	layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
+	for (i = 0; i < ofs->numdatalayer; i++, layer++) {
+		err = ovl_lookup_data_layer(dentry, redirect, layer, &datapath);
+		if (!err) {
+			mntput(datapath.mnt);
+			lowerdata->dentry = datapath.dentry;
+			lowerdata->layer = layer;
+			return 0;
+		}
+	}
+
+	return err;
+}
 
 int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 			struct dentry *upperdentry, struct ovl_path **stackp)
@@ -917,7 +978,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 		if (!ofs->config.redirect_follow)
 			d.last = i == ovl_numlower(poe) - 1;
-		else
+		else if (d.is_dir || !ofs->numdatalayer)
 			d.last = lower.layer->idx == ovl_numlower(roe);
 
 		d.mnt = lower.layer->mnt;
@@ -1011,6 +1072,16 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
+	/* Lookup absolute redirect from lower metacopy in data-only layers */
+	if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
+		err = ovl_lookup_data_layers(dentry, d.redirect,
+					     &stack[ctr]);
+		if (!err) {
+			d.metacopy = false;
+			ctr++;
+		}
+	}
+
 	/*
 	 * For regular non-metacopy upper dentries, there is no lower
 	 * path based lookup, hence ctr will be zero. If a dentry is found
-- 
2.34.1

[PATCH v2 11/13] ovl: prepare to store lowerdata redirect for lazy lowerdata lookup

[Amir Goldstein]

Prepare to allow ovl_lookup() to leave the last entry in a non-dir
lowerstack empty to signify lazy lowerdata lookup.

In this case, ovl_lookup() stores the redirect path from metacopy to
lowerdata in ovl_inode, which is going to be used later to perform the
lazy lowerdata lookup.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     |  2 ++
 fs/overlayfs/namei.c     |  5 +++++
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  3 ++-
 fs/overlayfs/super.c     |  3 +++
 fs/overlayfs/util.c      | 13 ++++++++++---
 6 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 62027e60a936..a7529b6a86e6 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1006,6 +1006,7 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 	oi->__upperdentry = oip->upperdentry;
 	oi->oe = oip->oe;
 	oi->redirect = oip->redirect;
+	oi->lowerdata_redirect = oip->lowerdata_redirect;
 
 	realinode = ovl_inode_real(inode);
 	ovl_copyattr(inode);
@@ -1366,6 +1367,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			dput(upperdentry);
 			ovl_free_entry(oip->oe);
 			kfree(oip->redirect);
+			kfree(oip->lowerdata_redirect);
 			goto out;
 		}
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 1ed64ff129d9..6df9a349cd04 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1181,6 +1181,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			.redirect = upperredirect,
 		};
 
+		/* Store lowerdata redirect for lazy lookup */
+		if (ctr > 1 && !d.is_dir && !stack[ctr - 1].dentry) {
+			oip.lowerdata_redirect = d.redirect;
+			d.redirect = NULL;
+		}
 		inode = ovl_get_inode(dentry->d_sb, &oip);
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 93ebc0edf4ef..cb0135ff6249 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -405,6 +405,7 @@ struct inode *ovl_inode_lower(struct inode *inode);
 struct inode *ovl_inode_lowerdata(struct inode *inode);
 struct inode *ovl_inode_real(struct inode *inode);
 struct inode *ovl_inode_realdata(struct inode *inode);
+const char *ovl_lowerdata_redirect(struct inode *inode);
 struct ovl_dir_cache *ovl_dir_cache(struct inode *inode);
 void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache);
 void ovl_dentry_set_flag(unsigned long flag, struct dentry *dentry);
@@ -656,6 +657,7 @@ struct ovl_inode_params {
 	struct ovl_entry *oe;
 	bool index;
 	char *redirect;
+	char *lowerdata_redirect;
 };
 void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 		    unsigned long ino, int fsid);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 93ff299da0dd..513c2c499e41 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -141,6 +141,7 @@ static inline struct ovl_path *ovl_lowerdata(struct ovl_entry *oe)
 	return lowerstack ? &lowerstack[oe->__numlower - 1] : NULL;
 }
 
+/* May return NULL if lazy lookup of lowerdata is needed */
 static inline struct dentry *ovl_lowerdata_dentry(struct ovl_entry *oe)
 {
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
@@ -157,7 +158,7 @@ static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
 struct ovl_inode {
 	union {
 		struct ovl_dir_cache *cache;	/* directory */
-		/* place holder for non-dir */	/* regular file */
+		const char *lowerdata_redirect;	/* regular file */
 	};
 	const char *redirect;
 	u64 version;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 988edb9e9d23..b960b9d84b66 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -171,6 +171,7 @@ static struct inode *ovl_alloc_inode(struct super_block *sb)
 	oi->version = 0;
 	oi->flags = 0;
 	oi->__upperdentry = NULL;
+	oi->lowerdata_redirect = NULL;
 	oi->oe = NULL;
 	mutex_init(&oi->lock);
 
@@ -194,6 +195,8 @@ static void ovl_destroy_inode(struct inode *inode)
 	ovl_free_entry(oi->oe);
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
+	else
+		kfree(oi->lowerdata_redirect);
 }
 
 static void ovl_free_fs(struct ovl_fs *ofs)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7495b4d378b0..ad93a3132495 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -226,10 +226,11 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
+	struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
 
-	if (ovl_numlower(oe)) {
+	if (lowerdata_dentry) {
 		path->mnt = lowerdata->layer->mnt;
-		path->dentry = lowerdata->dentry;
+		path->dentry = lowerdata_dentry;
 	} else {
 		*path = (struct path) { };
 	}
@@ -356,9 +357,15 @@ struct inode *ovl_inode_realdata(struct inode *inode)
 	return ovl_inode_lowerdata(inode);
 }
 
+const char *ovl_lowerdata_redirect(struct inode *inode)
+{
+	return inode && S_ISREG(inode->i_mode) ?
+		OVL_I(inode)->lowerdata_redirect : NULL;
+}
+
 struct ovl_dir_cache *ovl_dir_cache(struct inode *inode)
 {
-	return OVL_I(inode)->cache;
+	return inode && S_ISDIR(inode->i_mode) ? OVL_I(inode)->cache : NULL;
 }
 
 void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache)
-- 
2.34.1

[PATCH v2 12/13] ovl: prepare for lazy lookup of lowerdata inode

[Amir Goldstein]

Make the code handle the case of numlower > 1 and missing lowerdata
dentry gracefully.

Missing lowerdata dentry is an indication for lazy lookup of lowerdata
and in that case the lowerdata_redirect path is stored in ovl_inode.

Following commits will defer lookup and perform the lazy lookup on
access.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c  |  7 +++++++
 fs/overlayfs/inode.c | 18 ++++++++++++++----
 fs/overlayfs/super.c | 11 +++++++++++
 fs/overlayfs/util.c  |  2 +-
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 7c04f033aadd..951683a66ff6 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -115,6 +115,9 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 		ovl_path_real(dentry, &realpath);
 	else
 		ovl_path_realdata(dentry, &realpath);
+	/* TODO: lazy lookup of lowerdata */
+	if (!realpath.dentry)
+		return -EIO;
 
 	/* Has it been copied up since we'd opened it? */
 	if (unlikely(file_inode(real->file) != d_inode(realpath.dentry))) {
@@ -158,6 +161,10 @@ static int ovl_open(struct inode *inode, struct file *file)
 	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	ovl_path_realdata(dentry, &realpath);
+	/* TODO: lazy lookup of lowerdata */
+	if (!realpath.dentry)
+		return -EIO;
+
 	realfile = ovl_open_realfile(file, &realpath);
 	if (IS_ERR(realfile))
 		return PTR_ERR(realfile);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index a7529b6a86e6..ebbe13bccc97 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -240,15 +240,22 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 			/*
 			 * If lower is not same as lowerdata or if there was
 			 * no origin on upper, we can end up here.
+			 * With lazy lowerdata lookup, guess lowerdata blocks
+			 * from size to avoid lowerdata lookup on stat(2).
 			 */
 			struct kstat lowerdatastat;
 			u32 lowermask = STATX_BLOCKS;
 
 			ovl_path_lowerdata(dentry, &realpath);
-			err = vfs_getattr(&realpath, &lowerdatastat,
-					  lowermask, flags);
-			if (err)
-				goto out;
+			if (realpath.dentry) {
+				err = vfs_getattr(&realpath, &lowerdatastat,
+						  lowermask, flags);
+				if (err)
+					goto out;
+			} else {
+				lowerdatastat.blocks =
+					round_up(stat->size, stat->blksize) >> 9;
+			}
 			stat->blocks = lowerdatastat.blocks;
 		}
 	}
@@ -710,6 +717,9 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	struct inode *realinode = ovl_inode_realdata(inode);
 	const struct cred *old_cred;
 
+	if (!realinode)
+		return -EIO;
+
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b960b9d84b66..ad9a68bec565 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -81,6 +81,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	if (real && !inode && ovl_has_upperdata(d_inode(dentry)))
 		return real;
 
+	/*
+	 * XXX: We may need lazy lookup of lowerdata for !inode case to return
+	 * the real lowerdata dentry.  The only current caller of d_real() with
+	 * NULL inode is d_real_inode() from trace_uprobe and this caller is
+	 * likely going to be followed reading from the file, before placing
+	 * uprobes on offset within the file, so lowerdata should be available
+	 * when setting the uprobe.
+	 */
 	lower = ovl_dentry_lowerdata(dentry);
 	if (!lower)
 		goto bug;
@@ -103,6 +111,9 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 {
 	int ret = 1;
 
+	if (!d)
+		return 1;
+
 	if (weak) {
 		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
 			ret =  d->d_op->d_weak_revalidate(d, flags);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index ad93a3132495..9b7c0163734a 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -159,7 +159,7 @@ void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
 
 	if (upperdentry)
 		flags |= upperdentry->d_flags;
-	for (i = 0; i < ovl_numlower(oe); i++)
+	for (i = 0; i < ovl_numlower(oe) && lowerstack[i].dentry; i++)
 		flags |= lowerstack[i].dentry->d_flags;
 
 	spin_lock(&dentry->d_lock);
-- 
2.34.1

[PATCH v2 13/13] ovl: implement lazy lookup of lowerdata in data-only layers

[Amir Goldstein]

Defer lookup of lowerdata in the data-only layers to first data access
or before copy up.

We perform lowerdata lookup before copy up even if copy up is metadata
only copy up.  We can further optimize this lookup later if needed.

We do best effort lazy lookup of lowerdata for d_real_inode(), because
this interface does not expect errors.  The only current in-tree caller
of d_real_inode() is trace_uprobe and this caller is likely going to be
followed reading from the file, before placing uprobes on offset within
the file, so lowerdata should be available when setting the uprobe.

Reviewed-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  9 +++++++
 fs/overlayfs/file.c      | 18 ++++++++++---
 fs/overlayfs/namei.c     | 56 +++++++++++++++++++++++++++++++++++-----
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  2 +-
 fs/overlayfs/super.c     |  3 ++-
 fs/overlayfs/util.c      | 31 +++++++++++++++++++++-
 7 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7bf101e756c8..eb266fb68730 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1074,6 +1074,15 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	if (WARN_ON(disconnected && d_is_dir(dentry)))
 		return -EIO;
 
+	/*
+	 * We may not need lowerdata if we are only doing metacopy up, but it is
+	 * not very important to optimize this case, so do lazy lowerdata lookup
+	 * before any copy up, so we can do it before taking ovl_inode_lock().
+	 */
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
 	old_cred = ovl_override_creds(dentry->d_sb);
 	while (!err) {
 		struct dentry *next;
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 951683a66ff6..39737c2aaa84 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -107,15 +107,21 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 {
 	struct dentry *dentry = file_dentry(file);
 	struct path realpath;
+	int err;
 
 	real->flags = 0;
 	real->file = file->private_data;
 
-	if (allow_meta)
+	if (allow_meta) {
 		ovl_path_real(dentry, &realpath);
-	else
+	} else {
+		/* lazy lookup of lowerdata */
+		err = ovl_maybe_lookup_lowerdata(dentry);
+		if (err)
+			return err;
+
 		ovl_path_realdata(dentry, &realpath);
-	/* TODO: lazy lookup of lowerdata */
+	}
 	if (!realpath.dentry)
 		return -EIO;
 
@@ -153,6 +159,11 @@ static int ovl_open(struct inode *inode, struct file *file)
 	struct path realpath;
 	int err;
 
+	/* lazy lookup of lowerdata */
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
 	err = ovl_maybe_copy_up(dentry, file->f_flags);
 	if (err)
 		return err;
@@ -161,7 +172,6 @@ static int ovl_open(struct inode *inode, struct file *file)
 	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	ovl_path_realdata(dentry, &realpath);
-	/* TODO: lazy lookup of lowerdata */
 	if (!realpath.dentry)
 		return -EIO;
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6df9a349cd04..292b8a948f1a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -889,6 +889,52 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	return err;
 }
 
+/* Lazy lookup of lowerdata */
+int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
+{
+	struct inode *inode = d_inode(dentry);
+	const char *redirect = ovl_lowerdata_redirect(inode);
+	struct ovl_path datapath = {};
+	const struct cred *old_cred;
+	int err;
+
+	if (!redirect || ovl_dentry_lowerdata(dentry))
+		return 0;
+
+	if (redirect[0] != '/')
+		return -EIO;
+
+	err = ovl_inode_lock_interruptible(inode);
+	if (err)
+		return err;
+
+	err = 0;
+	/* Someone got here before us? */
+	if (ovl_dentry_lowerdata(dentry))
+		goto out;
+
+	old_cred = ovl_override_creds(dentry->d_sb);
+	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
+	revert_creds(old_cred);
+	if (err)
+		goto out_err;
+
+	err = ovl_dentry_set_lowerdata(dentry, &datapath);
+	if (err)
+		goto out_err;
+
+out:
+	ovl_inode_unlock(inode);
+	dput(datapath.dentry);
+
+	return err;
+
+out_err:
+	pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2, err=%i)\n",
+			    dentry, err);
+	goto out;
+}
+
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
@@ -1072,14 +1118,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	/* Lookup absolute redirect from lower metacopy in data-only layers */
+	/* Defer lookup of lowerdata in data-only layers to first access */
 	if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
-		err = ovl_lookup_data_layers(dentry, d.redirect,
-					     &stack[ctr]);
-		if (!err) {
-			d.metacopy = false;
-			ctr++;
-		}
+		d.metacopy = false;
+		ctr++;
 	}
 
 	/*
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index cb0135ff6249..c1233eec2d40 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -396,6 +396,7 @@ enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
+int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path *datapath);
 const struct ovl_layer *ovl_i_layer_lower(struct inode *inode);
 const struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
@@ -558,6 +559,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
 struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 				struct dentry *origin, bool verify);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
+int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 513c2c499e41..c6c7d09b494e 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -146,7 +146,7 @@ static inline struct dentry *ovl_lowerdata_dentry(struct ovl_entry *oe)
 {
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
 
-	return lowerdata ? lowerdata->dentry : NULL;
+	return lowerdata ? READ_ONCE(lowerdata->dentry) : NULL;
 }
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ad9a68bec565..c6209592bb3f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -82,13 +82,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 		return real;
 
 	/*
-	 * XXX: We may need lazy lookup of lowerdata for !inode case to return
+	 * Best effort lazy lookup of lowerdata for !inode case to return
 	 * the real lowerdata dentry.  The only current caller of d_real() with
 	 * NULL inode is d_real_inode() from trace_uprobe and this caller is
 	 * likely going to be followed reading from the file, before placing
 	 * uprobes on offset within the file, so lowerdata should be available
 	 * when setting the uprobe.
 	 */
+	ovl_maybe_lookup_lowerdata(dentry);
 	lower = ovl_dentry_lowerdata(dentry);
 	if (!lower)
 		goto bug;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 9b7c0163734a..e526ab059872 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -229,8 +229,14 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 	struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
 
 	if (lowerdata_dentry) {
-		path->mnt = lowerdata->layer->mnt;
 		path->dentry = lowerdata_dentry;
+		/*
+		 * Pairs with smp_wmb() in ovl_dentry_set_lowerdata().
+		 * Make sure that if lowerdata->dentry is visible, then
+		 * datapath->layer is visible as well.
+		 */
+		smp_rmb();
+		path->mnt = READ_ONCE(lowerdata->layer)->mnt;
 	} else {
 		*path = (struct path) { };
 	}
@@ -292,6 +298,29 @@ struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
 	return ovl_lowerdata_dentry(OVL_E(dentry));
 }
 
+int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path *datapath)
+{
+	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerdata = ovl_lowerdata(oe);
+	struct dentry *datadentry = datapath->dentry;
+
+	if (WARN_ON_ONCE(ovl_numlower(oe) <= 1))
+		return -EIO;
+
+	WRITE_ONCE(lowerdata->layer, datapath->layer);
+	/*
+	 * Pairs with smp_rmb() in ovl_path_lowerdata().
+	 * Make sure that if lowerdata->dentry is visible, then
+	 * lowerdata->layer is visible as well.
+	 */
+	smp_wmb();
+	WRITE_ONCE(lowerdata->dentry, dget(datadentry));
+
+	ovl_dentry_update_reval(dentry, datadentry);
+
+	return 0;
+}
+
 struct dentry *ovl_dentry_real(struct dentry *dentry)
 {
 	return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);
-- 
2.34.1

Re: [PATCH v2 09/13] ovl: introduce data-only lower layers

[Amir Goldstein]

On Thu, Apr 27, 2023 at 4:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Introduce the format lowerdir=lower1:lower2::lowerdata1:lowerdata2
> where the lower layers on the right of the :: separator are not merged
> into the overlayfs merge dirs.
>
> The files in those layers are only meant to be accessible via absolute
> redirect from metacopy files in lower layers.  Following changes will
> implement lookup in the data layers.
>
> This feature was requested for composefs ostree use case, where the
> lower data layer should only be accessiable via absolute redirects
> from metacopy inodes.
>
> The lower data layers are not required to a have a unique uuid or any
> uuid at all, because they are never used to compose the overlayfs inode
> st_ino/st_dev.
>
> Reviewed-by: Alexander Larsson <alexl@redhat.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/overlayfs.rst | 36 +++++++++++++++++++
>  fs/overlayfs/namei.c                    |  2 +-
>  fs/overlayfs/ovl_entry.h                |  9 +++++
>  fs/overlayfs/super.c                    | 46 +++++++++++++++++++++----
>  4 files changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 4c76fda07645..bc95343bafba 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -371,6 +371,42 @@ conflict with metacopy=on, and will result in an error.
>  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
>  given.
>
> +
> +Data-only lower layers
> +----------------------
> +
> +With "metacopy" feature enabled, an overlayfs regular file may be a composition
> +of information from up to three different layers:
> +
> + 1) metadata from a file in the upper layer
> +
> + 2) st_ino and st_dev object identifier from a file in a lower layer
> +
> + 3) data from a file in another lower layer (further below)
> +
> +The "lower data" file can be on any lower layer, except from the top most
> +lower layer.
> +
> +Below the top most lower layer, any number of lower most layers may be defined
> +as "data-only" lower layers, using the double colon ("::") separator.
> +The double colon ("::") separator can only occur once and it must have a
> +non-empty list of lower directory paths on the left and a non-empty
> +list of "data-only" lower directory paths on the right.
> +
> +
> +For example:
> +
> +  mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1:/do2 /merged
> +
> +The paths of files in the "data-only" lower layers are not visible in the
> +merged overlayfs directories and the metadata and st_ino/st_dev of files
> +in the "data-only" lower layers are not visible in overlayfs inodes.
> +
> +Only the data of the files in the "data-only" lower layers may be visible
> +when a "metacopy" file in one of the lower layers above it, has a "redirect"
> +to the absolute path of the "lower data" file in the "data-only" lower layer.
> +
> +
>  Sharing and copying layers
>  --------------------------
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index e2b3c8f6753a..6bb07e1c01ee 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>         struct dentry *origin = NULL;
>         int i;
>
> -       for (i = 1; i < ofs->numlayer; i++) {
> +       for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
>                 /*
>                  * If lower fs uuid is not unique among lower fs we cannot match
>                  * fh->uuid to layer.
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 548c93e030fc..93ff299da0dd 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -57,6 +57,8 @@ struct ovl_fs {
>         unsigned int numlayer;
>         /* Number of unique fs among layers including upper fs */
>         unsigned int numfs;
> +       /* Number of data-only lower layers */
> +       unsigned int numdatalayer;
>         const struct ovl_layer *layers;
>         struct ovl_sb *fs;
>         /* workbasedir is the path at workdir= mount option */
> @@ -90,6 +92,13 @@ struct ovl_fs {
>         errseq_t errseq;
>  };
>
> +
> +/* Number of lower layers, not including data-only layers */
> +static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
> +{
> +       return ofs->numlayer - ofs->numdatalayer - 1;
> +}
> +
>  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
>  {
>         return ofs->layers[0].mnt;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 9b326b857ad6..988edb9e9d23 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1576,6 +1576,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
>         return ofs->numfs++;
>  }
>
> +/*
> + * The fsid after the last lower fsid is used for the data layers.
> + * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
> + */
> +static int ovl_get_data_fsid(struct ovl_fs *ofs)
> +{
> +       return ofs->numfs;
> +}
> +
> +
>  static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
>                           struct path *stack, unsigned int numlower,
>                           struct ovl_layer *layers)
> @@ -1583,11 +1593,14 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
>         int err;
>         unsigned int i;
>
> -       ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
> +       ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb), GFP_KERNEL);
>         if (ofs->fs == NULL)
>                 return -ENOMEM;
>
> -       /* idx/fsid 0 are reserved for upper fs even with lower only overlay */
> +       /*
> +        * idx/fsid 0 are reserved for upper fs even with lower only overlay
> +        * and the last fsid is reserved for "null fs" of the data layers.
> +        */
>         ofs->numfs++;
>
>         /*
> @@ -1612,7 +1625,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
>                 struct inode *trap;
>                 int fsid;
>
> -               fsid = ovl_get_fsid(ofs, &stack[i]);
> +               if (i < numlower - ofs->numdatalayer)
> +                       fsid = ovl_get_fsid(ofs, &stack[i]);
> +               else
> +                       fsid = ovl_get_data_fsid(ofs);
>                 if (fsid < 0)
>                         return fsid;
>
> @@ -1700,6 +1716,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         int err;
>         struct path *stack = NULL;
>         struct ovl_path *lowerstack;
> +       unsigned int numlowerdata = 0;
>         unsigned int i;
>         struct ovl_entry *oe;
>
> @@ -1712,13 +1729,27 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         if (!stack)
>                 return ERR_PTR(-ENOMEM);
>
> -       err = -EINVAL;
> -       for (i = 0; i < numlower; i++) {
> +       for (i = 0; i < numlower;) {
>                 err = ovl_lower_dir(lower, &stack[i], ofs, &sb->s_stack_depth);
>                 if (err)
>                         goto out_err;
>
>                 lower = strchr(lower, '\0') + 1;
> +
> +               i++;
> +               err = -EINVAL;
> +               /* :: separator indicates the start of lower data layers */
> +               if (!*lower && i < numlower && !numlowerdata) {

FYI, kernel test bot reported a KASAN out-of-bounds access on !*lower
because it is tested also when i == numlower and in any case, the test
should be i < numlower - 1, so :: cannot be at the end of the list.

Pushed a fix to branch ovl-lazy-lowerdata with an improved comment:

                /*
                 * Empty lower layer path could mean :: separator that indicates
                 * the start of lower data layers.
                 * Only one :: separator is allowed and it has to have at least
                 * one lowerdir to the left and one lowerdir to the right.
                 */
                if (!numlowerdata && i < numlower - 1 && !*lower) {


Thanks,
Amir.

Re: [PATCH v2 09/13] ovl: introduce data-only lower layers

[Amir Goldstein]

On Sun, May 14, 2023 at 10:13 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Apr 27, 2023 at 4:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Introduce the format lowerdir=lower1:lower2::lowerdata1:lowerdata2
> > where the lower layers on the right of the :: separator are not merged
> > into the overlayfs merge dirs.
> >
> > The files in those layers are only meant to be accessible via absolute
> > redirect from metacopy files in lower layers.  Following changes will
> > implement lookup in the data layers.
> >
> > This feature was requested for composefs ostree use case, where the
> > lower data layer should only be accessiable via absolute redirects
> > from metacopy inodes.
> >
> > The lower data layers are not required to a have a unique uuid or any
> > uuid at all, because they are never used to compose the overlayfs inode
> > st_ino/st_dev.
> >
> > Reviewed-by: Alexander Larsson <alexl@redhat.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  Documentation/filesystems/overlayfs.rst | 36 +++++++++++++++++++
> >  fs/overlayfs/namei.c                    |  2 +-
> >  fs/overlayfs/ovl_entry.h                |  9 +++++
> >  fs/overlayfs/super.c                    | 46 +++++++++++++++++++++----
> >  4 files changed, 85 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > index 4c76fda07645..bc95343bafba 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -371,6 +371,42 @@ conflict with metacopy=on, and will result in an error.
> >  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
> >  given.
> >
> > +
> > +Data-only lower layers
> > +----------------------
> > +
> > +With "metacopy" feature enabled, an overlayfs regular file may be a composition
> > +of information from up to three different layers:
> > +
> > + 1) metadata from a file in the upper layer
> > +
> > + 2) st_ino and st_dev object identifier from a file in a lower layer
> > +
> > + 3) data from a file in another lower layer (further below)
> > +
> > +The "lower data" file can be on any lower layer, except from the top most
> > +lower layer.
> > +
> > +Below the top most lower layer, any number of lower most layers may be defined
> > +as "data-only" lower layers, using the double colon ("::") separator.
> > +The double colon ("::") separator can only occur once and it must have a
> > +non-empty list of lower directory paths on the left and a non-empty
> > +list of "data-only" lower directory paths on the right.
> > +
> > +
> > +For example:
> > +
> > +  mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1:/do2 /merged
> > +
> > +The paths of files in the "data-only" lower layers are not visible in the
> > +merged overlayfs directories and the metadata and st_ino/st_dev of files
> > +in the "data-only" lower layers are not visible in overlayfs inodes.
> > +
> > +Only the data of the files in the "data-only" lower layers may be visible
> > +when a "metacopy" file in one of the lower layers above it, has a "redirect"
> > +to the absolute path of the "lower data" file in the "data-only" lower layer.
> > +
> > +
> >  Sharing and copying layers
> >  --------------------------
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index e2b3c8f6753a..6bb07e1c01ee 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> >         struct dentry *origin = NULL;
> >         int i;
> >
> > -       for (i = 1; i < ofs->numlayer; i++) {
> > +       for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
> >                 /*
> >                  * If lower fs uuid is not unique among lower fs we cannot match
> >                  * fh->uuid to layer.
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index 548c93e030fc..93ff299da0dd 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -57,6 +57,8 @@ struct ovl_fs {
> >         unsigned int numlayer;
> >         /* Number of unique fs among layers including upper fs */
> >         unsigned int numfs;
> > +       /* Number of data-only lower layers */
> > +       unsigned int numdatalayer;
> >         const struct ovl_layer *layers;
> >         struct ovl_sb *fs;
> >         /* workbasedir is the path at workdir= mount option */
> > @@ -90,6 +92,13 @@ struct ovl_fs {
> >         errseq_t errseq;
> >  };
> >
> > +
> > +/* Number of lower layers, not including data-only layers */
> > +static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
> > +{
> > +       return ofs->numlayer - ofs->numdatalayer - 1;
> > +}
> > +
> >  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
> >  {
> >         return ofs->layers[0].mnt;
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 9b326b857ad6..988edb9e9d23 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1576,6 +1576,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> >         return ofs->numfs++;
> >  }
> >
> > +/*
> > + * The fsid after the last lower fsid is used for the data layers.
> > + * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
> > + */
> > +static int ovl_get_data_fsid(struct ovl_fs *ofs)
> > +{
> > +       return ofs->numfs;
> > +}
> > +
> > +
> >  static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
> >                           struct path *stack, unsigned int numlower,
> >                           struct ovl_layer *layers)
> > @@ -1583,11 +1593,14 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
> >         int err;
> >         unsigned int i;
> >
> > -       ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
> > +       ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb), GFP_KERNEL);
> >         if (ofs->fs == NULL)
> >                 return -ENOMEM;
> >
> > -       /* idx/fsid 0 are reserved for upper fs even with lower only overlay */
> > +       /*
> > +        * idx/fsid 0 are reserved for upper fs even with lower only overlay
> > +        * and the last fsid is reserved for "null fs" of the data layers.
> > +        */
> >         ofs->numfs++;
> >
> >         /*
> > @@ -1612,7 +1625,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
> >                 struct inode *trap;
> >                 int fsid;
> >
> > -               fsid = ovl_get_fsid(ofs, &stack[i]);
> > +               if (i < numlower - ofs->numdatalayer)
> > +                       fsid = ovl_get_fsid(ofs, &stack[i]);
> > +               else
> > +                       fsid = ovl_get_data_fsid(ofs);
> >                 if (fsid < 0)
> >                         return fsid;
> >
> > @@ -1700,6 +1716,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
> >         int err;
> >         struct path *stack = NULL;
> >         struct ovl_path *lowerstack;
> > +       unsigned int numlowerdata = 0;
> >         unsigned int i;
> >         struct ovl_entry *oe;
> >
> > @@ -1712,13 +1729,27 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
> >         if (!stack)
> >                 return ERR_PTR(-ENOMEM);
> >
> > -       err = -EINVAL;
> > -       for (i = 0; i < numlower; i++) {
> > +       for (i = 0; i < numlower;) {
> >                 err = ovl_lower_dir(lower, &stack[i], ofs, &sb->s_stack_depth);
> >                 if (err)
> >                         goto out_err;
> >
> >                 lower = strchr(lower, '\0') + 1;
> > +
> > +               i++;
> > +               err = -EINVAL;
> > +               /* :: separator indicates the start of lower data layers */
> > +               if (!*lower && i < numlower && !numlowerdata) {
>
> FYI, kernel test bot reported a KASAN out-of-bounds access on !*lower
> because it is tested also when i == numlower and in any case, the test
> should be i < numlower - 1, so :: cannot be at the end of the list.
>
> Pushed a fix to branch ovl-lazy-lowerdata with an improved comment:
>
>                 /*
>                  * Empty lower layer path could mean :: separator that indicates
>                  * the start of lower data layers.
>                  * Only one :: separator is allowed and it has to have at least
>                  * one lowerdir to the left and one lowerdir to the right.
>                  */
>                 if (!numlowerdata && i < numlower - 1 && !*lower) {
>
>

FYI, rebased branch ovl-lazy-lowerdata onto v6.4-rc2.

Miklos, please let me know if you want me to re-post v3
with the KASAN warning fix above.

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Thu, Apr 27, 2023 at 4:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> This v2 combines the prep patch set [1] and lazy lookup patch set [2].
>
> This work is motivated by Alexander's composefs use case.
> Alexander has been developing and testing his fsverity patches over
> my lazy-lowerdata-lookup branch [3].

FYI, I rebased this branch on top of v6.4-rc2 and on top
of branch ovl-fixes with the NULL pointer defer fix patches
from Zhihao Cheng:

https://lore.kernel.org/linux-unionfs/20230516141619.2160800-1-chengzhihao1@huawei.com/

Thanks,
Amir.

>
> Alexander has also written tests for lazy lowerdata lookup [4].
>
> Note that patch #1 is a Fixes patch for stable.
> Gao commented that the fix may not be complete, but I think it is better
> than no fix at all.
>
> Regarding lazy lookup in d_real(), I am not sure if the best effort
> lookup is the best solution, but in any case, none of this code kicks in
> without explicit opt-in to data-only layers, so the risk of breaking
> existing setups is quite low.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Include the prep patch set
> - Split remove lowerdata from add lowerdata_redirect patch
> - Remove embedded ovl_entry stack optimization
> - Add lazy lookup and comment in d_real_inode()
> - Improve documentation of :: data-only layers syntax
> - Added RVBs
>
> [1] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-unionfs/20230412135412.1684197-1-amir73il@gmail.com/
> [3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
> [4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
>
> Amir Goldstein (13):
>   ovl: update of dentry revalidate flags after copy up
>   ovl: use OVL_E() and OVL_E_FLAGS() accessors
>   ovl: use ovl_numlower() and ovl_lowerstack() accessors
>   ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
>   ovl: move ovl_entry into ovl_inode
>   ovl: deduplicate lowerpath and lowerstack[]
>   ovl: deduplicate lowerdata and lowerstack[]
>   ovl: remove unneeded goto instructions
>   ovl: introduce data-only lower layers
>   ovl: implement lookup in data-only layers
>   ovl: prepare to store lowerdata redirect for lazy lowerdata lookup
>   ovl: prepare for lazy lookup of lowerdata inode
>   ovl: implement lazy lookup of lowerdata in data-only layers
>
>  Documentation/filesystems/overlayfs.rst |  36 +++++
>  fs/overlayfs/copy_up.c                  |  11 ++
>  fs/overlayfs/dir.c                      |   3 +-
>  fs/overlayfs/export.c                   |  41 +++---
>  fs/overlayfs/file.c                     |  21 ++-
>  fs/overlayfs/inode.c                    |  38 +++--
>  fs/overlayfs/namei.c                    | 185 +++++++++++++++++++-----
>  fs/overlayfs/overlayfs.h                |  20 ++-
>  fs/overlayfs/ovl_entry.h                |  73 ++++++++--
>  fs/overlayfs/super.c                    | 132 ++++++++++-------
>  fs/overlayfs/util.c                     | 165 ++++++++++++++++-----
>  11 files changed, 534 insertions(+), 191 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Alexander Larsson]

Something that came up about this in a discussion recently was
multi-layer composefs style images. For example, this may be a useful
approach for multi-layer container images.

In such a setup you would have one lowerdata layer, but two real
lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
accidentally have the same name as a file on C, causing a redirect
from A to end up in B instead of C.

Would it be possible to have a syntax for redirects that mean "only
lookup in lowerdata layers. For example a double-slash path
//some/file.

On Thu, Apr 27, 2023 at 3:06 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> This v2 combines the prep patch set [1] and lazy lookup patch set [2].
>
> This work is motivated by Alexander's composefs use case.
> Alexander has been developing and testing his fsverity patches over
> my lazy-lowerdata-lookup branch [3].
>
> Alexander has also written tests for lazy lowerdata lookup [4].
>
> Note that patch #1 is a Fixes patch for stable.
> Gao commented that the fix may not be complete, but I think it is better
> than no fix at all.
>
> Regarding lazy lookup in d_real(), I am not sure if the best effort
> lookup is the best solution, but in any case, none of this code kicks in
> without explicit opt-in to data-only layers, so the risk of breaking
> existing setups is quite low.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Include the prep patch set
> - Split remove lowerdata from add lowerdata_redirect patch
> - Remove embedded ovl_entry stack optimization
> - Add lazy lookup and comment in d_real_inode()
> - Improve documentation of :: data-only layers syntax
> - Added RVBs
>
> [1] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-unionfs/20230412135412.1684197-1-amir73il@gmail.com/
> [3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
> [4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
>
> Amir Goldstein (13):
>   ovl: update of dentry revalidate flags after copy up
>   ovl: use OVL_E() and OVL_E_FLAGS() accessors
>   ovl: use ovl_numlower() and ovl_lowerstack() accessors
>   ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
>   ovl: move ovl_entry into ovl_inode
>   ovl: deduplicate lowerpath and lowerstack[]
>   ovl: deduplicate lowerdata and lowerstack[]
>   ovl: remove unneeded goto instructions
>   ovl: introduce data-only lower layers
>   ovl: implement lookup in data-only layers
>   ovl: prepare to store lowerdata redirect for lazy lowerdata lookup
>   ovl: prepare for lazy lookup of lowerdata inode
>   ovl: implement lazy lookup of lowerdata in data-only layers
>
>  Documentation/filesystems/overlayfs.rst |  36 +++++
>  fs/overlayfs/copy_up.c                  |  11 ++
>  fs/overlayfs/dir.c                      |   3 +-
>  fs/overlayfs/export.c                   |  41 +++---
>  fs/overlayfs/file.c                     |  21 ++-
>  fs/overlayfs/inode.c                    |  38 +++--
>  fs/overlayfs/namei.c                    | 185 +++++++++++++++++++-----
>  fs/overlayfs/overlayfs.h                |  20 ++-
>  fs/overlayfs/ovl_entry.h                |  73 ++++++++--
>  fs/overlayfs/super.c                    | 132 ++++++++++-------
>  fs/overlayfs/util.c                     | 165 ++++++++++++++++-----
>  11 files changed, 534 insertions(+), 191 deletions(-)
>
> --
> 2.34.1
>


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> Something that came up about this in a discussion recently was
> multi-layer composefs style images. For example, this may be a useful
> approach for multi-layer container images.
>
> In such a setup you would have one lowerdata layer, but two real
> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
> accidentally have the same name as a file on C, causing a redirect
> from A to end up in B instead of C.
>

I was under the impression that the names of the data blobs in C
are supposed to be content derived names (hash).
Is this not the case or is the concern about hash conflicts?

> Would it be possible to have a syntax for redirects that mean "only
> lookup in lowerdata layers. For example a double-slash path
> //some/file.
>

Anything is possible if we can define the problem that needs to be solved.
In this case, I did not understand why the problem is limited to finding a file
by mistake in layer B.

If there are several data layers A:B::C:D why wouldn't we have the same
problem with a file name collision between C and D?

So if there was a need to be able to redirect to a specific layer,
I would imagine that we would need to be able to address any layer
and not just "the start of data layers".

If we were looking for a syntax that is not a current valid redirect,
anything with // would work as well as anything with / that is not
an absolute path, e.g. 3:/path/to/file, so both NFS and SMB ;-)

Please describe the problem with more details and examples.

Thanks,
Amir.

> On Thu, Apr 27, 2023 at 3:06 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Miklos,
> >
> > This v2 combines the prep patch set [1] and lazy lookup patch set [2].
> >
> > This work is motivated by Alexander's composefs use case.
> > Alexander has been developing and testing his fsverity patches over
> > my lazy-lowerdata-lookup branch [3].
> >
> > Alexander has also written tests for lazy lowerdata lookup [4].
> >
> > Note that patch #1 is a Fixes patch for stable.
> > Gao commented that the fix may not be complete, but I think it is better
> > than no fix at all.
> >
> > Regarding lazy lookup in d_real(), I am not sure if the best effort
> > lookup is the best solution, but in any case, none of this code kicks in
> > without explicit opt-in to data-only layers, so the risk of breaking
> > existing setups is quite low.
> >
> > Thanks,
> > Amir.
> >
> > Changes since v1:
> > - Include the prep patch set
> > - Split remove lowerdata from add lowerdata_redirect patch
> > - Remove embedded ovl_entry stack optimization
> > - Add lazy lookup and comment in d_real_inode()
> > - Improve documentation of :: data-only layers syntax
> > - Added RVBs
> >
> > [1] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
> > [2] https://lore.kernel.org/linux-unionfs/20230412135412.1684197-1-amir73il@gmail.com/
> > [3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
> > [4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
> >
> > Amir Goldstein (13):
> >   ovl: update of dentry revalidate flags after copy up
> >   ovl: use OVL_E() and OVL_E_FLAGS() accessors
> >   ovl: use ovl_numlower() and ovl_lowerstack() accessors
> >   ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
> >   ovl: move ovl_entry into ovl_inode
> >   ovl: deduplicate lowerpath and lowerstack[]
> >   ovl: deduplicate lowerdata and lowerstack[]
> >   ovl: remove unneeded goto instructions
> >   ovl: introduce data-only lower layers
> >   ovl: implement lookup in data-only layers
> >   ovl: prepare to store lowerdata redirect for lazy lowerdata lookup
> >   ovl: prepare for lazy lookup of lowerdata inode
> >   ovl: implement lazy lookup of lowerdata in data-only layers
> >
> >  Documentation/filesystems/overlayfs.rst |  36 +++++
> >  fs/overlayfs/copy_up.c                  |  11 ++
> >  fs/overlayfs/dir.c                      |   3 +-
> >  fs/overlayfs/export.c                   |  41 +++---
> >  fs/overlayfs/file.c                     |  21 ++-
> >  fs/overlayfs/inode.c                    |  38 +++--
> >  fs/overlayfs/namei.c                    | 185 +++++++++++++++++++-----
> >  fs/overlayfs/overlayfs.h                |  20 ++-
> >  fs/overlayfs/ovl_entry.h                |  73 ++++++++--
> >  fs/overlayfs/super.c                    | 132 ++++++++++-------
> >  fs/overlayfs/util.c                     | 165 ++++++++++++++++-----
> >  11 files changed, 534 insertions(+), 191 deletions(-)
> >
> > --
> > 2.34.1
> >
>
>
> --
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>  Alexander Larsson                                Red Hat, Inc
>        alexl@redhat.com         alexander.larsson@gmail.com
>

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Giuseppe Scrivano]

Hi Amir,

Amir Goldstein <amir73il@gmail.com> writes:

> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>>
>> Something that came up about this in a discussion recently was
>> multi-layer composefs style images. For example, this may be a useful
>> approach for multi-layer container images.
>>
>> In such a setup you would have one lowerdata layer, but two real
>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
>> accidentally have the same name as a file on C, causing a redirect
>> from A to end up in B instead of C.
>>
>
> I was under the impression that the names of the data blobs in C
> are supposed to be content derived names (hash).
> Is this not the case or is the concern about hash conflicts?
>
>> Would it be possible to have a syntax for redirects that mean "only
>> lookup in lowerdata layers. For example a double-slash path
>> //some/file.
>>
>
> Anything is possible if we can define the problem that needs to be solved.
> In this case, I did not understand why the problem is limited to finding a file
> by mistake in layer B.
>
> If there are several data layers A:B::C:D why wouldn't we have the same
> problem with a file name collision between C and D?

the data layer is constructed in a way that files are stored by their
hash and there is control from the container runtime on how this is
built and maintained.  So a file name collision would happen only when
on a hash collision.

Differently for the other layers we've no control on what files are in
the image, unless we limit to mount only one EROFS as the first lower
layer and then all the other lower layers are data layers.

Given your example above A:B::C:D, if both A and B are EROFS we are
limited in the files/directories that can be in B.

e.g. we have A/foo with the following xattrs:

trusted.overlay.metacopy=""
trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"

Now what would happen if /1e is present as a file in layer B?  It will
just cause the lookup for `foo` to fail with EIO since the redirect
didn't find any file in the layers below.


> So if there was a need to be able to redirect to a specific layer,
> I would imagine that we would need to be able to address any layer
> and not just "the start of data layers".
>
> If we were looking for a syntax that is not a current valid redirect,
> anything with // would work as well as anything with / that is not
> an absolute path, e.g. 3:/path/to/file, so both NFS and SMB ;-)
>
> Please describe the problem with more details and examples.
>
> Thanks,
> Amir.
>
>> On Thu, Apr 27, 2023 at 3:06 PM Amir Goldstein <amir73il@gmail.com> wrote:
>> >
>> > Miklos,
>> >
>> > This v2 combines the prep patch set [1] and lazy lookup patch set [2].
>> >
>> > This work is motivated by Alexander's composefs use case.
>> > Alexander has been developing and testing his fsverity patches over
>> > my lazy-lowerdata-lookup branch [3].
>> >
>> > Alexander has also written tests for lazy lowerdata lookup [4].
>> >
>> > Note that patch #1 is a Fixes patch for stable.
>> > Gao commented that the fix may not be complete, but I think it is better
>> > than no fix at all.
>> >
>> > Regarding lazy lookup in d_real(), I am not sure if the best effort
>> > lookup is the best solution, but in any case, none of this code kicks in
>> > without explicit opt-in to data-only layers, so the risk of breaking
>> > existing setups is quite low.
>> >
>> > Thanks,
>> > Amir.
>> >
>> > Changes since v1:
>> > - Include the prep patch set
>> > - Split remove lowerdata from add lowerdata_redirect patch
>> > - Remove embedded ovl_entry stack optimization
>> > - Add lazy lookup and comment in d_real_inode()
>> > - Improve documentation of :: data-only layers syntax
>> > - Added RVBs
>> >
>> > [1] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
>> > [2] https://lore.kernel.org/linux-unionfs/20230412135412.1684197-1-amir73il@gmail.com/
>> > [3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
>> > [4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
>> >
>> > Amir Goldstein (13):
>> >   ovl: update of dentry revalidate flags after copy up
>> >   ovl: use OVL_E() and OVL_E_FLAGS() accessors
>> >   ovl: use ovl_numlower() and ovl_lowerstack() accessors
>> >   ovl: factor out ovl_free_entry() and ovl_stack_*() helpers
>> >   ovl: move ovl_entry into ovl_inode
>> >   ovl: deduplicate lowerpath and lowerstack[]
>> >   ovl: deduplicate lowerdata and lowerstack[]
>> >   ovl: remove unneeded goto instructions
>> >   ovl: introduce data-only lower layers
>> >   ovl: implement lookup in data-only layers
>> >   ovl: prepare to store lowerdata redirect for lazy lowerdata lookup
>> >   ovl: prepare for lazy lookup of lowerdata inode
>> >   ovl: implement lazy lookup of lowerdata in data-only layers
>> >
>> >  Documentation/filesystems/overlayfs.rst |  36 +++++
>> >  fs/overlayfs/copy_up.c                  |  11 ++
>> >  fs/overlayfs/dir.c                      |   3 +-
>> >  fs/overlayfs/export.c                   |  41 +++---
>> >  fs/overlayfs/file.c                     |  21 ++-
>> >  fs/overlayfs/inode.c                    |  38 +++--
>> >  fs/overlayfs/namei.c                    | 185 +++++++++++++++++++-----
>> >  fs/overlayfs/overlayfs.h                |  20 ++-
>> >  fs/overlayfs/ovl_entry.h                |  73 ++++++++--
>> >  fs/overlayfs/super.c                    | 132 ++++++++++-------
>> >  fs/overlayfs/util.c                     | 165 ++++++++++++++++-----
>> >  11 files changed, 534 insertions(+), 191 deletions(-)
>> >
>> > --
>> > 2.34.1
>> >
>>
>>
>> --
>> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>>  Alexander Larsson                                Red Hat, Inc
>>        alexl@redhat.com         alexander.larsson@gmail.com
>>

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Gao Xiang]

Hi Giuseppe,

On 2023/5/26 09:59, Giuseppe Scrivano wrote:
> Hi Amir,
> 
> Amir Goldstein <amir73il@gmail.com> writes:
> 
>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>>>
>>> Something that came up about this in a discussion recently was
>>> multi-layer composefs style images. For example, this may be a useful
>>> approach for multi-layer container images.
>>>
>>> In such a setup you would have one lowerdata layer, but two real
>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
>>> accidentally have the same name as a file on C, causing a redirect
>>> from A to end up in B instead of C.
>>>
>>
>> I was under the impression that the names of the data blobs in C
>> are supposed to be content derived names (hash).
>> Is this not the case or is the concern about hash conflicts?
>>
>>> Would it be possible to have a syntax for redirects that mean "only
>>> lookup in lowerdata layers. For example a double-slash path
>>> //some/file.
>>>
>>
>> Anything is possible if we can define the problem that needs to be solved.
>> In this case, I did not understand why the problem is limited to finding a file
>> by mistake in layer B.
>>
>> If there are several data layers A:B::C:D why wouldn't we have the same
>> problem with a file name collision between C and D?
> 
> the data layer is constructed in a way that files are stored by their
> hash and there is control from the container runtime on how this is
> built and maintained.  So a file name collision would happen only when
> on a hash collision.
> 
> Differently for the other layers we've no control on what files are in
> the image, unless we limit to mount only one EROFS as the first lower
> layer and then all the other lower layers are data layers.
> 
> Given your example above A:B::C:D, if both A and B are EROFS we are
> limited in the files/directories that can be in B.

If my understanding is correct (hopefully), I might ask if it's the
proposal to pass in multiple composefs manifests (rather than one)
all together to overlayfs in one shot?

> 
> e.g. we have A/foo with the following xattrs:
> 
> trusted.overlay.metacopy=""
> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
> 
> Now what would happen if /1e is present as a file in layer B?  It will
> just cause the lookup for `foo` to fail with EIO since the redirect
> didn't find any file in the layers below.

If my understanding is correct, alternative one way might one do
a merged manifest before mounting? (of course a new manifest is
generated, but I think if it could be acceptable since the whole
merging process is under control?)

My overall thought is that it seems another seperate new
enhancement (if it needs more discussion) and have we might need
to land this lazy lookup first upstream as the first step?
(since our internal overlayfs users benefit this optimization
  as well... I'd like to see it upstream)

If it's a pure use case discussion, ignore me. ;)

Many thanks,
Gao Xiang

> 
> 

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Gao Xiang]

On 2023/5/26 10:27, Gao Xiang wrote:
> Hi Giuseppe,
> 
> On 2023/5/26 09:59, Giuseppe Scrivano wrote:
>> Hi Amir,
>>
>> Amir Goldstein <amir73il@gmail.com> writes:
>>
>>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>>>>
>>>> Something that came up about this in a discussion recently was
>>>> multi-layer composefs style images. For example, this may be a useful
>>>> approach for multi-layer container images.
>>>>
>>>> In such a setup you would have one lowerdata layer, but two real
>>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
>>>> accidentally have the same name as a file on C, causing a redirect
>>>> from A to end up in B instead of C.
>>>>
>>>
>>> I was under the impression that the names of the data blobs in C
>>> are supposed to be content derived names (hash).
>>> Is this not the case or is the concern about hash conflicts?
>>>
>>>> Would it be possible to have a syntax for redirects that mean "only
>>>> lookup in lowerdata layers. For example a double-slash path
>>>> //some/file.
>>>>
>>>
>>> Anything is possible if we can define the problem that needs to be solved.
>>> In this case, I did not understand why the problem is limited to finding a file
>>> by mistake in layer B.
>>>
>>> If there are several data layers A:B::C:D why wouldn't we have the same
>>> problem with a file name collision between C and D?
>>
>> the data layer is constructed in a way that files are stored by their
>> hash and there is control from the container runtime on how this is
>> built and maintained.  So a file name collision would happen only when
>> on a hash collision.
>>
>> Differently for the other layers we've no control on what files are in
>> the image, unless we limit to mount only one EROFS as the first lower
>> layer and then all the other lower layers are data layers.
>>
>> Given your example above A:B::C:D, if both A and B are EROFS we are
>> limited in the files/directories that can be in B.
> 
> If my understanding is correct (hopefully), I might ask if it's the
> proposal to pass in multiple composefs manifests (rather than one)
> all together to overlayfs in one shot?

Oh I could see the issue if a composefs manifest works with other
regular underlay layers, such redirect might need a way directly
to data layers instead of any potential underlay layer.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Hi Amir,
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
> >>
> >> Something that came up about this in a discussion recently was
> >> multi-layer composefs style images. For example, this may be a useful
> >> approach for multi-layer container images.
> >>
> >> In such a setup you would have one lowerdata layer, but two real
> >> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
> >> accidentally have the same name as a file on C, causing a redirect
> >> from A to end up in B instead of C.
> >>
> >
> > I was under the impression that the names of the data blobs in C
> > are supposed to be content derived names (hash).
> > Is this not the case or is the concern about hash conflicts?
> >
> >> Would it be possible to have a syntax for redirects that mean "only
> >> lookup in lowerdata layers. For example a double-slash path
> >> //some/file.
> >>
> >
> > Anything is possible if we can define the problem that needs to be solved.
> > In this case, I did not understand why the problem is limited to finding a file
> > by mistake in layer B.
> >
> > If there are several data layers A:B::C:D why wouldn't we have the same
> > problem with a file name collision between C and D?
>
> the data layer is constructed in a way that files are stored by their
> hash and there is control from the container runtime on how this is
> built and maintained.  So a file name collision would happen only when
> on a hash collision.
>
> Differently for the other layers we've no control on what files are in
> the image, unless we limit to mount only one EROFS as the first lower
> layer and then all the other lower layers are data layers.
>
> Given your example above A:B::C:D, if both A and B are EROFS we are
> limited in the files/directories that can be in B.
>
> e.g. we have A/foo with the following xattrs:
>
> trusted.overlay.metacopy=""
> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
>
> Now what would happen if /1e is present as a file in layer B?  It will
> just cause the lookup for `foo` to fail with EIO since the redirect
> didn't find any file in the layers below.
>
>

I understand the problem and I understand why a // redirect to data-only layers
would be a simple and workable solution for composefs.

Unlike the rest of the changes to overlayfs that we worked on to support
composefs, this would really be a composefs only on-disk format because it
could not be generated by overlayfs itself, so we need Miklos to chime in to
say if this is acceptable.

I have one question though:

If you place all the blobs under /.cfs/1e/... is that really going to
be an issue?
I mean the middle layers are not random stuff and I don't think we need to worry
about malicious images blocking the data layers, because malicious images have
much easier ways of making damage.

It doesn't seem so likely for images to overload /.cfs by mistake with
anything other
than a proper composefs blobs repository (which should have no hash conflicts)
and in the unlikely case that the image does happen to have a rogue /.cfs file
the container runtime can declare that image invalid for composefs mount.

Another observation: this problem reminds me of the "follow origin" [1] feature
I was working on a long time ago.

[1] https://github.com/amir73il/linux/commits/ovl-follow-origin

This work had a different use case and the patches (that follow directories)
are not relevant for this use case, but the same principle could be applied
to following metacopy to lowerdata by origin when the lowerdata cannot be
found by redirect (or redirected to "" to signify disconnected path).

Overlayfs copied up files and metacopy in particular have an "origin" xattr.
The "origin" xattr holds a file handle of the origin inode and uuid of
the origin layer.
This xattr has some uses, but I would like to point out one particular use -
in ovl_lookup() we use ovl_check_origin() to find a lower non-dir, so
that we can
use its i_ino for the overlay inode.

We do that also for non-metacopy and non-redirect. The special thing about
ovl_check_origin() is that it is blind to the problem that you described.
Unless the origin inode was deleted from the filesystem, overlayfs will find it.
The path of the found "origin" may not be known to overlayfs (i.e. disconnected
dentry), but it also does not matter to overlayfs.

The reason I am telling you about this is because in the case that composefs
is used to create composefs layers on a specific machine or in a specific
local network, where the blobs are going to be stored on a specific
shared backing
filesystem, using "origin" instead of "redirect" as the way to refer
to the lower-data
might not be such a bad idea.

For example, in LSFMM, Lennart presented the idea of a system service that would
be able to provide "signed composefs image creation" services for unprivileged
containers from standard OCI images. In that case, creating images
optimized for a
specific local blobs repository could be an option.

Apart from enabling composefs mount for unprivileged containers, the centric
system service scheme could also be used to "optimize" distributed composefs
images to the local storage (i.e. convert //data redirect to origin reference).

Combined with Alex's verity feature, I think we could allow following lowerdata
by "origin" without any further opt-in from user, meaning:
If lowerdata cannot be found by path (or has intentional "" redirect)
AND if metacopy has verity xattr, defer to lazy lowerdata lookup,
where lowerdata would be looked up by origin.

Does this sound like something that would be useful for composefs ecosystem?
If it is, I could send a patch for testing.

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Alexander Larsson]

On Fri, May 26, 2023 at 7:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >
> > Hi Amir,
> >
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> > > On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >>
> > >> Something that came up about this in a discussion recently was
> > >> multi-layer composefs style images. For example, this may be a useful
> > >> approach for multi-layer container images.
> > >>
> > >> In such a setup you would have one lowerdata layer, but two real
> > >> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
> > >> accidentally have the same name as a file on C, causing a redirect
> > >> from A to end up in B instead of C.
> > >>
> > >
> > > I was under the impression that the names of the data blobs in C
> > > are supposed to be content derived names (hash).
> > > Is this not the case or is the concern about hash conflicts?
> > >
> > >> Would it be possible to have a syntax for redirects that mean "only
> > >> lookup in lowerdata layers. For example a double-slash path
> > >> //some/file.
> > >>
> > >
> > > Anything is possible if we can define the problem that needs to be solved.
> > > In this case, I did not understand why the problem is limited to finding a file
> > > by mistake in layer B.
> > >
> > > If there are several data layers A:B::C:D why wouldn't we have the same
> > > problem with a file name collision between C and D?
> >
> > the data layer is constructed in a way that files are stored by their
> > hash and there is control from the container runtime on how this is
> > built and maintained.  So a file name collision would happen only when
> > on a hash collision.
> >
> > Differently for the other layers we've no control on what files are in
> > the image, unless we limit to mount only one EROFS as the first lower
> > layer and then all the other lower layers are data layers.
> >
> > Given your example above A:B::C:D, if both A and B are EROFS we are
> > limited in the files/directories that can be in B.
> >
> > e.g. we have A/foo with the following xattrs:
> >
> > trusted.overlay.metacopy=""
> > trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
> >
> > Now what would happen if /1e is present as a file in layer B?  It will
> > just cause the lookup for `foo` to fail with EIO since the redirect
> > didn't find any file in the layers below.
> >
> >
>
> I understand the problem and I understand why a // redirect to data-only layers
> would be a simple and workable solution for composefs.
>
> Unlike the rest of the changes to overlayfs that we worked on to support
> composefs, this would really be a composefs only on-disk format because it
> could not be generated by overlayfs itself, so we need Miklos to chime in to
> say if this is acceptable.
>
> I have one question though:
>
> If you place all the blobs under /.cfs/1e/... is that really going to
> be an issue?
> I mean the middle layers are not random stuff and I don't think we need to worry
> about malicious images blocking the data layers, because malicious images have
> much easier ways of making damage.

I think if you make the prefix (the /.cfs part) "weird" enough it will
only become an issue for the malicious layer case. For example, a
malicious layer could inject a file that was typically unused, but if
an upper layer happened to use a particular redirect path it would get
unexpected content for its redirect.

However, you wouldn't be able to inject such a base layer after the
fact, it would have to have been there already in the base layer used
when building the upper layer. So in this case I agree, such a
malicious layer used already at image build time can do malicious
things in much easier ways anyway.

> It doesn't seem so likely for images to overload /.cfs by mistake with
> anything other
> than a proper composefs blobs repository (which should have no hash conflicts)
> and in the unlikely case that the image does happen to have a rogue /.cfs file
> the container runtime can declare that image invalid for composefs mount.
>
> Another observation: this problem reminds me of the "follow origin" [1] feature
> I was working on a long time ago.
>
> [1] https://github.com/amir73il/linux/commits/ovl-follow-origin
>
> This work had a different use case and the patches (that follow directories)
> are not relevant for this use case, but the same principle could be applied
> to following metacopy to lowerdata by origin when the lowerdata cannot be
> found by redirect (or redirected to "" to signify disconnected path).
>
> Overlayfs copied up files and metacopy in particular have an "origin" xattr.
> The "origin" xattr holds a file handle of the origin inode and uuid of
> the origin layer.
> This xattr has some uses, but I would like to point out one particular use -
> in ovl_lookup() we use ovl_check_origin() to find a lower non-dir, so
> that we can
> use its i_ino for the overlay inode.
>
> We do that also for non-metacopy and non-redirect. The special thing about
> ovl_check_origin() is that it is blind to the problem that you described.
> Unless the origin inode was deleted from the filesystem, overlayfs will find it.
> The path of the found "origin" may not be known to overlayfs (i.e. disconnected
> dentry), but it also does not matter to overlayfs.
>
> The reason I am telling you about this is because in the case that composefs
> is used to create composefs layers on a specific machine or in a specific
> local network, where the blobs are going to be stored on a specific
> shared backing
> filesystem, using "origin" instead of "redirect" as the way to refer
> to the lower-data
> might not be such a bad idea.
>
> For example, in LSFMM, Lennart presented the idea of a system service that would
> be able to provide "signed composefs image creation" services for unprivileged
> containers from standard OCI images. In that case, creating images
> optimized for a
> specific local blobs repository could be an option.
>
> Apart from enabling composefs mount for unprivileged containers, the centric
> system service scheme could also be used to "optimize" distributed composefs
> images to the local storage (i.e. convert //data redirect to origin reference).
>
> Combined with Alex's verity feature, I think we could allow following lowerdata
> by "origin" without any further opt-in from user, meaning:
> If lowerdata cannot be found by path (or has intentional "" redirect)
> AND if metacopy has verity xattr, defer to lazy lowerdata lookup,
> where lowerdata would be looked up by origin.
>
> Does this sound like something that would be useful for composefs ecosystem?
> If it is, I could send a patch for testing.

I can't think of any use of this currently. Generally we very rarely
work with images using a backing store tied to a particular machine
like this. Generally the goal is to have something you can distribute.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Gao Xiang]

Hi,

On 2023/5/26 04:36, Alexander Larsson wrote:
> On Fri, May 26, 2023 at 7:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>>
>>> Hi Amir,
>>>
>>> Amir Goldstein <amir73il@gmail.com> writes:
>>>
>>>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>>>>>
>>>>> Something that came up about this in a discussion recently was
>>>>> multi-layer composefs style images. For example, this may be a useful
>>>>> approach for multi-layer container images.
>>>>>
>>>>> In such a setup you would have one lowerdata layer, but two real
>>>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
>>>>> accidentally have the same name as a file on C, causing a redirect
>>>>> from A to end up in B instead of C.
>>>>>
>>>>
>>>> I was under the impression that the names of the data blobs in C
>>>> are supposed to be content derived names (hash).
>>>> Is this not the case or is the concern about hash conflicts?
>>>>
>>>>> Would it be possible to have a syntax for redirects that mean "only
>>>>> lookup in lowerdata layers. For example a double-slash path
>>>>> //some/file.
>>>>>
>>>>
>>>> Anything is possible if we can define the problem that needs to be solved.
>>>> In this case, I did not understand why the problem is limited to finding a file
>>>> by mistake in layer B.
>>>>
>>>> If there are several data layers A:B::C:D why wouldn't we have the same
>>>> problem with a file name collision between C and D?
>>>
>>> the data layer is constructed in a way that files are stored by their
>>> hash and there is control from the container runtime on how this is
>>> built and maintained.  So a file name collision would happen only when
>>> on a hash collision.
>>>
>>> Differently for the other layers we've no control on what files are in
>>> the image, unless we limit to mount only one EROFS as the first lower
>>> layer and then all the other lower layers are data layers.
>>>
>>> Given your example above A:B::C:D, if both A and B are EROFS we are
>>> limited in the files/directories that can be in B.
>>>
>>> e.g. we have A/foo with the following xattrs:
>>>
>>> trusted.overlay.metacopy=""
>>> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
>>>
>>> Now what would happen if /1e is present as a file in layer B?  It will
>>> just cause the lookup for `foo` to fail with EIO since the redirect
>>> didn't find any file in the layers below.
>>>
>>>
>>
>> I understand the problem and I understand why a // redirect to data-only layers
>> would be a simple and workable solution for composefs.
>>
>> Unlike the rest of the changes to overlayfs that we worked on to support
>> composefs, this would really be a composefs only on-disk format because it
>> could not be generated by overlayfs itself, so we need Miklos to chime in to
>> say if this is acceptable.

An alternative way might allow data-only layers (or invisible layers) in the
middle rather than as the tail?

I'm not sure in the long term if it's flexible to fix data-only layers as the
bottom-most layers for future potential use cases.

At a quick glance, I've seen the implementation of this patchset also
strictly code that.   I wonder if using non-fixed invisible layers increases
the complexity or am I still missing something?

Thanks,
Gao Xiang

> 

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Fri, May 26, 2023 at 9:27 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
> Hi,
>
> On 2023/5/26 04:36, Alexander Larsson wrote:
> > On Fri, May 26, 2023 at 7:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >>
> >> On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >>>
> >>> Hi Amir,
> >>>
> >>> Amir Goldstein <amir73il@gmail.com> writes:
> >>>
> >>>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
> >>>>>
> >>>>> Something that came up about this in a discussion recently was
> >>>>> multi-layer composefs style images. For example, this may be a useful
> >>>>> approach for multi-layer container images.
> >>>>>
> >>>>> In such a setup you would have one lowerdata layer, but two real
> >>>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
> >>>>> accidentally have the same name as a file on C, causing a redirect
> >>>>> from A to end up in B instead of C.
> >>>>>
> >>>>
> >>>> I was under the impression that the names of the data blobs in C
> >>>> are supposed to be content derived names (hash).
> >>>> Is this not the case or is the concern about hash conflicts?
> >>>>
> >>>>> Would it be possible to have a syntax for redirects that mean "only
> >>>>> lookup in lowerdata layers. For example a double-slash path
> >>>>> //some/file.
> >>>>>
> >>>>
> >>>> Anything is possible if we can define the problem that needs to be solved.
> >>>> In this case, I did not understand why the problem is limited to finding a file
> >>>> by mistake in layer B.
> >>>>
> >>>> If there are several data layers A:B::C:D why wouldn't we have the same
> >>>> problem with a file name collision between C and D?
> >>>
> >>> the data layer is constructed in a way that files are stored by their
> >>> hash and there is control from the container runtime on how this is
> >>> built and maintained.  So a file name collision would happen only when
> >>> on a hash collision.
> >>>
> >>> Differently for the other layers we've no control on what files are in
> >>> the image, unless we limit to mount only one EROFS as the first lower
> >>> layer and then all the other lower layers are data layers.
> >>>
> >>> Given your example above A:B::C:D, if both A and B are EROFS we are
> >>> limited in the files/directories that can be in B.
> >>>
> >>> e.g. we have A/foo with the following xattrs:
> >>>
> >>> trusted.overlay.metacopy=""
> >>> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
> >>>
> >>> Now what would happen if /1e is present as a file in layer B?  It will
> >>> just cause the lookup for `foo` to fail with EIO since the redirect
> >>> didn't find any file in the layers below.
> >>>
> >>>
> >>
> >> I understand the problem and I understand why a // redirect to data-only layers
> >> would be a simple and workable solution for composefs.
> >>
> >> Unlike the rest of the changes to overlayfs that we worked on to support
> >> composefs, this would really be a composefs only on-disk format because it
> >> could not be generated by overlayfs itself, so we need Miklos to chime in to
> >> say if this is acceptable.
>
> An alternative way might allow data-only layers (or invisible layers) in the
> middle rather than as the tail?
>

Anything is possible if you can justify its worth.

> I'm not sure in the long term if it's flexible to fix data-only layers as the
> bottom-most layers for future potential use cases.
>
> At a quick glance, I've seen the implementation of this patchset also
> strictly code that.   I wonder if using non-fixed invisible layers increases
> the complexity or am I still missing something?
>

The current implementation is quite simplified due to keeping data-only
layers in the tail, and even more simplified that lazy lowerdata lookup
is only in the data-only layers at the tail of the stack.
The documentation is also simpler as do the tests.

Making all the the above more complex needs justification and so far
I did not see any use case that would justify it, because the /.cfs
workaround is good enough IMO.

That leaves the question - is the design/API flexible enough to be
extended in the future if we needed to?

If we would want to support data-only layers in the middle on the
stack, which would this syntax make sense?
lowerdir=lower1::data1:lower2::data2

If this syntax makes sense to everyone, then we can change the syntax
of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
and enforce that after the first ::, only :: are allowed.

Miklos, any thoughts?
I have a feeling that this was your natural interpretation when you first
saw the :: syntax.

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Gao Xiang]

Hi Amir,

On 2023/5/27 22:04, Amir Goldstein wrote:
> On Fri, May 26, 2023 at 9:27 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>
>> Hi,
>>
>> On 2023/5/26 04:36, Alexander Larsson wrote:
>>> On Fri, May 26, 2023 at 7:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>> On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>>>>
>>>>> Hi Amir,
>>>>>
>>>>> Amir Goldstein <amir73il@gmail.com> writes:
>>>>>
>>>>>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
>>>>>>>
>>>>>>> Something that came up about this in a discussion recently was
>>>>>>> multi-layer composefs style images. For example, this may be a useful
>>>>>>> approach for multi-layer container images.
>>>>>>>
>>>>>>> In such a setup you would have one lowerdata layer, but two real
>>>>>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
>>>>>>> accidentally have the same name as a file on C, causing a redirect
>>>>>>> from A to end up in B instead of C.
>>>>>>>
>>>>>>
>>>>>> I was under the impression that the names of the data blobs in C
>>>>>> are supposed to be content derived names (hash).
>>>>>> Is this not the case or is the concern about hash conflicts?
>>>>>>
>>>>>>> Would it be possible to have a syntax for redirects that mean "only
>>>>>>> lookup in lowerdata layers. For example a double-slash path
>>>>>>> //some/file.
>>>>>>>
>>>>>>
>>>>>> Anything is possible if we can define the problem that needs to be solved.
>>>>>> In this case, I did not understand why the problem is limited to finding a file
>>>>>> by mistake in layer B.
>>>>>>
>>>>>> If there are several data layers A:B::C:D why wouldn't we have the same
>>>>>> problem with a file name collision between C and D?
>>>>>
>>>>> the data layer is constructed in a way that files are stored by their
>>>>> hash and there is control from the container runtime on how this is
>>>>> built and maintained.  So a file name collision would happen only when
>>>>> on a hash collision.
>>>>>
>>>>> Differently for the other layers we've no control on what files are in
>>>>> the image, unless we limit to mount only one EROFS as the first lower
>>>>> layer and then all the other lower layers are data layers.
>>>>>
>>>>> Given your example above A:B::C:D, if both A and B are EROFS we are
>>>>> limited in the files/directories that can be in B.
>>>>>
>>>>> e.g. we have A/foo with the following xattrs:
>>>>>
>>>>> trusted.overlay.metacopy=""
>>>>> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
>>>>>
>>>>> Now what would happen if /1e is present as a file in layer B?  It will
>>>>> just cause the lookup for `foo` to fail with EIO since the redirect
>>>>> didn't find any file in the layers below.
>>>>>
>>>>>
>>>>
>>>> I understand the problem and I understand why a // redirect to data-only layers
>>>> would be a simple and workable solution for composefs.
>>>>
>>>> Unlike the rest of the changes to overlayfs that we worked on to support
>>>> composefs, this would really be a composefs only on-disk format because it
>>>> could not be generated by overlayfs itself, so we need Miklos to chime in to
>>>> say if this is acceptable.
>>
>> An alternative way might allow data-only layers (or invisible layers) in the
>> middle rather than as the tail?
>>
> 
> Anything is possible if you can justify its worth.

TBH, I have no strong tendency of this for now.  But the hidden layers
(just for redirection) might be useful if they're used for data provider
only and my potential concern might be too strict so that we lose the
later extendibility...

> 
>> I'm not sure in the long term if it's flexible to fix data-only layers as the
>> bottom-most layers for future potential use cases.
>>
>> At a quick glance, I've seen the implementation of this patchset also
>> strictly code that.   I wonder if using non-fixed invisible layers increases
>> the complexity or am I still missing something?
>>
> 
> The current implementation is quite simplified due to keeping data-only
> layers in the tail, and even more simplified that lazy lowerdata lookup
> is only in the data-only layers at the tail of the stack.
> The documentation is also simpler as do the tests.
> 
> Making all the the above more complex needs justification and so far
> I did not see any use case that would justify it, because the /.cfs
> workaround is good enough IMO.
> 
> That leaves the question - is the design/API flexible enough to be
> extended in the future if we needed to?

Yeah, that is what I'd like to mention for now, at least leave
some extendibility for option format...

Thanks,
Gao Xiang

> 
> If we would want to support data-only layers in the middle on the
> stack, which would this syntax make sense?
> lowerdir=lower1::data1:lower2::data2
> 
> If this syntax makes sense to everyone, then we can change the syntax
> of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> and enforce that after the first ::, only :: are allowed.
> 
> Miklos, any thoughts?
> I have a feeling that this was your natural interpretation when you first
> saw the :: syntax.
> 
> Thanks,
> Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Alexander Larsson]

On Sat, May 27, 2023 at 4:04 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, May 26, 2023 at 9:27 PM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >
> > Hi,
> >
> > On 2023/5/26 04:36, Alexander Larsson wrote:
> > > On Fri, May 26, 2023 at 7:12 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >>
> > >> On Thu, May 25, 2023 at 7:59 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >>>
> > >>> Hi Amir,
> > >>>
> > >>> Amir Goldstein <amir73il@gmail.com> writes:
> > >>>
> > >>>> On Thu, May 25, 2023 at 6:21 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >>>>>
> > >>>>> Something that came up about this in a discussion recently was
> > >>>>> multi-layer composefs style images. For example, this may be a useful
> > >>>>> approach for multi-layer container images.
> > >>>>>
> > >>>>> In such a setup you would have one lowerdata layer, but two real
> > >>>>> lowerdirs, like lowerdir=A:B::C. In this situation a file in B may
> > >>>>> accidentally have the same name as a file on C, causing a redirect
> > >>>>> from A to end up in B instead of C.
> > >>>>>
> > >>>>
> > >>>> I was under the impression that the names of the data blobs in C
> > >>>> are supposed to be content derived names (hash).
> > >>>> Is this not the case or is the concern about hash conflicts?
> > >>>>
> > >>>>> Would it be possible to have a syntax for redirects that mean "only
> > >>>>> lookup in lowerdata layers. For example a double-slash path
> > >>>>> //some/file.
> > >>>>>
> > >>>>
> > >>>> Anything is possible if we can define the problem that needs to be solved.
> > >>>> In this case, I did not understand why the problem is limited to finding a file
> > >>>> by mistake in layer B.
> > >>>>
> > >>>> If there are several data layers A:B::C:D why wouldn't we have the same
> > >>>> problem with a file name collision between C and D?
> > >>>
> > >>> the data layer is constructed in a way that files are stored by their
> > >>> hash and there is control from the container runtime on how this is
> > >>> built and maintained.  So a file name collision would happen only when
> > >>> on a hash collision.
> > >>>
> > >>> Differently for the other layers we've no control on what files are in
> > >>> the image, unless we limit to mount only one EROFS as the first lower
> > >>> layer and then all the other lower layers are data layers.
> > >>>
> > >>> Given your example above A:B::C:D, if both A and B are EROFS we are
> > >>> limited in the files/directories that can be in B.
> > >>>
> > >>> e.g. we have A/foo with the following xattrs:
> > >>>
> > >>> trusted.overlay.metacopy=""
> > >>> trusted.overlay.redirect="/1e/de1743e73b904f16924c04fbd0b7fbfb7e45b8640241e7a08779e8f38fc20d"
> > >>>
> > >>> Now what would happen if /1e is present as a file in layer B?  It will
> > >>> just cause the lookup for `foo` to fail with EIO since the redirect
> > >>> didn't find any file in the layers below.
> > >>>
> > >>>
> > >>
> > >> I understand the problem and I understand why a // redirect to data-only layers
> > >> would be a simple and workable solution for composefs.
> > >>
> > >> Unlike the rest of the changes to overlayfs that we worked on to support
> > >> composefs, this would really be a composefs only on-disk format because it
> > >> could not be generated by overlayfs itself, so we need Miklos to chime in to
> > >> say if this is acceptable.
> >
> > An alternative way might allow data-only layers (or invisible layers) in the
> > middle rather than as the tail?
> >
>
> Anything is possible if you can justify its worth.
>
> > I'm not sure in the long term if it's flexible to fix data-only layers as the
> > bottom-most layers for future potential use cases.
> >
> > At a quick glance, I've seen the implementation of this patchset also
> > strictly code that.   I wonder if using non-fixed invisible layers increases
> > the complexity or am I still missing something?
> >
>
> The current implementation is quite simplified due to keeping data-only
> layers in the tail, and even more simplified that lazy lowerdata lookup
> is only in the data-only layers at the tail of the stack.
> The documentation is also simpler as do the tests.
>
> Making all the the above more complex needs justification and so far
> I did not see any use case that would justify it, because the /.cfs
> workaround is good enough IMO.
>
> That leaves the question - is the design/API flexible enough to be
> extended in the future if we needed to?
>
> If we would want to support data-only layers in the middle on the
> stack, which would this syntax make sense?
> lowerdir=lower1::data1:lower2::data2
>
> If this syntax makes sense to everyone, then we can change the syntax
> of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> and enforce that after the first ::, only :: are allowed.
>
> Miklos, any thoughts?
> I have a feeling that this was your natural interpretation when you first
> saw the :: syntax.

I agree that the current limit of data only at the end is good enough
for now. But yeah, having this syntax seems to give more options long
terms, and no real disadvantages. At the very least it is not harder
to understand or worse, and as you say, probably even more natural.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Miklos Szeredi]

On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:

> If we would want to support data-only layers in the middle on the
> stack, which would this syntax make sense?
> lowerdir=lower1::data1:lower2::data2
>
> If this syntax makes sense to everyone, then we can change the syntax
> of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> and enforce that after the first ::, only :: are allowed.
>
> Miklos, any thoughts?
> I have a feeling that this was your natural interpretation when you first
> saw the :: syntax.

Yes, I think it's more natural to have a prefix for each data-only
layer.  And this is also good for extensibility, as discussed.

Thanks,
Miklos

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Tue, May 30, 2023 at 5:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > If we would want to support data-only layers in the middle on the
> > stack, which would this syntax make sense?
> > lowerdir=lower1::data1:lower2::data2
> >
> > If this syntax makes sense to everyone, then we can change the syntax
> > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > and enforce that after the first ::, only :: are allowed.
> >
> > Miklos, any thoughts?
> > I have a feeling that this was your natural interpretation when you first
> > saw the :: syntax.
>
> Yes, I think it's more natural to have a prefix for each data-only
> layer.  And this is also good for extensibility, as discussed.
>

Good timing ;-)

I was just about to say that I changed the syntax and pushed to:

https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata-v3
https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata

The gist of the documentation of v3 is:

Below the top most lower layer, any number of lower most layers may be defined
as "data-only" lower layers, using double colon ("::") separators.
A normal lower layer is not allowed to be below a data-only layer, so single
colon separators are not allowed to the right of double colon ("::") separators.

For example:

  mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1::/do2 /merged


Do you need me to post the v3 patches?

The changes since ovl-lazy-lowerdata-v2 branch are:
- Reabse on 6.4-rc2 + NULL deref fixes
- Syntax change

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Christian Brauner]

On Tue, May 30, 2023 at 04:08:38PM +0200, Miklos Szeredi wrote:
> On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> > If we would want to support data-only layers in the middle on the
> > stack, which would this syntax make sense?
> > lowerdir=lower1::data1:lower2::data2
> >
> > If this syntax makes sense to everyone, then we can change the syntax
> > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > and enforce that after the first ::, only :: are allowed.
> >
> > Miklos, any thoughts?
> > I have a feeling that this was your natural interpretation when you first
> > saw the :: syntax.
> 
> Yes, I think it's more natural to have a prefix for each data-only
> layer.  And this is also good for extensibility, as discussed.

Sorry, just a quick braindump vaguely related to this new mount syntax.

A while ago util-linux reported issues with overlayfs when mounted
through the new mount api (cf. [1]) and I completely forgot to mention
this to you during LSFMM. So say you do:

        fs_fd = fsopen("overlay", FSOPEN_CLOEXEC);

        fsconfig(fs_fd, FSCONFIG_SET_STRING, "upperdir", "/home/asavah/kross/tmp/asusb450eg/upper", 0);

        fsconfig(fs_fd, FSCONFIG_SET_STRING, "workdir", "/tmp/work", 0);

        fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "0:1:2:3:4:5:6:7:8:9:a:b:c:d:e:f:10:11:12:13:14:15:16:17:18:19:1a:1b:1c:1d:1e:1f:20:21:22:23:24:25:26:27:28:29:2a:2b:2c:2d:2e:2f:

This will fail because FSCONFIG_SET_STRING is limited to 256 bytes.
That's a reasonable limit and I don't think we need to extend this to
PATH_MAX.

Instead, my reaction had been that lowerdir should be specifiable
multiple times through fsconfig() and overlayfs should probably append
lower layers via:

        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
        // append 
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data2", 0);
        // append
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data3", 0);

        // replace everything specified until now
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data4", 0);

        // reset everything
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "", 0);

so with the new syntax this would probably be:

        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
        // append data only layer
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data2", 0);
        // append data only layer
        ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data3", 0);

[1]: https://github.com/util-linux/util-linux/issues/1992

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Miklos Szeredi]

On Tue, 30 May 2023 at 16:15, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, May 30, 2023 at 5:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > If we would want to support data-only layers in the middle on the
> > > stack, which would this syntax make sense?
> > > lowerdir=lower1::data1:lower2::data2
> > >
> > > If this syntax makes sense to everyone, then we can change the syntax
> > > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > > and enforce that after the first ::, only :: are allowed.
> > >
> > > Miklos, any thoughts?
> > > I have a feeling that this was your natural interpretation when you first
> > > saw the :: syntax.
> >
> > Yes, I think it's more natural to have a prefix for each data-only
> > layer.  And this is also good for extensibility, as discussed.
> >
>
> Good timing ;-)
>
> I was just about to say that I changed the syntax and pushed to:
>
> https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata-v3
> https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
>
> The gist of the documentation of v3 is:
>
> Below the top most lower layer, any number of lower most layers may be defined
> as "data-only" lower layers, using double colon ("::") separators.
> A normal lower layer is not allowed to be below a data-only layer, so single
> colon separators are not allowed to the right of double colon ("::") separators.
>
> For example:
>
>   mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1::/do2 /merged
>
>
> Do you need me to post the v3 patches?
>
> The changes since ovl-lazy-lowerdata-v2 branch are:
> - Reabse on 6.4-rc2 + NULL deref fixes
> - Syntax change

Patches look good to me.

Pushed v3 to overlayfs-next.

It'd be interesting to hear what obstacles you encountered when trying
to implement generic lazy lookup.  I can put that into the pull
request so the information is not lost.

Thanks,
Miklos

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Alexander Larsson]

On Tue, May 30, 2023 at 6:19 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, May 30, 2023 at 04:08:38PM +0200, Miklos Szeredi wrote:
> > On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > If we would want to support data-only layers in the middle on the
> > > stack, which would this syntax make sense?
> > > lowerdir=lower1::data1:lower2::data2
> > >
> > > If this syntax makes sense to everyone, then we can change the syntax
> > > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > > and enforce that after the first ::, only :: are allowed.
> > >
> > > Miklos, any thoughts?
> > > I have a feeling that this was your natural interpretation when you first
> > > saw the :: syntax.
> >
> > Yes, I think it's more natural to have a prefix for each data-only
> > layer.  And this is also good for extensibility, as discussed.
>
> Sorry, just a quick braindump vaguely related to this new mount syntax.
>
> A while ago util-linux reported issues with overlayfs when mounted
> through the new mount api (cf. [1]) and I completely forgot to mention
> this to you during LSFMM. So say you do:
>
>         fs_fd = fsopen("overlay", FSOPEN_CLOEXEC);
>
>         fsconfig(fs_fd, FSCONFIG_SET_STRING, "upperdir", "/home/asavah/kross/tmp/asusb450eg/upper", 0);
>
>         fsconfig(fs_fd, FSCONFIG_SET_STRING, "workdir", "/tmp/work", 0);
>
>         fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "0:1:2:3:4:5:6:7:8:9:a:b:c:d:e:f:10:11:12:13:14:15:16:17:18:19:1a:1b:1c:1d:1e:1f:20:21:22:23:24:25:26:27:28:29:2a:2b:2c:2d:2e:2f:
>
> This will fail because FSCONFIG_SET_STRING is limited to 256 bytes.
> That's a reasonable limit and I don't think we need to extend this to
> PATH_MAX.
>
> Instead, my reaction had been that lowerdir should be specifiable
> multiple times through fsconfig() and overlayfs should probably append
> lower layers via:
>
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
>         // append
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data2", 0);
>         // append
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data3", 0);
>
>         // replace everything specified until now
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data4", 0);
>
>         // reset everything
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "", 0);
>
> so with the new syntax this would probably be:
>
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
>         // append data only layer
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data2", 0);
>         // append data only layer
>         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data3", 0);
>
> [1]: https://github.com/util-linux/util-linux/issues/1992

Btw. I forgot to mention this, but I ran into an issue with overlayfs
and the new mount api. In order to be able to pass in any pathname in
the
lowerdir option, overlayfs escapes commas, like:

  -o lowerdir=/lower/dir/with\,commas,upperdir=/upper

This is handled in overlayfs in ovl_split_lowerdirs() and ovl_unescape().

However, when using the new mount APIs, currently overlayfs uses
legacy_fs_context_ops, and legacy_parse_param() forbids commas in the
string, even if it is escaped. So the above mount will fail with "VFS:
Legacy: Option '%s' contained comma".

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                Red Hat, Inc
       alexl@redhat.com         alexander.larsson@gmail.com

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Christian Brauner]

On Fri, Jun 09, 2023 at 10:17:35AM +0200, Alexander Larsson wrote:
> On Tue, May 30, 2023 at 6:19 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, May 30, 2023 at 04:08:38PM +0200, Miklos Szeredi wrote:
> > > On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > If we would want to support data-only layers in the middle on the
> > > > stack, which would this syntax make sense?
> > > > lowerdir=lower1::data1:lower2::data2
> > > >
> > > > If this syntax makes sense to everyone, then we can change the syntax
> > > > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > > > and enforce that after the first ::, only :: are allowed.
> > > >
> > > > Miklos, any thoughts?
> > > > I have a feeling that this was your natural interpretation when you first
> > > > saw the :: syntax.
> > >
> > > Yes, I think it's more natural to have a prefix for each data-only
> > > layer.  And this is also good for extensibility, as discussed.
> >
> > Sorry, just a quick braindump vaguely related to this new mount syntax.
> >
> > A while ago util-linux reported issues with overlayfs when mounted
> > through the new mount api (cf. [1]) and I completely forgot to mention
> > this to you during LSFMM. So say you do:
> >
> >         fs_fd = fsopen("overlay", FSOPEN_CLOEXEC);
> >
> >         fsconfig(fs_fd, FSCONFIG_SET_STRING, "upperdir", "/home/asavah/kross/tmp/asusb450eg/upper", 0);
> >
> >         fsconfig(fs_fd, FSCONFIG_SET_STRING, "workdir", "/tmp/work", 0);
> >
> >         fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "0:1:2:3:4:5:6:7:8:9:a:b:c:d:e:f:10:11:12:13:14:15:16:17:18:19:1a:1b:1c:1d:1e:1f:20:21:22:23:24:25:26:27:28:29:2a:2b:2c:2d:2e:2f:
> >
> > This will fail because FSCONFIG_SET_STRING is limited to 256 bytes.
> > That's a reasonable limit and I don't think we need to extend this to
> > PATH_MAX.
> >
> > Instead, my reaction had been that lowerdir should be specifiable
> > multiple times through fsconfig() and overlayfs should probably append
> > lower layers via:
> >
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
> >         // append
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data2", 0);
> >         // append
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/home/username/project/data3", 0);
> >
> >         // replace everything specified until now
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data4", 0);
> >
> >         // reset everything
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "", 0);
> >
> > so with the new syntax this would probably be:
> >
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/home/username/project/data1", 0);
> >         // append data only layer
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data2", 0);
> >         // append data only layer
> >         ret = fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "::/home/username/project/data3", 0);
> >
> > [1]: https://github.com/util-linux/util-linux/issues/1992
> 
> Btw. I forgot to mention this, but I ran into an issue with overlayfs
> and the new mount api. In order to be able to pass in any pathname in
> the
> lowerdir option, overlayfs escapes commas, like:
> 
>   -o lowerdir=/lower/dir/with\,commas,upperdir=/upper
> 
> This is handled in overlayfs in ovl_split_lowerdirs() and ovl_unescape().
> 
> However, when using the new mount APIs, currently overlayfs uses
> legacy_fs_context_ops, and legacy_parse_param() forbids commas in the
> string, even if it is escaped. So the above mount will fail with "VFS:
> Legacy: Option '%s' contained comma".

I've sent a patch yesterday that converts overlayfs to the new mount api
that would also fix this fwiw.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Fri, Jun 9, 2023 at 10:24 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 30 May 2023 at 16:15, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, May 30, 2023 at 5:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > If we would want to support data-only layers in the middle on the
> > > > stack, which would this syntax make sense?
> > > > lowerdir=lower1::data1:lower2::data2
> > > >
> > > > If this syntax makes sense to everyone, then we can change the syntax
> > > > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > > > and enforce that after the first ::, only :: are allowed.
> > > >
> > > > Miklos, any thoughts?
> > > > I have a feeling that this was your natural interpretation when you first
> > > > saw the :: syntax.
> > >
> > > Yes, I think it's more natural to have a prefix for each data-only
> > > layer.  And this is also good for extensibility, as discussed.
> > >
> >
> > Good timing ;-)
> >
> > I was just about to say that I changed the syntax and pushed to:
> >
> > https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata-v3
> > https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
> >
> > The gist of the documentation of v3 is:
> >
> > Below the top most lower layer, any number of lower most layers may be defined
> > as "data-only" lower layers, using double colon ("::") separators.
> > A normal lower layer is not allowed to be below a data-only layer, so single
> > colon separators are not allowed to the right of double colon ("::") separators.
> >
> > For example:
> >
> >   mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1::/do2 /merged
> >
> >
> > Do you need me to post the v3 patches?
> >
> > The changes since ovl-lazy-lowerdata-v2 branch are:
> > - Reabse on 6.4-rc2 + NULL deref fixes
> > - Syntax change
>
> Patches look good to me.
>
> Pushed v3 to overlayfs-next.
>
> It'd be interesting to hear what obstacles you encountered when trying
> to implement generic lazy lookup.  I can put that into the pull
> request so the information is not lost.
>

Depends on what you mean by "generic lazy lookup".

Generic lazy lookup in the sense that it does lazy lookup to lower
(not to lowerdata) is challenging, because we use lower inode
number for many things including hashing the overlay inode,
which is done during lookup.

The challenges with generic lazy lowerdata lookup in layers
that are not data-only is that there is more state to store for
the lazy lookup:
- Which layer we stopped lookup
- The current path, if not an absolute redirect
- Maybe more?

And the lazy lookup will have to resume the ovl layers lookup
state machine on access including checking redirects,
metacopy, etc.

The current code only stores an optional absolute lowerdata_redirect
string on ovl_lookup() only for the case of lowerdata lookup in data-only
layers and lazy lookup calls a single vfs helper vfs_path_lookup() per
data-only layer - it is quite trivial.

No obstacles that I am aware of - only more work that
is not driven by request from users.

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Fri, Jun 9, 2023 at 10:24 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 30 May 2023 at 16:15, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, May 30, 2023 at 5:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sat, 27 May 2023 at 16:04, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > If we would want to support data-only layers in the middle on the
> > > > stack, which would this syntax make sense?
> > > > lowerdir=lower1::data1:lower2::data2
> > > >
> > > > If this syntax makes sense to everyone, then we can change the syntax
> > > > of data-only in the tail from lower1::data1:data2 to lower1::data1::data2
> > > > and enforce that after the first ::, only :: are allowed.
> > > >
> > > > Miklos, any thoughts?
> > > > I have a feeling that this was your natural interpretation when you first
> > > > saw the :: syntax.
> > >
> > > Yes, I think it's more natural to have a prefix for each data-only
> > > layer.  And this is also good for extensibility, as discussed.
> > >
> >
> > Good timing ;-)
> >
> > I was just about to say that I changed the syntax and pushed to:
> >
> > https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata-v3
> > https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata
> >
> > The gist of the documentation of v3 is:
> >
> > Below the top most lower layer, any number of lower most layers may be defined
> > as "data-only" lower layers, using double colon ("::") separators.
> > A normal lower layer is not allowed to be below a data-only layer, so single
> > colon separators are not allowed to the right of double colon ("::") separators.
> >
> > For example:
> >
> >   mount -t overlay overlay -olowerdir=/l1:/l2:/l3::/do1::/do2 /merged
> >
> >
> > Do you need me to post the v3 patches?
> >
> > The changes since ovl-lazy-lowerdata-v2 branch are:
> > - Reabse on 6.4-rc2 + NULL deref fixes
> > - Syntax change
>
> Patches look good to me.
>
> Pushed v3 to overlayfs-next.
>

Miklos,

I see you pushed the branch as is.

Please be warned that it contains the following unexplained
merge commit:

commit b892fac09d57668181ff5c433958e96ec7755453
Merge: f1fcbaa18b28 7cdafe6cc4a6
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Thu May 25 15:14:13 2023 +0300

    Merge remote-tracking branch 'jack/fsnotify' into next

And you know how Linus hates unexplained merge commits.

In this case, it is unexplained and also does not have a
good reason in the context of an ovl pull request.

Thanks,
Amir.

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Miklos Szeredi]

On Fri, 9 Jun 2023 at 15:42, Amir Goldstein <amir73il@gmail.com> wrote:

> Miklos,
>
> I see you pushed the branch as is.
>
> Please be warned that it contains the following unexplained
> merge commit:
>
> commit b892fac09d57668181ff5c433958e96ec7755453
> Merge: f1fcbaa18b28 7cdafe6cc4a6
> Author: Amir Goldstein <amir73il@gmail.com>
> Date:   Thu May 25 15:14:13 2023 +0300
>
>     Merge remote-tracking branch 'jack/fsnotify' into next
>
> And you know how Linus hates unexplained merge commits.
>
> In this case, it is unexplained and also does not have a
> good reason in the context of an ovl pull request.

Yes, I will redo this, but for getting into -next this will do.

Thanks,
Miklos

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

On Fri, Jun 9, 2023 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 9 Jun 2023 at 15:42, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Miklos,
> >
> > I see you pushed the branch as is.
> >
> > Please be warned that it contains the following unexplained
> > merge commit:
> >
> > commit b892fac09d57668181ff5c433958e96ec7755453
> > Merge: f1fcbaa18b28 7cdafe6cc4a6
> > Author: Amir Goldstein <amir73il@gmail.com>
> > Date:   Thu May 25 15:14:13 2023 +0300
> >
> >     Merge remote-tracking branch 'jack/fsnotify' into next
> >
> > And you know how Linus hates unexplained merge commits.
> >
> > In this case, it is unexplained and also does not have a
> > good reason in the context of an ovl pull request.
>
> Yes, I will redo this, but for getting into -next this will do.
>

Miklos,

I found a memory leak in these patches (reported by kmemleak).
For negative dentries, oe was allocated but not stored and not
freed.

Below is diff -w of the fix.
I squashed this fix into:
"ovl: move ovl_entry into ovl_inode"
and pushed the fixed overlayfs-next to my github [1]
I've also reabsed overlayfs-next onto 6.4-rc6.

Let me know if you want me to ask Steven to pull the fixed
branch into linux-next.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fs-overlayfs-mount_api

--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -938,7 +938,7 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                          unsigned int flags)
 {
-       struct ovl_entry *oe;
+       struct ovl_entry *oe = NULL;
        const struct cred *old_cred;
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *poe = OVL_E(dentry->d_parent);
@@ -1180,12 +1180,14 @@ struct dentry *ovl_lookup(struct inode *dir,
struct dentry *dentry,
                }
        }

+       if (ctr) {
                oe = ovl_alloc_entry(ctr);
                err = -ENOMEM;
                if (!oe)
                        goto out_put;

                ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
+       }

Re: [PATCH v2 00/13] Overlayfs lazy lookup of lowerdata

[Miklos Szeredi]

On Sat, 17 Jun 2023 at 19:40, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 9, 2023 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Fri, 9 Jun 2023 at 15:42, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > Miklos,
> > >
> > > I see you pushed the branch as is.
> > >
> > > Please be warned that it contains the following unexplained
> > > merge commit:
> > >
> > > commit b892fac09d57668181ff5c433958e96ec7755453
> > > Merge: f1fcbaa18b28 7cdafe6cc4a6
> > > Author: Amir Goldstein <amir73il@gmail.com>
> > > Date:   Thu May 25 15:14:13 2023 +0300
> > >
> > >     Merge remote-tracking branch 'jack/fsnotify' into next
> > >
> > > And you know how Linus hates unexplained merge commits.
> > >
> > > In this case, it is unexplained and also does not have a
> > > good reason in the context of an ovl pull request.
> >
> > Yes, I will redo this, but for getting into -next this will do.
> >
>
> Miklos,
>
> I found a memory leak in these patches (reported by kmemleak).
> For negative dentries, oe was allocated but not stored and not
> freed.
>
> Below is diff -w of the fix.
> I squashed this fix into:
> "ovl: move ovl_entry into ovl_inode"
> and pushed the fixed overlayfs-next to my github [1]
> I've also reabsed overlayfs-next onto 6.4-rc6.
>
> Let me know if you want me to ask Steven to pull the fixed
> branch into linux-next.

I pushed this to overlayfs-next.  I'm still reviewing the rest...

Thanks,
Miklos