[PATCH 0/7] Prepare for lazy lowerdata lookup

[PATCH 0/7] Prepare for lazy lowerdata lookup

[Amir Goldstein]

Miklos,

This series is a cleanup towards the lazy lowerdata lookup patches that
I mentioned in an earlier email [1].  The lazy lowerdata patches are
ready including tests, but I am waiting for your feedback on the
data-only layers before I post the complete series.

I am posting this cleanup series independently, because it mostly [*]
stands on its own.

Specifically, patch #1 is a bug fix for stable.

Feel free to take just patch #1, or only part of the cleanup or wait for
the posting of the complete work.

Thanks,
Amir.

[*] The last patch, which reserves the space for lowerdata_redirect is
not completely independent of the lazy lowerdata series, but I preferred
to do it this way then to remove the lowerdata inode union field and add
lowerdata_redirect later, because I think the change is easier to review
this way.

[1] https://lore.kernel.org/linux-unionfs/CAOQ4uxich227fP7bGSCNqx-JX5h36O-MLwqPoy0r33tuH=z2cA@mail.gmail.com/

Amir Goldstein (7):
  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[0]
  ovl: replace lowerdata inode reference with lowerdata redirect

 fs/overlayfs/copy_up.c   |   2 +
 fs/overlayfs/dir.c       |   5 +-
 fs/overlayfs/export.c    |  37 ++++-----
 fs/overlayfs/inode.c     |  20 ++---
 fs/overlayfs/namei.c     |  75 +++++++++---------
 fs/overlayfs/overlayfs.h |  24 ++++--
 fs/overlayfs/ovl_entry.h |  63 +++++++++++----
 fs/overlayfs/super.c     |  86 +++++++-------------
 fs/overlayfs/util.c      | 164 +++++++++++++++++++++++++++++----------
 9 files changed, 281 insertions(+), 195 deletions(-)

-- 
2.34.1

[PATCH 1/7] 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")
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 2/7] 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.

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 3/7] ovl: use ovl_numlower() and ovl_lowerstack() accessors

[Amir Goldstein]

This helps fortify against dereferencing a NULL ovl_entry.

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..f456a99d6017 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 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 4/7] 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 transfered, so cleanup
always needs to call ovl_stack_free(stack).

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 f456a99d6017..754f8ae4ce62 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 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 5/7] 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.

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..d4caf57c8e17 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)
+		goto nomem;
+
 	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 754f8ae4ce62..201de9da45d3 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 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 6/7] ovl: deduplicate lowerpath and lowerstack[0]

[Amir Goldstein]

For the common case of single lower layer, embed ovl_entry with a
single lower path in ovl_inode, so no stack allocation is needed.

For directory with more than single lower layer and for regular file
with lowerdata, the lower stack is stored in an external allocation.

Use accessor ovl_lowerstack() to get the embedded or external stack.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       |  2 ++
 fs/overlayfs/export.c    | 18 +++++----------
 fs/overlayfs/inode.c     | 12 ++++------
 fs/overlayfs/namei.c     | 15 +++++--------
 fs/overlayfs/overlayfs.h | 10 +++++----
 fs/overlayfs/ovl_entry.h | 14 +++++++-----
 fs/overlayfs/super.c     | 41 +++++++++++++---------------------
 fs/overlayfs/util.c      | 48 +++++++++++++++++++++++++++++-----------
 8 files changed, 81 insertions(+), 79 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 92bdcedfaaec..aa0465c61064 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -262,9 +262,11 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
 static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 			   struct dentry *newdentry, bool hardlink)
 {
+	struct ovl_entry oe = {};
 	struct ovl_inode_params oip = {
 		.upperdentry = newdentry,
 		.newinode = inode,
+		.oe = &oe,
 	};
 
 	ovl_dir_modified(dentry->d_parent, false);
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index d4caf57c8e17..9951c504fb8d 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -287,30 +287,22 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	struct dentry *upper = upper_alias ?: index;
 	struct dentry *dentry;
 	struct inode *inode = NULL;
-	struct ovl_entry *oe;
+	struct ovl_entry oe;
 	struct ovl_inode_params oip = {
-		.lowerpath = lowerpath,
+		.oe = &oe,
 		.index = index,
-		.numlower = !!lower
 	};
 
 	/* We get overlay directory dentries with ovl_lookup_real() */
 	if (d_is_dir(upper ?: lower))
 		return ERR_PTR(-EIO);
 
-	oe = ovl_alloc_entry(!!lower);
-	if (!oe)
-		goto nomem;
-
 	oip.upperdentry = dget(upper);
-	if (lower) {
-		ovl_lowerstack(oe)->dentry = dget(lower);
-		ovl_lowerstack(oe)->layer = lowerpath->layer;
-	}
-	oip.oe = oe;
+	/* Should not fail because does not allocate lowerstack */
+	ovl_init_entry(&oe, lowerpath, !!lower);
 	inode = ovl_get_inode(sb, &oip);
 	if (IS_ERR(inode)) {
-		ovl_free_entry(oe);
+		ovl_destroy_entry(&oe);
 		dput(upper);
 		return ERR_CAST(inode);
 	}
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index c296bd656858..9f29fc3e9fa5 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1004,12 +1004,8 @@ void ovl_inode_init(struct inode *inode, struct ovl_inode_params *oip,
 	struct ovl_inode *oi = OVL_I(inode);
 
 	oi->__upperdentry = oip->upperdentry;
-	oi->oe = oip->oe;
+	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_lowerstack(oip->oe);
 	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
 	struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
@@ -1370,7 +1366,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			}
 
 			dput(upperdentry);
-			ovl_free_entry(oip->oe);
+			ovl_destroy_entry(oip->oe);
 			kfree(oip->redirect);
 			goto out;
 		}
@@ -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..cdcb2ac5d95c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -831,7 +831,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
-	struct ovl_entry *oe;
+	struct ovl_entry oe;
 	const struct cred *old_cred;
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct ovl_entry *poe = OVL_E(dentry->d_parent);
@@ -1067,13 +1067,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	oe = ovl_alloc_entry(ctr);
-	err = -ENOMEM;
-	if (!oe)
+	err = ovl_init_entry(&oe, stack, ctr);
+	if (err)
 		goto out_put;
 
-	ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
-
 	if (upperopaque)
 		ovl_dentry_set_opaque(dentry);
 
@@ -1105,10 +1102,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,
+			.oe = &oe,
 			.index = index,
-			.numlower = ctr,
 			.redirect = upperredirect,
 			.lowerdata = (ctr > 1 && !d.is_dir) ?
 				      stack[ctr - 1].dentry : NULL,
@@ -1135,7 +1130,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	return d_splice_alias(inode, dentry);
 
 out_free_oe:
-	ovl_free_entry(oe);
+	ovl_destroy_entry(&oe);
 out_put:
 	dput(index);
 	ovl_stack_free(stack, ctr);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e14ca0fd1063..32532342e56a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -377,8 +377,12 @@ 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);
+struct ovl_path *ovl_alloc_stack(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);
+int ovl_init_entry(struct ovl_entry *oe, struct ovl_path *stack,
+		   unsigned int numlower);
+void ovl_destroy_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,
@@ -653,10 +657,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 201de9da45d3..5d95e937f555 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -49,7 +49,12 @@ struct ovl_path {
 
 struct ovl_entry {
 	unsigned int __numlower;
-	struct ovl_path __lowerstack[];
+	union {
+		/* Embedded path for numlower == 1 */
+		struct ovl_path __lowerpath;
+		/* External stack for numlower > 1 */
+		struct ovl_path *__lowerstack;
+	};
 };
 
 /* private information held for overlayfs's superblock */
@@ -117,7 +122,7 @@ static inline unsigned int ovl_numlower(struct ovl_entry *oe)
 
 static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
 {
-	return oe ? oe->__lowerstack : NULL;
+	return oe && oe->__numlower > 1 ? oe->__lowerstack : &oe->__lowerpath;
 }
 
 /* private information held for every overlayfs dentry */
@@ -136,8 +141,7 @@ struct ovl_inode {
 	unsigned long flags;
 	struct inode vfs_inode;
 	struct dentry *__upperdentry;
-	struct ovl_path lowerpath;
-	struct ovl_entry *oe;
+	struct ovl_entry oe;
 
 	/* synchronize copy up and more */
 	struct mutex lock;
@@ -150,7 +154,7 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
 
 static inline struct ovl_entry *OVL_I_E(struct inode *inode)
 {
-	return inode ? OVL_I(inode)->oe : NULL;
+	return inode ? &OVL_I(inode)->oe : NULL;
 }
 
 static inline struct ovl_entry *OVL_E(struct dentry *dentry)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b9e62ccd609f..e01a76de787c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -171,9 +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;
+	ovl_init_entry(&oi->oe, NULL, 0);
 	oi->lowerdata = NULL;
 	mutex_init(&oi->lock);
 
@@ -194,8 +192,7 @@ 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);
+	ovl_destroy_entry(&oi->oe);
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
 	else
@@ -1702,7 +1699,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	return err;
 }
 
-static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
+static int ovl_get_lowerstack(struct super_block *sb, struct ovl_entry *oe,
 				const char *lower, unsigned int numlower,
 				struct ovl_fs *ofs, struct ovl_layer *layers)
 {
@@ -1710,16 +1707,15 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	struct path *stack = NULL;
 	struct ovl_path *lowerstack;
 	unsigned int i;
-	struct ovl_entry *oe;
 
 	if (!ofs->config.upperdir && numlower == 1) {
 		pr_err("at least 2 lowerdir are needed while upperdir nonexistent\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	stack = kcalloc(numlower, sizeof(struct path), GFP_KERNEL);
 	if (!stack)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	err = -EINVAL;
 	for (i = 0; i < numlower; i++) {
@@ -1741,9 +1737,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (err)
 		goto out_err;
 
-	err = -ENOMEM;
-	oe = ovl_alloc_entry(numlower);
-	if (!oe)
+	err = ovl_init_entry(oe, NULL, numlower);
+	if (err)
 		goto out_err;
 
 	lowerstack = ovl_lowerstack(oe);
@@ -1752,16 +1747,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		lowerstack[i].layer = &ofs->layers[i+1];
 	}
 
-out:
+out_err:
 	for (i = 0; i < numlower; i++)
 		path_put(&stack[i]);
 	kfree(stack);
 
-	return oe;
-
-out_err:
-	oe = ERR_PTR(err);
-	goto out;
+	return err;
 }
 
 /*
@@ -1847,7 +1838,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,
 	};
 
@@ -1878,7 +1868,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct path upperpath = { };
 	struct dentry *root_dentry;
-	struct ovl_entry *oe;
+	struct ovl_entry oe;
 	struct ovl_fs *ofs;
 	struct ovl_layer *layers;
 	struct cred *cred;
@@ -1991,9 +1981,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		sb->s_stack_depth = upper_sb->s_stack_depth;
 		sb->s_time_gran = upper_sb->s_time_gran;
 	}
-	oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers);
-	err = PTR_ERR(oe);
-	if (IS_ERR(oe))
+	err = ovl_get_lowerstack(sb, &oe, splitlower, numlower, ofs, layers);
+	if (err)
 		goto out_err;
 
 	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
@@ -2006,7 +1995,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	if (!ovl_force_readonly(ofs) && ofs->config.index) {
-		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
+		err = ovl_get_indexdir(sb, ofs, &oe, &upperpath);
 		if (err)
 			goto out_free_oe;
 
@@ -2047,7 +2036,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_iflags |= SB_I_SKIP_SYNC;
 
 	err = -ENOMEM;
-	root_dentry = ovl_get_root(sb, upperpath.dentry, oe);
+	root_dentry = ovl_get_root(sb, upperpath.dentry, &oe);
 	if (!root_dentry)
 		goto out_free_oe;
 
@@ -2059,7 +2048,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	return 0;
 
 out_free_oe:
-	ovl_free_entry(oe);
+	ovl_destroy_entry(&oe);
 out_err:
 	kfree(splitlower);
 	path_put(&upperpath);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f5e2c70a57f8..540819ac9b9c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -111,21 +111,41 @@ void ovl_stack_free(struct ovl_path *stack, unsigned int n)
 	kfree(stack);
 }
 
-struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
-{
-	size_t size = offsetof(struct ovl_entry, __lowerstack[numlower]);
-	struct ovl_entry *oe = kzalloc(size, GFP_KERNEL);
+/* On success, takes references on @stack dentries */
+int ovl_init_entry(struct ovl_entry *oe, struct ovl_path *stack,
+		   unsigned int numlower)
+{
+	oe->__numlower = numlower;
+	oe->__lowerpath = (struct ovl_path) {};
+
+	/* No allocated stack for numlower <= 1 */
+	if (numlower <= 1) {
+		if (numlower && stack)
+			oe->__lowerpath = *stack;
+		dget(oe->__lowerpath.dentry);
+		return 0;
+	}
+
+	oe->__lowerstack = ovl_stack_alloc(numlower);
+	if (!oe->__lowerstack)
+		return -ENOMEM;
+
+	if (!stack)
+		return 0;
 
-	if (oe)
-		oe->__numlower = numlower;
+	ovl_stack_cpy(oe->__lowerstack, stack, numlower);
 
-	return oe;
+	return 0;
 }
 
-void ovl_free_entry(struct ovl_entry *oe)
+void ovl_destroy_entry(struct ovl_entry *oe)
 {
-	ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
-	kfree(oe);
+	if (oe->__numlower > 1) {
+		ovl_stack_put(oe->__lowerstack, oe->__numlower);
+		kfree(oe->__lowerstack);
+	} else {
+		dput(oe->__lowerpath.dentry);
+	}
 }
 
 #define OVL_D_REVALIDATE (DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE)
@@ -306,10 +326,12 @@ struct dentry *ovl_i_dentry_upper(struct inode *inode)
 
 void ovl_i_path_real(struct inode *inode, struct path *path)
 {
+	struct ovl_path *lowerstack = ovl_lowerstack(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 = lowerstack->dentry;
+		path->mnt = lowerstack->layer->mnt;
 	} else {
 		path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
 	}
@@ -324,7 +346,7 @@ 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 dentry *lowerdentry = ovl_lowerstack(OVL_I_E(inode))->dentry;
 
 	return lowerdentry ? d_inode(lowerdentry) : NULL;
 }
-- 
2.34.1

[PATCH 7/7] ovl: replace lowerdata inode reference with lowerdata redirect

[Amir Goldstein]

Now that we have the entire lower stack in ovl_inode, we do not
need to hold another reference to the lowerdata inode.

Instead, use the vacant ovl_inode space as a place holder for lowerdata
redirect path from the metacopy to lowerdata, which is going to be used
later on for lazy lowerdata lookup.

Use accessors to get the lowerdata path and dentry.

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 9f29fc3e9fa5..35d51a6dced7 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -1006,8 +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;
-	if (oip->lowerdata)
-		oi->lowerdata = igrab(d_inode(oip->lowerdata));
+	oi->lowerdata_redirect = oip->lowerdata_redirect;
 
 	realinode = ovl_inode_real(inode);
 	ovl_copyattr(inode);
@@ -1368,6 +1367,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 			dput(upperdentry);
 			ovl_destroy_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 cdcb2ac5d95c..b629261324f1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1105,10 +1105,13 @@ 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,
 		};
 
+		/* 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 32532342e56a..011b7b466f70 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -409,6 +409,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);
@@ -660,7 +661,7 @@ struct ovl_inode_params {
 	struct ovl_entry *oe;
 	bool index;
 	char *redirect;
-	struct dentry *lowerdata;
+	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 5d95e937f555..221f0cbe748e 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -125,6 +125,20 @@ static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
 	return oe && oe->__numlower > 1 ? oe->__lowerstack : &oe->__lowerpath;
 }
 
+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 */
+		const char *lowerdata_redirect;	/* regular file */
 	};
 	const char *redirect;
 	u64 version;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e01a76de787c..6e4231799b86 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -172,7 +172,7 @@ static struct inode *ovl_alloc_inode(struct super_block *sb)
 	oi->flags = 0;
 	oi->__upperdentry = NULL;
 	ovl_init_entry(&oi->oe, NULL, 0);
-	oi->lowerdata = NULL;
+	oi->lowerdata_redirect = NULL;
 	mutex_init(&oi->lock);
 
 	return &oi->vfs_inode;
@@ -196,7 +196,7 @@ static void ovl_destroy_inode(struct inode *inode)
 	if (S_ISDIR(inode->i_mode))
 		ovl_dir_cache_free(inode);
 	else
-		iput(oi->lowerdata);
+		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 540819ac9b9c..fe2e5a8b216b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -245,11 +245,12 @@ 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);
+	struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
 
-	if (ovl_numlower(oe)) {
-		path->mnt = lowerstack[ovl_numlower(oe) - 1].layer->mnt;
-		path->dentry = lowerstack[ovl_numlower(oe) - 1].dentry;
+	if (lowerdata_dentry) {
+		path->dentry = lowerdata_dentry;
+		path->mnt = lowerdata->layer->mnt;
 	} else {
 		*path = (struct path) { };
 	}
@@ -308,10 +309,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)
@@ -359,10 +357,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 */
@@ -377,9 +377,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

Re: [PATCH 1/7] ovl: update of dentry revalidate flags after copy up

[Gao Xiang]

On 2023/4/9 00:42, Amir Goldstein wrote:
> 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")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.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;

Although I'm not sure if it could cause some lazy awareness due to dcache
RCU-walk, but maybe that is fine since such window is small?

Thanks,
Gao Xiang

Re: [PATCH 2/7] ovl: use OVL_E() and OVL_E_FLAGS() accessors

[Alexander Larsson]

On Sat, 2023-04-08 at 19:42 +0300, Amir Goldstein wrote:
> Instead of open coded instances, because we are about to split
> the two apart.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Alexander Larsson <alexl@redhat.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;

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a one-legged devious stage actor for the 21st century. She's a 
mistrustful blonde research scientist prone to fits of savage, 
blood-crazed rage. They fight crime! 

Re: [PATCH 3/7] ovl: use ovl_numlower() and ovl_lowerstack() accessors

[Alexander Larsson]

On Sat, 2023-04-08 at 19:42 +0300, Amir Goldstein wrote:
> This helps fortify against dereferencing a NULL ovl_entry.

It may help to foreshadow here why there might be NULL ovl_entries.

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Alexander Larsson <alexl@redhat.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..f456a99d6017 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 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)

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an otherworldly zombie househusband who knows the secret of the 
alien invasion. She's a beautiful cigar-chomping hooker with the power
to 
bend men's minds. They fight crime! 

Re: [PATCH 4/7] ovl: factor out ovl_free_entry() and ovl_stack_*() helpers

[Alexander Larsson]

On Sat, 2023-04-08 at 19:42 +0300, Amir Goldstein wrote:
> 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 transfered, so cleanup
> always needs to call ovl_stack_free(stack).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Alexander Larsson <alexl@redhat.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);

I realize this is not really related to your patch, but why is this
using ofs->numlayer - 1, rather than ovl_numlower(poe)? The later is
what we iterate over below, and is potentially smaller than numlayers.

>                 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 f456a99d6017..754f8ae4ce62 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 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)

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an oversexed devious romance novelist who hangs with the wrong 
crowd. She's a radical red-headed doctor prone to fits of savage, 
blood-crazed rage. They fight crime! 

Re: [PATCH 5/7] ovl: move ovl_entry into ovl_inode

[Alexander Larsson]

On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> 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.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>


Reviewed-by: Alexander Larsson <alexl@redhat.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..d4caf57c8e17 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)
> +               goto nomem;

You goto nomem here, but that will dput(dentry), and dentry is not
initialized yet. I think you should just return an error directly here.

> +
>         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 754f8ae4ce62..201de9da45d3 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 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;
>  

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a notorious small-town vagrant haunted by an iconic dead American 
confidante She's a radical junkie research scientist who don't take no 
shit from nobody. They fight crime! 

Re: [PATCH 6/7] ovl: deduplicate lowerpath and lowerstack[0]

[Alexander Larsson]

On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> For the common case of single lower layer, embed ovl_entry with a
> single lower path in ovl_inode, so no stack allocation is needed.
> 
> For directory with more than single lower layer and for regular file
> with lowerdata, the lower stack is stored in an external allocation.
> 
> Use accessor ovl_lowerstack() to get the embedded or external stack.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Alexander Larsson <alexl@redhat.com>

> ---
>  fs/overlayfs/dir.c       |  2 ++
>  fs/overlayfs/export.c    | 18 +++++----------
>  fs/overlayfs/inode.c     | 12 ++++------
>  fs/overlayfs/namei.c     | 15 +++++--------
>  fs/overlayfs/overlayfs.h | 10 +++++----
>  fs/overlayfs/ovl_entry.h | 14 +++++++-----
>  fs/overlayfs/super.c     | 41 +++++++++++++---------------------
>  fs/overlayfs/util.c      | 48 +++++++++++++++++++++++++++++---------
> --
>  8 files changed, 81 insertions(+), 79 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 92bdcedfaaec..aa0465c61064 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -262,9 +262,11 @@ static int ovl_set_opaque(struct dentry *dentry,
> struct dentry *upperdentry)
>  static int ovl_instantiate(struct dentry *dentry, struct inode
> *inode,
>                            struct dentry *newdentry, bool hardlink)
>  {
> +       struct ovl_entry oe = {};
>         struct ovl_inode_params oip = {
>                 .upperdentry = newdentry,
>                 .newinode = inode,
> +               .oe = &oe,
>         };
>  
>         ovl_dir_modified(dentry->d_parent, false);
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index d4caf57c8e17..9951c504fb8d 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -287,30 +287,22 @@ static struct dentry *ovl_obtain_alias(struct
> super_block *sb,
>         struct dentry *upper = upper_alias ?: index;
>         struct dentry *dentry;
>         struct inode *inode = NULL;
> -       struct ovl_entry *oe;
> +       struct ovl_entry oe;
>         struct ovl_inode_params oip = {
> -               .lowerpath = lowerpath,
> +               .oe = &oe,
>                 .index = index,
> -               .numlower = !!lower
>         };
>  
>         /* We get overlay directory dentries with ovl_lookup_real()
> */
>         if (d_is_dir(upper ?: lower))
>                 return ERR_PTR(-EIO);
>  
> -       oe = ovl_alloc_entry(!!lower);
> -       if (!oe)
> -               goto nomem;
> -

Ah, I see that the goto nomem goes away here, so I guess ignore my
comment on previous patch.


>         oip.upperdentry = dget(upper);
> -       if (lower) {
> -               ovl_lowerstack(oe)->dentry = dget(lower);
> -               ovl_lowerstack(oe)->layer = lowerpath->layer;
> -       }
> -       oip.oe = oe;
> +       /* Should not fail because does not allocate lowerstack */
> +       ovl_init_entry(&oe, lowerpath, !!lower);
>         inode = ovl_get_inode(sb, &oip);
>         if (IS_ERR(inode)) {
> -               ovl_free_entry(oe);
> +               ovl_destroy_entry(&oe);
>                 dput(upper);
>                 return ERR_CAST(inode);
>         }
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index c296bd656858..9f29fc3e9fa5 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -1004,12 +1004,8 @@ void ovl_inode_init(struct inode *inode,
> struct ovl_inode_params *oip,
>         struct ovl_inode *oi = OVL_I(inode);
>  
>         oi->__upperdentry = oip->upperdentry;
> -       oi->oe = oip->oe;
> +       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_lowerstack(oip->oe);
>         struct inode *realinode = upperdentry ? d_inode(upperdentry)
> : NULL;
>         struct inode *inode;
>         struct dentry *lowerdentry = lowerpath ? lowerpath->dentry :
> NULL;
> @@ -1370,7 +1366,7 @@ struct inode *ovl_get_inode(struct super_block
> *sb,
>                         }
>  
>                         dput(upperdentry);
> -                       ovl_free_entry(oip->oe);
> +                       ovl_destroy_entry(oip->oe);
>                         kfree(oip->redirect);
>                         goto out;
>                 }
> @@ -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..cdcb2ac5d95c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -831,7 +831,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs,
> struct dentry *dentry,
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags)
>  {
> -       struct ovl_entry *oe;
> +       struct ovl_entry oe;
>         const struct cred *old_cred;
>         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>         struct ovl_entry *poe = OVL_E(dentry->d_parent);
> @@ -1067,13 +1067,10 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>                 }
>         }
>  
> -       oe = ovl_alloc_entry(ctr);
> -       err = -ENOMEM;
> -       if (!oe)
> +       err = ovl_init_entry(&oe, stack, ctr);
> +       if (err)
>                 goto out_put;
>  
> -       ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
> -
>         if (upperopaque)
>                 ovl_dentry_set_opaque(dentry);
>  
> @@ -1105,10 +1102,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,
> +                       .oe = &oe,
>                         .index = index,
> -                       .numlower = ctr,
>                         .redirect = upperredirect,
>                         .lowerdata = (ctr > 1 && !d.is_dir) ?
>                                       stack[ctr - 1].dentry : NULL,
> @@ -1135,7 +1130,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>         return d_splice_alias(inode, dentry);
>  
>  out_free_oe:
> -       ovl_free_entry(oe);
> +       ovl_destroy_entry(&oe);
>  out_put:
>         dput(index);
>         ovl_stack_free(stack, ctr);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index e14ca0fd1063..32532342e56a 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -377,8 +377,12 @@ 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);
> +struct ovl_path *ovl_alloc_stack(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);
> +int ovl_init_entry(struct ovl_entry *oe, struct ovl_path *stack,
> +                  unsigned int numlower);
> +void ovl_destroy_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,
> @@ -653,10 +657,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 201de9da45d3..5d95e937f555 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -49,7 +49,12 @@ struct ovl_path {
>  
>  struct ovl_entry {
>         unsigned int __numlower;
> -       struct ovl_path __lowerstack[];
> +       union {
> +               /* Embedded path for numlower == 1 */
> +               struct ovl_path __lowerpath;
> +               /* External stack for numlower > 1 */
> +               struct ovl_path *__lowerstack;
> +       };
>  };
>  
>  /* private information held for overlayfs's superblock */
> @@ -117,7 +122,7 @@ static inline unsigned int ovl_numlower(struct
> ovl_entry *oe)
>  
>  static inline struct ovl_path *ovl_lowerstack(struct ovl_entry *oe)
>  {
> -       return oe ? oe->__lowerstack : NULL;
> +       return oe && oe->__numlower > 1 ? oe->__lowerstack : &oe-
> >__lowerpath;
>  }
>  
>  /* private information held for every overlayfs dentry */
> @@ -136,8 +141,7 @@ struct ovl_inode {
>         unsigned long flags;
>         struct inode vfs_inode;
>         struct dentry *__upperdentry;
> -       struct ovl_path lowerpath;
> -       struct ovl_entry *oe;
> +       struct ovl_entry oe;
>  
>         /* synchronize copy up and more */
>         struct mutex lock;
> @@ -150,7 +154,7 @@ static inline struct ovl_inode *OVL_I(struct
> inode *inode)
>  
>  static inline struct ovl_entry *OVL_I_E(struct inode *inode)
>  {
> -       return inode ? OVL_I(inode)->oe : NULL;
> +       return inode ? &OVL_I(inode)->oe : NULL;
>  }
>  
>  static inline struct ovl_entry *OVL_E(struct dentry *dentry)
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index b9e62ccd609f..e01a76de787c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -171,9 +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;
> +       ovl_init_entry(&oi->oe, NULL, 0);
>         oi->lowerdata = NULL;
>         mutex_init(&oi->lock);
>  
> @@ -194,8 +192,7 @@ 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);
> +       ovl_destroy_entry(&oi->oe);
>         if (S_ISDIR(inode->i_mode))
>                 ovl_dir_cache_free(inode);
>         else
> @@ -1702,7 +1699,7 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>         return err;
>  }
>  
> -static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
> +static int ovl_get_lowerstack(struct super_block *sb, struct
> ovl_entry *oe,
>                                 const char *lower, unsigned int
> numlower,
>                                 struct ovl_fs *ofs, struct ovl_layer
> *layers)
>  {
> @@ -1710,16 +1707,15 @@ static struct ovl_entry
> *ovl_get_lowerstack(struct super_block *sb,
>         struct path *stack = NULL;
>         struct ovl_path *lowerstack;
>         unsigned int i;
> -       struct ovl_entry *oe;
>  
>         if (!ofs->config.upperdir && numlower == 1) {
>                 pr_err("at least 2 lowerdir are needed while upperdir
> nonexistent\n");
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>         }
>  
>         stack = kcalloc(numlower, sizeof(struct path), GFP_KERNEL);
>         if (!stack)
> -               return ERR_PTR(-ENOMEM);
> +               return -ENOMEM;
>  
>         err = -EINVAL;
>         for (i = 0; i < numlower; i++) {
> @@ -1741,9 +1737,8 @@ static struct ovl_entry
> *ovl_get_lowerstack(struct super_block *sb,
>         if (err)
>                 goto out_err;
>  
> -       err = -ENOMEM;
> -       oe = ovl_alloc_entry(numlower);
> -       if (!oe)
> +       err = ovl_init_entry(oe, NULL, numlower);
> +       if (err)
>                 goto out_err;
>  
>         lowerstack = ovl_lowerstack(oe);
> @@ -1752,16 +1747,12 @@ static struct ovl_entry
> *ovl_get_lowerstack(struct super_block *sb,
>                 lowerstack[i].layer = &ofs->layers[i+1];
>         }
>  
> -out:
> +out_err:
>         for (i = 0; i < numlower; i++)
>                 path_put(&stack[i]);
>         kfree(stack);
>  
> -       return oe;
> -
> -out_err:
> -       oe = ERR_PTR(err);
> -       goto out;
> +       return err;
>  }
>  
>  /*
> @@ -1847,7 +1838,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,
>         };
>  
> @@ -1878,7 +1868,7 @@ static int ovl_fill_super(struct super_block
> *sb, void *data, int silent)
>  {
>         struct path upperpath = { };
>         struct dentry *root_dentry;
> -       struct ovl_entry *oe;
> +       struct ovl_entry oe;
>         struct ovl_fs *ofs;
>         struct ovl_layer *layers;
>         struct cred *cred;
> @@ -1991,9 +1981,8 @@ static int ovl_fill_super(struct super_block
> *sb, void *data, int silent)
>                 sb->s_stack_depth = upper_sb->s_stack_depth;
>                 sb->s_time_gran = upper_sb->s_time_gran;
>         }
> -       oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs,
> layers);
> -       err = PTR_ERR(oe);
> -       if (IS_ERR(oe))
> +       err = ovl_get_lowerstack(sb, &oe, splitlower, numlower, ofs,
> layers);
> +       if (err)
>                 goto out_err;
>  
>         /* If the upper fs is nonexistent, we mark overlayfs r/o too
> */
> @@ -2006,7 +1995,7 @@ static int ovl_fill_super(struct super_block
> *sb, void *data, int silent)
>         }
>  
>         if (!ovl_force_readonly(ofs) && ofs->config.index) {
> -               err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
> +               err = ovl_get_indexdir(sb, ofs, &oe, &upperpath);
>                 if (err)
>                         goto out_free_oe;
>  
> @@ -2047,7 +2036,7 @@ static int ovl_fill_super(struct super_block
> *sb, void *data, int silent)
>         sb->s_iflags |= SB_I_SKIP_SYNC;
>  
>         err = -ENOMEM;
> -       root_dentry = ovl_get_root(sb, upperpath.dentry, oe);
> +       root_dentry = ovl_get_root(sb, upperpath.dentry, &oe);
>         if (!root_dentry)
>                 goto out_free_oe;
>  
> @@ -2059,7 +2048,7 @@ static int ovl_fill_super(struct super_block
> *sb, void *data, int silent)
>         return 0;
>  
>  out_free_oe:
> -       ovl_free_entry(oe);
> +       ovl_destroy_entry(&oe);
>  out_err:
>         kfree(splitlower);
>         path_put(&upperpath);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f5e2c70a57f8..540819ac9b9c 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -111,21 +111,41 @@ void ovl_stack_free(struct ovl_path *stack,
> unsigned int n)
>         kfree(stack);
>  }
>  
> -struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
> -{
> -       size_t size = offsetof(struct ovl_entry,
> __lowerstack[numlower]);
> -       struct ovl_entry *oe = kzalloc(size, GFP_KERNEL);
> +/* On success, takes references on @stack dentries */
> +int ovl_init_entry(struct ovl_entry *oe, struct ovl_path *stack,
> +                  unsigned int numlower)
> +{
> +       oe->__numlower = numlower;
> +       oe->__lowerpath = (struct ovl_path) {};
> +
> +       /* No allocated stack for numlower <= 1 */
> +       if (numlower <= 1) {
> +               if (numlower && stack)
> +                       oe->__lowerpath = *stack;
> +               dget(oe->__lowerpath.dentry);
> +               return 0;
> +       }
> +
> +       oe->__lowerstack = ovl_stack_alloc(numlower);
> +       if (!oe->__lowerstack)
> +               return -ENOMEM;
> +
> +       if (!stack)
> +               return 0;
>  
> -       if (oe)
> -               oe->__numlower = numlower;
> +       ovl_stack_cpy(oe->__lowerstack, stack, numlower);
>  
> -       return oe;
> +       return 0;
>  }
>  
> -void ovl_free_entry(struct ovl_entry *oe)
> +void ovl_destroy_entry(struct ovl_entry *oe)
>  {
> -       ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
> -       kfree(oe);
> +       if (oe->__numlower > 1) {
> +               ovl_stack_put(oe->__lowerstack, oe->__numlower);
> +               kfree(oe->__lowerstack);
> +       } else {
> +               dput(oe->__lowerpath.dentry);
> +       }
>  }
>  
>  #define OVL_D_REVALIDATE (DCACHE_OP_REVALIDATE |
> DCACHE_OP_WEAK_REVALIDATE)
> @@ -306,10 +326,12 @@ struct dentry *ovl_i_dentry_upper(struct inode
> *inode)
>  
>  void ovl_i_path_real(struct inode *inode, struct path *path)
>  {
> +       struct ovl_path *lowerstack = ovl_lowerstack(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 = lowerstack->dentry;
> +               path->mnt = lowerstack->layer->mnt;
>         } else {
>                 path->mnt = ovl_upper_mnt(OVL_FS(inode->i_sb));
>         }
> @@ -324,7 +346,7 @@ 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 dentry *lowerdentry = ovl_lowerstack(OVL_I_E(inode))-
> >dentry;
>  
>         return lowerdentry ? d_inode(lowerdentry) : NULL;
>  }

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a globe-trotting shark-wrestling vagrant on a search for his
missing 
sister. She's a mistrustful extravagent advertising executive with an
MBA 
from Harvard. They fight crime! 

Re: [PATCH 7/7] ovl: replace lowerdata inode reference with lowerdata redirect

[Alexander Larsson]

On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> Now that we have the entire lower stack in ovl_inode, we do not
> need to hold another reference to the lowerdata inode.
> 
> Instead, use the vacant ovl_inode space as a place holder for
> lowerdata
> redirect path from the metacopy to lowerdata, which is going to be
> used
> later on for lazy lowerdata lookup.
> 
> Use accessors to get the lowerdata path and dentry.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Alexander Larsson <alexl@redhat.com>

> ---
>  fs/overlayfs/inode.c     |  4 ++--
>  fs/overlayfs/namei.c     |  7 +++++--
>  fs/overlayfs/overlayfs.h |  3 ++-
>  fs/overlayfs/ovl_entry.h | 16 +++++++++++++++-
>  fs/overlayfs/super.c     |  4 ++--
>  fs/overlayfs/util.c      | 26 ++++++++++++++++----------
>  6 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 9f29fc3e9fa5..35d51a6dced7 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -1006,8 +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;
> -       if (oip->lowerdata)
> -               oi->lowerdata = igrab(d_inode(oip->lowerdata));
> +       oi->lowerdata_redirect = oip->lowerdata_redirect;
>  
>         realinode = ovl_inode_real(inode);
>         ovl_copyattr(inode);
> @@ -1368,6 +1367,7 @@ struct inode *ovl_get_inode(struct super_block
> *sb,
>                         dput(upperdentry);
>                         ovl_destroy_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 cdcb2ac5d95c..b629261324f1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1105,10 +1105,13 @@ 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,
>                 };
>  
> +               /* 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 32532342e56a..011b7b466f70 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -409,6 +409,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);
> @@ -660,7 +661,7 @@ struct ovl_inode_params {
>         struct ovl_entry *oe;
>         bool index;
>         char *redirect;
> -       struct dentry *lowerdata;
> +       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 5d95e937f555..221f0cbe748e 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -125,6 +125,20 @@ static inline struct ovl_path
> *ovl_lowerstack(struct ovl_entry *oe)
>         return oe && oe->__numlower > 1 ? oe->__lowerstack : &oe-
> >__lowerpath;
>  }
>  
> +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 */
> +               const char *lowerdata_redirect; /* regular file */
>         };
>         const char *redirect;
>         u64 version;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index e01a76de787c..6e4231799b86 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -172,7 +172,7 @@ static struct inode *ovl_alloc_inode(struct
> super_block *sb)
>         oi->flags = 0;
>         oi->__upperdentry = NULL;
>         ovl_init_entry(&oi->oe, NULL, 0);
> -       oi->lowerdata = NULL;
> +       oi->lowerdata_redirect = NULL;
>         mutex_init(&oi->lock);
>  
>         return &oi->vfs_inode;
> @@ -196,7 +196,7 @@ static void ovl_destroy_inode(struct inode
> *inode)
>         if (S_ISDIR(inode->i_mode))
>                 ovl_dir_cache_free(inode);
>         else
> -               iput(oi->lowerdata);
> +               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 540819ac9b9c..fe2e5a8b216b 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -245,11 +245,12 @@ 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);
> +       struct dentry *lowerdata_dentry = ovl_lowerdata_dentry(oe);
>  
> -       if (ovl_numlower(oe)) {
> -               path->mnt = lowerstack[ovl_numlower(oe) - 1].layer-
> >mnt;
> -               path->dentry = lowerstack[ovl_numlower(oe) -
> 1].dentry;
> +       if (lowerdata_dentry) {
> +               path->dentry = lowerdata_dentry;
> +               path->mnt = lowerdata->layer->mnt;
>         } else {
>                 *path = (struct path) { };
>         }
> @@ -308,10 +309,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)
> @@ -359,10 +357,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 */
> @@ -377,9 +377,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)

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a shy one-eyed hairdresser with a robot buddy named Sparky. She's
a 
mentally unstable communist bodyguard with the soul of a mighty
warrior. 
They fight crime! 

Re: [PATCH 6/7] ovl: deduplicate lowerpath and lowerstack[0]

[Miklos Szeredi]

On Sat, 8 Apr 2023 at 18:43, Amir Goldstein <amir73il@gmail.com> wrote:
>
> For the common case of single lower layer, embed ovl_entry with a
> single lower path in ovl_inode, so no stack allocation is needed.

This makes ovl_inode grow by 8 bytes, right?  That's a win only in the
numlower = 1 case, in the other cases it's a net loss, so it might not
be worth it even without the added complexity.

Thanks,
Miklos

Re: [PATCH 7/7] ovl: replace lowerdata inode reference with lowerdata redirect

[Miklos Szeredi]

On Sat, 8 Apr 2023 at 18:43, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Now that we have the entire lower stack in ovl_inode, we do not
> need to hold another reference to the lowerdata inode.
>
> Instead, use the vacant ovl_inode space as a place holder for lowerdata
> redirect path from the metacopy to lowerdata, which is going to be used
> later on for lazy lowerdata lookup.

Seems like this patch is combining two independent changes into one.
Could this be spit into

  - remove lowerdata
  - add lowerdata_redirect

?



> +               /* Store lowerdata redirect for lazy lookup */
> +               if (ctr > 1 && !d.is_dir && !stack[ctr - 1].dentry) {

 So lazy lookup will be signaled with a NULL dentry?  This should be
spelled out in the patch header.

Thanks,
Miklos

Re: [PATCH 1/7] ovl: update of dentry revalidate flags after copy up

[Amir Goldstein]

On Fri, Apr 14, 2023 at 10:21 AM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>
>
>
> On 2023/4/9 00:42, Amir Goldstein wrote:
> > 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")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.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;
>
> Although I'm not sure if it could cause some lazy awareness due to dcache
> RCU-walk, but maybe that is fine since such window is small?
>

Good question.
I am not sure.
The alternative would be to the set revalidate flags on non-upper dentry
if the upper fs is remote.

Thanks,
Amir.

Re: [PATCH 5/7] ovl: move ovl_entry into ovl_inode

[Amir Goldstein]

On Tue, Apr 18, 2023 at 10:55 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> > 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.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
>
> Reviewed-by: Alexander Larsson <alexl@redhat.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..d4caf57c8e17 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)
> > +               goto nomem;
>
> You goto nomem here, but that will dput(dentry), and dentry is not
> initialized yet. I think you should just return an error directly here.
>

Good catch!
Thanks for the review.

Amir.

Re: [PATCH 1/7] ovl: update of dentry revalidate flags after copy up

[Gao Xiang]

On 2023/4/27 13:48, Amir Goldstein wrote:
> On Fri, Apr 14, 2023 at 10:21 AM Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
>>
>>
>>
>> On 2023/4/9 00:42, Amir Goldstein wrote:
>>> 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")
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>
>> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.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;
>>
>> Although I'm not sure if it could cause some lazy awareness due to dcache
>> RCU-walk, but maybe that is fine since such window is small?
>>
> 
> Good question.
> I am not sure.
> The alternative would be to the set revalidate flags on non-upper dentry
> if the upper fs is remote.

Yeah, I think that may be safer because I'm not sure

(DCACHE_OP_REVALIDATE, DCACHE_OP_WEAK_REVALIDATE)

could be changed dynamicly as safe after d_obtain_alias()..

Thanks,
Gao Xiang

> 
> Thanks,
> Amir.

Re: [PATCH 6/7] ovl: deduplicate lowerpath and lowerstack[0]

[Amir Goldstein]

On Wed, Apr 26, 2023 at 3:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 8 Apr 2023 at 18:43, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > For the common case of single lower layer, embed ovl_entry with a
> > single lower path in ovl_inode, so no stack allocation is needed.
>
> This makes ovl_inode grow by 8 bytes, right?

I can get rid of those extra bytes by stashing __numlower
inside the union with the LSB set to indicate an external stack.

> That's a win only in the numlower = 1 case,

That's a *very* common case (i.e. non-dir without lowerdata inode)

> in the other cases it's a net loss, so it might not
> be worth it even without the added complexity.
>

I guess this is an optimization that should be justified with a benchmark.
I'll drop it for now.

Thanks,
Amir.

Re: [PATCH 6/7] ovl: deduplicate lowerpath and lowerstack[0]

[Amir Goldstein]

On Tue, Apr 18, 2023 at 11:10 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> > For the common case of single lower layer, embed ovl_entry with a
> > single lower path in ovl_inode, so no stack allocation is needed.
> >
> > For directory with more than single lower layer and for regular file
> > with lowerdata, the lower stack is stored in an external allocation.
> >
> > Use accessor ovl_lowerstack() to get the embedded or external stack.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Reviewed-by: Alexander Larsson <alexl@redhat.com>
>
> > ---
> >  fs/overlayfs/dir.c       |  2 ++
> >  fs/overlayfs/export.c    | 18 +++++----------
> >  fs/overlayfs/inode.c     | 12 ++++------
> >  fs/overlayfs/namei.c     | 15 +++++--------
> >  fs/overlayfs/overlayfs.h | 10 +++++----
> >  fs/overlayfs/ovl_entry.h | 14 +++++++-----
> >  fs/overlayfs/super.c     | 41 +++++++++++++---------------------
> >  fs/overlayfs/util.c      | 48 +++++++++++++++++++++++++++++---------
> > --
> >  8 files changed, 81 insertions(+), 79 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 92bdcedfaaec..aa0465c61064 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -262,9 +262,11 @@ static int ovl_set_opaque(struct dentry *dentry,
> > struct dentry *upperdentry)
> >  static int ovl_instantiate(struct dentry *dentry, struct inode
> > *inode,
> >                            struct dentry *newdentry, bool hardlink)
> >  {
> > +       struct ovl_entry oe = {};
> >         struct ovl_inode_params oip = {
> >                 .upperdentry = newdentry,
> >                 .newinode = inode,
> > +               .oe = &oe,
> >         };
> >
> >         ovl_dir_modified(dentry->d_parent, false);
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index d4caf57c8e17..9951c504fb8d 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -287,30 +287,22 @@ static struct dentry *ovl_obtain_alias(struct
> > super_block *sb,
> >         struct dentry *upper = upper_alias ?: index;
> >         struct dentry *dentry;
> >         struct inode *inode = NULL;
> > -       struct ovl_entry *oe;
> > +       struct ovl_entry oe;
> >         struct ovl_inode_params oip = {
> > -               .lowerpath = lowerpath,
> > +               .oe = &oe,
> >                 .index = index,
> > -               .numlower = !!lower
> >         };
> >
> >         /* We get overlay directory dentries with ovl_lookup_real()
> > */
> >         if (d_is_dir(upper ?: lower))
> >                 return ERR_PTR(-EIO);
> >
> > -       oe = ovl_alloc_entry(!!lower);
> > -       if (!oe)
> > -               goto nomem;
> > -
>
> Ah, I see that the goto nomem goes away here, so I guess ignore my
> comment on previous patch.
>
>

No need to ignore, we do not leave mid series bugs.
It is bad for bisection.

Thanks,
Amir.

Re: [PATCH 7/7] ovl: replace lowerdata inode reference with lowerdata redirect

[Amir Goldstein]

On Wed, Apr 26, 2023 at 3:39 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 8 Apr 2023 at 18:43, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Now that we have the entire lower stack in ovl_inode, we do not
> > need to hold another reference to the lowerdata inode.
> >
> > Instead, use the vacant ovl_inode space as a place holder for lowerdata
> > redirect path from the metacopy to lowerdata, which is going to be used
> > later on for lazy lowerdata lookup.
>
> Seems like this patch is combining two independent changes into one.
> Could this be spit into
>
>   - remove lowerdata
>   - add lowerdata_redirect
>
> ?

Yeh, ok.
I posted it like that for review because I thought it would be
easier to review.
But really remove lowerdata belongs to this prep series
and add lowerdata_redirect belongs to the next one so
I wont mix them

>
>
>
> > +               /* Store lowerdata redirect for lazy lookup */
> > +               if (ctr > 1 && !d.is_dir && !stack[ctr - 1].dentry) {
>
>  So lazy lookup will be signaled with a NULL dentry?  This should be
> spelled out in the patch header.
>

This also doesnt belong to this series.

Thanks,
Amir.