[PATCH 0/5] Overlayfs lazy lookup of lowerdata

[PATCH 0/5] Overlayfs lazy lookup of lowerdata

[Amir Goldstein]

Miklos,

Here are the patches discussed earlier at [1].
They are based on the already posted cleanup series [2].

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

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

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAOQ4uxich227fP7bGSCNqx-JX5h36O-MLwqPoy0r33tuH=z2cA@mail.gmail.com/
[2] https://lore.kernel.org/linux-unionfs/20230408164302.1392694-1-amir73il@gmail.com/
[3] https://github.com/amir73il/linux/commits/ovl-lazy-lowerdata
[4] https://github.com/amir73il/xfstests/commits/ovl-lazy-lowerdata


Amir Goldstein (5):
  ovl: remove unneeded goto instructions
  ovl: introduce data-only lower layers
  ovl: implement lookup in data-only layers
  ovl: prepare for lazy lookup of lowerdata inode
  ovl: implement lazy lookup of lowerdata in data-only layers

 Documentation/filesystems/overlayfs.rst |  32 +++++++
 fs/overlayfs/copy_up.c                  |   9 ++
 fs/overlayfs/export.c                   |   2 +-
 fs/overlayfs/file.c                     |  21 +++-
 fs/overlayfs/inode.c                    |  18 +++-
 fs/overlayfs/namei.c                    | 121 +++++++++++++++++++++++-
 fs/overlayfs/overlayfs.h                |   2 +
 fs/overlayfs/ovl_entry.h                |  11 ++-
 fs/overlayfs/super.c                    |  68 +++++++++----
 fs/overlayfs/util.c                     |  33 ++++++-
 10 files changed, 286 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH 1/5] ovl: remove unneeded goto instructions

[Amir Goldstein]

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

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 6e4231799b86..7742aef3f3b3 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1586,10 +1586,9 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	int err;
 	unsigned int i;
 
-	err = -ENOMEM;
 	ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
 	if (ofs->fs == NULL)
-		goto out;
+		return -ENOMEM;
 
 	/* idx/fsid 0 are reserved for upper fs even with lower only overlay */
 	ofs->numfs++;
@@ -1603,7 +1602,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	err = get_anon_bdev(&ofs->fs[0].pseudo_dev);
 	if (err) {
 		pr_err("failed to get anonymous bdev for upper fs\n");
-		goto out;
+		return err;
 	}
 
 	if (ovl_upper_mnt(ofs)) {
@@ -1616,9 +1615,9 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		struct inode *trap;
 		int fsid;
 
-		err = fsid = ovl_get_fsid(ofs, &stack[i]);
-		if (err < 0)
-			goto out;
+		fsid = ovl_get_fsid(ofs, &stack[i]);
+		if (fsid < 0)
+			return fsid;
 
 		/*
 		 * Check if lower root conflicts with this overlay layers before
@@ -1629,13 +1628,13 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		 */
 		err = ovl_setup_trap(sb, stack[i].dentry, &trap, "lowerdir");
 		if (err)
-			goto out;
+			return err;
 
 		if (ovl_is_inuse(stack[i].dentry)) {
 			err = ovl_report_in_use(ofs, "lowerdir");
 			if (err) {
 				iput(trap);
-				goto out;
+				return err;
 			}
 		}
 
@@ -1644,7 +1643,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		if (IS_ERR(mnt)) {
 			pr_err("failed to clone lowerpath\n");
 			iput(trap);
-			goto out;
+			return err;
 		}
 
 		/*
@@ -1694,9 +1693,7 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 			ofs->xino_mode);
 	}
 
-	err = 0;
-out:
-	return err;
+	return 0;
 }
 
 static int ovl_get_lowerstack(struct super_block *sb, struct ovl_entry *oe,
-- 
2.34.1

[PATCH 2/5] ovl: introduce data-only lower layers

[Amir Goldstein]

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

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

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

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

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst | 32 +++++++++++++++++
 fs/overlayfs/namei.c                    |  4 +--
 fs/overlayfs/ovl_entry.h                |  9 +++++
 fs/overlayfs/super.c                    | 46 +++++++++++++++++++++----
 4 files changed, 82 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 4c76fda07645..c8e04a4f0e21 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -371,6 +371,38 @@ conflict with metacopy=on, and will result in an error.
 [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
 given.
 
+
+Data-only lower layers
+----------------------
+
+With "metacopy" feature enabled, an overlayfs regular file may be a composition
+of information from up to three different layers:
+
+ 1) metadata from a file in the upper layer
+
+ 2) st_ino and st_dev object identifier from a file in a lower layer
+
+ 3) data from a file in another lower layer (further below)
+
+The "lower data" file can be on any lower layer, except from the top most
+lower layer.
+
+Below the top most lower layer, any number of lower most layers may be defined
+as "data-only" lower layers, using the double collon ("::") separator.
+
+For example:
+
+  mount -t overlay overlay -olowerdir=/lower1::/lower2:/lower3 /merged
+
+The paths of files in the "data-only" lower layers are not visible in the
+merged overlayfs directories and the metadata and st_ino/st_dev of files
+in the "data-only" lower layers are not visible in overlayfs inodes.
+
+Only the data of the files in the "data-only" lower layers may be visible
+when a "metacopy" file in one of the lower layers above it, has a "redirect"
+to the absolute path of the "lower data" file in the "data-only" lower layer.
+
+
 Sharing and copying layers
 --------------------------
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index b629261324f1..ff82155b4f7e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 	struct dentry *origin = NULL;
 	int i;
 
-	for (i = 1; i < ofs->numlayer; i++) {
+	for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
 		/*
 		 * If lower fs uuid is not unique among lower fs we cannot match
 		 * fh->uuid to layer.
@@ -907,7 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	if (!d.stop && ovl_numlower(poe)) {
 		err = -ENOMEM;
-		stack = ovl_stack_alloc(ofs->numlayer - 1);
+		stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
 		if (!stack)
 			goto out_put_upper;
 	}
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 221f0cbe748e..25fabb3175cf 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -62,6 +62,8 @@ struct ovl_fs {
 	unsigned int numlayer;
 	/* Number of unique fs among layers including upper fs */
 	unsigned int numfs;
+	/* Number of data-only lower layers */
+	unsigned int numdatalayer;
 	const struct ovl_layer *layers;
 	struct ovl_sb *fs;
 	/* workbasedir is the path at workdir= mount option */
@@ -95,6 +97,13 @@ struct ovl_fs {
 	errseq_t errseq;
 };
 
+
+/* Number of lower layers, not including data-only layers */
+static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
+{
+	return ofs->numlayer - ofs->numdatalayer - 1;
+}
+
 static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
 {
 	return ofs->layers[0].mnt;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7742aef3f3b3..3484f39a8f27 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1579,6 +1579,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 	return ofs->numfs++;
 }
 
+/*
+ * The fsid after the last lower fsid is used for the data layers.
+ * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
+ */
+static int ovl_get_data_fsid(struct ovl_fs *ofs)
+{
+	return ofs->numfs;
+}
+
+
 static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 			  struct path *stack, unsigned int numlower,
 			  struct ovl_layer *layers)
@@ -1586,11 +1596,14 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 	int err;
 	unsigned int i;
 
-	ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb), GFP_KERNEL);
+	ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb), GFP_KERNEL);
 	if (ofs->fs == NULL)
 		return -ENOMEM;
 
-	/* idx/fsid 0 are reserved for upper fs even with lower only overlay */
+	/*
+	 * idx/fsid 0 are reserved for upper fs even with lower only overlay
+	 * and the last fsid is reserved for "null fs" of the data layers.
+	 */
 	ofs->numfs++;
 
 	/*
@@ -1615,7 +1628,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		struct inode *trap;
 		int fsid;
 
-		fsid = ovl_get_fsid(ofs, &stack[i]);
+		if (i < numlower - ofs->numdatalayer)
+			fsid = ovl_get_fsid(ofs, &stack[i]);
+		else
+			fsid = ovl_get_data_fsid(ofs);
 		if (fsid < 0)
 			return fsid;
 
@@ -1703,6 +1719,7 @@ static int ovl_get_lowerstack(struct super_block *sb, struct ovl_entry *oe,
 	int err;
 	struct path *stack = NULL;
 	struct ovl_path *lowerstack;
+	unsigned int numlowerdata = 0;
 	unsigned int i;
 
 	if (!ofs->config.upperdir && numlower == 1) {
@@ -1714,13 +1731,27 @@ static int ovl_get_lowerstack(struct super_block *sb, struct ovl_entry *oe,
 	if (!stack)
 		return -ENOMEM;
 
-	err = -EINVAL;
-	for (i = 0; i < numlower; i++) {
+	for (i = 0; i < numlower;) {
 		err = ovl_lower_dir(lower, &stack[i], ofs, &sb->s_stack_depth);
 		if (err)
 			goto out_err;
 
 		lower = strchr(lower, '\0') + 1;
+
+		i++;
+		err = -EINVAL;
+		/* :: seperator indicates the start of lower data layers */
+		if (!*lower && i < numlower && !numlowerdata) {
+			if (!ofs->config.metacopy) {
+				pr_err("lower data-only dirs require metacopy support.\n");
+				goto out_err;
+			}
+			lower++;
+			numlower--;
+			ofs->numdatalayer = numlowerdata = numlower - i;
+			pr_info("using the lowest %d of %d lowerdirs as data layers\n",
+				numlowerdata, numlower);
+		}
 	}
 
 	err = -EINVAL;
@@ -1734,12 +1765,13 @@ static int ovl_get_lowerstack(struct super_block *sb, struct ovl_entry *oe,
 	if (err)
 		goto out_err;
 
-	err = ovl_init_entry(oe, NULL, numlower);
+	/* Data-only layers are not merged in root directory */
+	err = ovl_init_entry(oe, NULL, numlower - numlowerdata);
 	if (err)
 		goto out_err;
 
 	lowerstack = ovl_lowerstack(oe);
-	for (i = 0; i < numlower; i++) {
+	for (i = 0; i < numlower - numlowerdata; i++) {
 		lowerstack[i].dentry = dget(stack[i].dentry);
 		lowerstack[i].layer = &ofs->layers[i+1];
 	}
-- 
2.34.1

[PATCH 3/5] ovl: implement lookup in data-only layers

[Amir Goldstein]

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

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

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

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

[PATCH 4/5] ovl: prepare for lazy lookup of lowerdata inode

[Amir Goldstein]

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

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

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

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

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 9951c504fb8d..2498fa8311e3 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -343,7 +343,7 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
 	if (!idx)
 		return ovl_dentry_upper(dentry);
 
-	for (i = 0; i < ovl_numlower(oe); i++) {
+	for (i = 0; i < ovl_numlower(oe) && lowerstack[i].layer; i++) {
 		if (lowerstack[i].layer->idx == idx)
 			return lowerstack[i].dentry;
 	}
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 7c04f033aadd..951683a66ff6 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -115,6 +115,9 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 		ovl_path_real(dentry, &realpath);
 	else
 		ovl_path_realdata(dentry, &realpath);
+	/* TODO: lazy lookup of lowerdata */
+	if (!realpath.dentry)
+		return -EIO;
 
 	/* Has it been copied up since we'd opened it? */
 	if (unlikely(file_inode(real->file) != d_inode(realpath.dentry))) {
@@ -158,6 +161,10 @@ static int ovl_open(struct inode *inode, struct file *file)
 	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	ovl_path_realdata(dentry, &realpath);
+	/* TODO: lazy lookup of lowerdata */
+	if (!realpath.dentry)
+		return -EIO;
+
 	realfile = ovl_open_realfile(file, &realpath);
 	if (IS_ERR(realfile))
 		return PTR_ERR(realfile);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 35d51a6dced7..c29cbd9db64a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -240,15 +240,22 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 			/*
 			 * If lower is not same as lowerdata or if there was
 			 * no origin on upper, we can end up here.
+			 * With lazy lowerdata lookup, guess lowerdata blocks
+			 * from size to avoid lowerdata lookup on stat(2).
 			 */
 			struct kstat lowerdatastat;
 			u32 lowermask = STATX_BLOCKS;
 
 			ovl_path_lowerdata(dentry, &realpath);
-			err = vfs_getattr(&realpath, &lowerdatastat,
-					  lowermask, flags);
-			if (err)
-				goto out;
+			if (realpath.dentry) {
+				err = vfs_getattr(&realpath, &lowerdatastat,
+						  lowermask, flags);
+				if (err)
+					goto out;
+			} else {
+				lowerdatastat.blocks =
+					round_up(stat->size, stat->blksize) >> 9;
+			}
 			stat->blocks = lowerdatastat.blocks;
 		}
 	}
@@ -710,6 +717,9 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	struct inode *realinode = ovl_inode_realdata(inode);
 	const struct cred *old_cred;
 
+	if (!realinode)
+		return -EIO;
+
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3484f39a8f27..ef78abc21998 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -103,6 +103,9 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 {
 	int ret = 1;
 
+	if (!d)
+		return 1;
+
 	if (weak) {
 		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
 			ret =  d->d_op->d_weak_revalidate(d, flags);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index fe2e5a8b216b..284b5ba4fcf6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -179,7 +179,7 @@ void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
 
 	if (upperdentry)
 		flags |= upperdentry->d_flags;
-	for (i = 0; i < ovl_numlower(oe); i++)
+	for (i = 0; i < ovl_numlower(oe) && lowerstack[i].dentry; i++)
 		flags |= lowerstack[i].dentry->d_flags;
 
 	spin_lock(&dentry->d_lock);
-- 
2.34.1

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

[Amir Goldstein]

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

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

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  9 +++++++
 fs/overlayfs/file.c      | 18 ++++++++++---
 fs/overlayfs/namei.c     | 56 +++++++++++++++++++++++++++++++++++-----
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  2 +-
 fs/overlayfs/util.c      | 31 +++++++++++++++++++++-
 6 files changed, 105 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7bf101e756c8..eb266fb68730 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1074,6 +1074,15 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	if (WARN_ON(disconnected && d_is_dir(dentry)))
 		return -EIO;
 
+	/*
+	 * We may not need lowerdata if we are only doing metacopy up, but it is
+	 * not very important to optimize this case, so do lazy lowerdata lookup
+	 * before any copy up, so we can do it before taking ovl_inode_lock().
+	 */
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
 	old_cred = ovl_override_creds(dentry->d_sb);
 	while (!err) {
 		struct dentry *next;
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 951683a66ff6..39737c2aaa84 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -107,15 +107,21 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 {
 	struct dentry *dentry = file_dentry(file);
 	struct path realpath;
+	int err;
 
 	real->flags = 0;
 	real->file = file->private_data;
 
-	if (allow_meta)
+	if (allow_meta) {
 		ovl_path_real(dentry, &realpath);
-	else
+	} else {
+		/* lazy lookup of lowerdata */
+		err = ovl_maybe_lookup_lowerdata(dentry);
+		if (err)
+			return err;
+
 		ovl_path_realdata(dentry, &realpath);
-	/* TODO: lazy lookup of lowerdata */
+	}
 	if (!realpath.dentry)
 		return -EIO;
 
@@ -153,6 +159,11 @@ static int ovl_open(struct inode *inode, struct file *file)
 	struct path realpath;
 	int err;
 
+	/* lazy lookup of lowerdata */
+	err = ovl_maybe_lookup_lowerdata(dentry);
+	if (err)
+		return err;
+
 	err = ovl_maybe_copy_up(dentry, file->f_flags);
 	if (err)
 		return err;
@@ -161,7 +172,6 @@ static int ovl_open(struct inode *inode, struct file *file)
 	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
 	ovl_path_realdata(dentry, &realpath);
-	/* TODO: lazy lookup of lowerdata */
 	if (!realpath.dentry)
 		return -EIO;
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 82e103e2308b..ba2b156162ca 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -889,6 +889,52 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	return err;
 }
 
+/* Lazy lookup of lowerdata */
+int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
+{
+	struct inode *inode = d_inode(dentry);
+	const char *redirect = ovl_lowerdata_redirect(inode);
+	struct ovl_path datapath = {};
+	const struct cred *old_cred;
+	int err;
+
+	if (!redirect || ovl_dentry_lowerdata(dentry))
+		return 0;
+
+	if (redirect[0] != '/')
+		return -EIO;
+
+	err = ovl_inode_lock_interruptible(inode);
+	if (err)
+		return err;
+
+	err = 0;
+	/* Someone got here before us? */
+	if (ovl_dentry_lowerdata(dentry))
+		goto out;
+
+	old_cred = ovl_override_creds(dentry->d_sb);
+	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
+	revert_creds(old_cred);
+	if (err)
+		goto out_err;
+
+	err = ovl_dentry_set_lowerdata(dentry, &datapath);
+	if (err)
+		goto out_err;
+
+out:
+	ovl_inode_unlock(inode);
+	dput(datapath.dentry);
+
+	return err;
+
+out_err:
+	pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2, err=%i)\n",
+			    dentry, err);
+	goto out;
+}
+
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
@@ -1074,14 +1120,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	/* Lookup absolute redirect from lower metacopy in data-only layers */
+	/* Defer lookup of lowerdata in data-only layers to first access */
 	if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
-		err = ovl_lookup_data_layers(dentry, d.redirect,
-					     &stack[ctr]);
-		if (!err) {
-			d.metacopy = false;
-			ctr++;
-		}
+		d.metacopy = false;
+		ctr++;
 	}
 
 	/*
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 011b7b466f70..4e327665c316 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -400,6 +400,7 @@ enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
+int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path *datapath);
 const struct ovl_layer *ovl_i_layer_lower(struct inode *inode);
 const struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
@@ -562,6 +563,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
 struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 				struct dentry *origin, bool verify);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
+int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 25fabb3175cf..a7b1006c5321 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -145,7 +145,7 @@ static inline struct dentry *ovl_lowerdata_dentry(struct ovl_entry *oe)
 {
 	struct ovl_path *lowerdata = ovl_lowerdata(oe);
 
-	return lowerdata ? lowerdata->dentry : NULL;
+	return lowerdata ? READ_ONCE(lowerdata->dentry) : NULL;
 }
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 284b5ba4fcf6..9a042768013e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -250,7 +250,13 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
 
 	if (lowerdata_dentry) {
 		path->dentry = lowerdata_dentry;
-		path->mnt = lowerdata->layer->mnt;
+		/*
+		 * Pairs with smp_wmb() in ovl_dentry_set_lowerdata().
+		 * Make sure that if lowerdata->dentry is visible, then
+		 * datapath->layer is visible as well.
+		 */
+		smp_rmb();
+		path->mnt = READ_ONCE(lowerdata->layer)->mnt;
 	} else {
 		*path = (struct path) { };
 	}
@@ -312,6 +318,29 @@ struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
 	return ovl_lowerdata_dentry(OVL_E(dentry));
 }
 
+int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path *datapath)
+{
+	struct ovl_entry *oe = OVL_E(dentry);
+	struct ovl_path *lowerdata = ovl_lowerdata(oe);
+	struct dentry *datadentry = datapath->dentry;
+
+	if (WARN_ON_ONCE(ovl_numlower(oe) <= 1))
+		return -EIO;
+
+	WRITE_ONCE(lowerdata->layer, datapath->layer);
+	/*
+	 * Pairs with smp_rmb() in ovl_path_lowerdata().
+	 * Make sure that if lowerdata->dentry is visible, then
+	 * lowerdata->layer is visible as well.
+	 */
+	smp_wmb();
+	WRITE_ONCE(lowerdata->dentry, dget(datadentry));
+
+	ovl_dentry_update_reval(dentry, datadentry);
+
+	return 0;
+}
+
 struct dentry *ovl_dentry_real(struct dentry *dentry)
 {
 	return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);
-- 
2.34.1

Re: [PATCH 1/5] ovl: remove unneeded goto instructions

[Alexander Larsson]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> There is nothing in the out goto target of ovl_get_layers().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

LGTM

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

> ---
>  fs/overlayfs/super.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 6e4231799b86..7742aef3f3b3 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1586,10 +1586,9 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>         int err;
>         unsigned int i;
>  
> -       err = -ENOMEM;
>         ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb),
> GFP_KERNEL);
>         if (ofs->fs == NULL)
> -               goto out;
> +               return -ENOMEM;
>  
>         /* idx/fsid 0 are reserved for upper fs even with lower only
> overlay */
>         ofs->numfs++;
> @@ -1603,7 +1602,7 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>         err = get_anon_bdev(&ofs->fs[0].pseudo_dev);
>         if (err) {
>                 pr_err("failed to get anonymous bdev for upper
> fs\n");
> -               goto out;
> +               return err;
>         }
>  
>         if (ovl_upper_mnt(ofs)) {
> @@ -1616,9 +1615,9 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>                 struct inode *trap;
>                 int fsid;
>  
> -               err = fsid = ovl_get_fsid(ofs, &stack[i]);
> -               if (err < 0)
> -                       goto out;
> +               fsid = ovl_get_fsid(ofs, &stack[i]);
> +               if (fsid < 0)
> +                       return fsid;
>  
>                 /*
>                  * Check if lower root conflicts with this overlay
> layers before
> @@ -1629,13 +1628,13 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>                  */
>                 err = ovl_setup_trap(sb, stack[i].dentry, &trap,
> "lowerdir");
>                 if (err)
> -                       goto out;
> +                       return err;
>  
>                 if (ovl_is_inuse(stack[i].dentry)) {
>                         err = ovl_report_in_use(ofs, "lowerdir");
>                         if (err) {
>                                 iput(trap);
> -                               goto out;
> +                               return err;
>                         }
>                 }
>  
> @@ -1644,7 +1643,7 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>                 if (IS_ERR(mnt)) {
>                         pr_err("failed to clone lowerpath\n");
>                         iput(trap);
> -                       goto out;
> +                       return err;
>                 }
>  
>                 /*
> @@ -1694,9 +1693,7 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>                         ofs->xino_mode);
>         }
>  
> -       err = 0;
> -out:
> -       return err;
> +       return 0;
>  }
>  
>  static int ovl_get_lowerstack(struct super_block *sb, struct
> ovl_entry *oe,

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a maverick shark-wrestling romance novelist in a wheelchair. She's
a 
manipulative nymphomaniac Hell's Angel with a birthmark shaped like 
Liberty's torch. They fight crime! 

Re: [PATCH 2/5] ovl: introduce data-only lower layers

[Alexander Larsson]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> Introduce the format lowerdir=lower1:lower2::lowerdata1:lowerdata2
> where the lower layers after the :: separator are not merged into the
> overlayfs merge dirs.
> 
> The files in those layers are only meant to be accessible via
> absolute
> redirect from metacopy files in lower layers.  Following changes will
> implement lookup in the data layers.
> 
> This feature was requested for composefs ostree use case, where the
> lower data layer should only be accessiable via absolute redirects
> from metacopy inodes.
> 
> The lower data layers are not required to a have a unique uuid or any
> uuid at all, because they are never used to compose the overlayfs
> inode
> st_ino/st_dev.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

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

> ---
>  Documentation/filesystems/overlayfs.rst | 32 +++++++++++++++++
>  fs/overlayfs/namei.c                    |  4 +--
>  fs/overlayfs/ovl_entry.h                |  9 +++++
>  fs/overlayfs/super.c                    | 46 +++++++++++++++++++++--
> --
>  4 files changed, 82 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.rst
> b/Documentation/filesystems/overlayfs.rst
> index 4c76fda07645..c8e04a4f0e21 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -371,6 +371,38 @@ conflict with metacopy=on, and will result in an
> error.
>  [*] redirect_dir=follow only conflicts with metacopy=on if
> upperdir=... is
>  given.
>  
> +
> +Data-only lower layers
> +----------------------
> +
> +With "metacopy" feature enabled, an overlayfs regular file may be a
> composition
> +of information from up to three different layers:
> +
> + 1) metadata from a file in the upper layer
> +
> + 2) st_ino and st_dev object identifier from a file in a lower layer
> +
> + 3) data from a file in another lower layer (further below)
> +
> +The "lower data" file can be on any lower layer, except from the top
> most
> +lower layer.
> +
> +Below the top most lower layer, any number of lower most layers may
> be defined
> +as "data-only" lower layers, using the double collon ("::")
> separator.

"colon", not "collon"

> +
> +For example:
> +
> +  mount -t overlay overlay -olowerdir=/lower1::/lower2:/lower3
> /merged
> +
> +The paths of files in the "data-only" lower layers are not visible
> in the
> +merged overlayfs directories and the metadata and st_ino/st_dev of
> files
> +in the "data-only" lower layers are not visible in overlayfs inodes.
> +
> +Only the data of the files in the "data-only" lower layers may be
> visible
> +when a "metacopy" file in one of the lower layers above it, has a
> "redirect"
> +to the absolute path of the "lower data" file in the "data-only"
> lower layer.
> +
> +
>  Sharing and copying layers
>  --------------------------
>  
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index b629261324f1..ff82155b4f7e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs,
> struct ovl_fh *fh, bool connected,
>         struct dentry *origin = NULL;
>         int i;
>  
> -       for (i = 1; i < ofs->numlayer; i++) {
> +       for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
>                 /*
>                  * If lower fs uuid is not unique among lower fs we
> cannot match
>                  * fh->uuid to layer.
> @@ -907,7 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>  
>         if (!d.stop && ovl_numlower(poe)) {
>                 err = -ENOMEM;
> -               stack = ovl_stack_alloc(ofs->numlayer - 1);
> +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
>                 if (!stack)
>                         goto out_put_upper;
>         }

Again, surely ovl_numlower(poe) is a better size here?

> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 221f0cbe748e..25fabb3175cf 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -62,6 +62,8 @@ struct ovl_fs {
>         unsigned int numlayer;
>         /* Number of unique fs among layers including upper fs */
>         unsigned int numfs;
> +       /* Number of data-only lower layers */
> +       unsigned int numdatalayer;
>         const struct ovl_layer *layers;
>         struct ovl_sb *fs;
>         /* workbasedir is the path at workdir= mount option */
> @@ -95,6 +97,13 @@ struct ovl_fs {
>         errseq_t errseq;
>  };
>  
> +
> +/* Number of lower layers, not including data-only layers */
> +static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
> +{
> +       return ofs->numlayer - ofs->numdatalayer - 1;
> +}
> +
>  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
>  {
>         return ofs->layers[0].mnt;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 7742aef3f3b3..3484f39a8f27 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1579,6 +1579,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs,
> const struct path *path)
>         return ofs->numfs++;
>  }
>  
> +/*
> + * The fsid after the last lower fsid is used for the data layers.
> + * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
> + */
> +static int ovl_get_data_fsid(struct ovl_fs *ofs)
> +{
> +       return ofs->numfs;
> +}
> +
> +
>  static int ovl_get_layers(struct super_block *sb, struct ovl_fs
> *ofs,
>                           struct path *stack, unsigned int numlower,
>                           struct ovl_layer *layers)
> @@ -1586,11 +1596,14 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>         int err;
>         unsigned int i;
>  
> -       ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb),
> GFP_KERNEL);
> +       ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb),
> GFP_KERNEL);
>         if (ofs->fs == NULL)
>                 return -ENOMEM;
>  
> -       /* idx/fsid 0 are reserved for upper fs even with lower only
> overlay */
> +       /*
> +        * idx/fsid 0 are reserved for upper fs even with lower only
> overlay
> +        * and the last fsid is reserved for "null fs" of the data
> layers.
> +        */
>         ofs->numfs++;
>  
>         /*
> @@ -1615,7 +1628,10 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>                 struct inode *trap;
>                 int fsid;
>  
> -               fsid = ovl_get_fsid(ofs, &stack[i]);
> +               if (i < numlower - ofs->numdatalayer)
> +                       fsid = ovl_get_fsid(ofs, &stack[i]);
> +               else
> +                       fsid = ovl_get_data_fsid(ofs);
>                 if (fsid < 0)
>                         return fsid;
>  
> @@ -1703,6 +1719,7 @@ static int ovl_get_lowerstack(struct
> super_block *sb, struct ovl_entry *oe,
>         int err;
>         struct path *stack = NULL;
>         struct ovl_path *lowerstack;
> +       unsigned int numlowerdata = 0;
>         unsigned int i;
>  
>         if (!ofs->config.upperdir && numlower == 1) {
> @@ -1714,13 +1731,27 @@ static int ovl_get_lowerstack(struct
> super_block *sb, struct ovl_entry *oe,
>         if (!stack)
>                 return -ENOMEM;
>  
> -       err = -EINVAL;
> -       for (i = 0; i < numlower; i++) {
> +       for (i = 0; i < numlower;) {
>                 err = ovl_lower_dir(lower, &stack[i], ofs, &sb-
> >s_stack_depth);
>                 if (err)
>                         goto out_err;
>  
>                 lower = strchr(lower, '\0') + 1;
> +
> +               i++;
> +               err = -EINVAL;
> +               /* :: seperator indicates the start of lower data
> layers */
> +               if (!*lower && i < numlower && !numlowerdata) {
> +                       if (!ofs->config.metacopy) {
> +                               pr_err("lower data-only dirs require
> metacopy support.\n");
> +                               goto out_err;
> +                       }
> +                       lower++;
> +                       numlower--;
> +                       ofs->numdatalayer = numlowerdata = numlower -
> i;
> +                       pr_info("using the lowest %d of %d lowerdirs
> as data layers\n",
> +                               numlowerdata, numlower);
> +               }
>         }

This will handle a "::" at the end of the string as an error. Maybe it
would be better to treat it as "zero lower data layera", to make code
that generates mount options more regular? Not a huge issue though.

>         err = -EINVAL;
> @@ -1734,12 +1765,13 @@ static int ovl_get_lowerstack(struct
> super_block *sb, struct ovl_entry *oe,
>         if (err)
>                 goto out_err;
>  
> -       err = ovl_init_entry(oe, NULL, numlower);
> +       /* Data-only layers are not merged in root directory */
> +       err = ovl_init_entry(oe, NULL, numlower - numlowerdata);
>         if (err)
>                 goto out_err;
>  
>         lowerstack = ovl_lowerstack(oe);
> -       for (i = 0; i < numlower; i++) {
> +       for (i = 0; i < numlower - numlowerdata; i++) {
>                 lowerstack[i].dentry = dget(stack[i].dentry);
>                 lowerstack[i].layer = &ofs->layers[i+1];
>         }

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a one-legged native American master criminal on the edge. She's a 
supernatural paranoid wrestler with someone else's memories. They fight
crime! 

Re: [PATCH 3/5] ovl: implement lookup in data-only layers

[Alexander Larsson]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> Lookup in data-only layers only for a lower metacopy with an absolute
> redirect xattr.
> 
> The metacopy xattr is not checked on files found in the data-only
> layers
> and redirect xattr are not followed in the data-only layers.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

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

> ---
>  fs/overlayfs/namei.c | 77
> ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index ff82155b4f7e..82e103e2308b 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -14,6 +14,8 @@
>  #include <linux/exportfs.h>
>  #include "overlayfs.h"
>  
> +#include "../internal.h"       /* for vfs_path_lookup */
> +
>  struct ovl_lookup_data {
>         struct super_block *sb;
>         struct vfsmount *mnt;
> @@ -24,6 +26,8 @@ struct ovl_lookup_data {
>         bool last;
>         char *redirect;
>         bool metacopy;
> +       /* Referring to last redirect xattr */
> +       bool absolute_redirect;
>  };
>  
>  static int ovl_check_redirect(const struct path *path, struct
> ovl_lookup_data *d,
> @@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path
> *path, struct ovl_lookup_data *d
>         char *buf;
>         struct ovl_fs *ofs = OVL_FS(d->sb);
>  
> +       d->absolute_redirect = false;
>         buf = ovl_get_redirect_xattr(ofs, path, prelen +
> strlen(post));
>         if (IS_ERR_OR_NULL(buf))
>                 return PTR_ERR(buf);
>  
>         if (buf[0] == '/') {
> +               d->absolute_redirect = true;
>                 /*
>                  * One of the ancestor path elements in an absolute
> path
>                  * lookup in ovl_lookup_layer() could have been
> opaque and
> @@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base,
> struct ovl_lookup_data *d,
>         return 0;
>  }
>  
> +static int ovl_lookup_data_layer(struct dentry *dentry, const char
> *redirect,
> +                                const struct ovl_layer *layer,
> +                                struct path *datapath)
> +{
> +       int err;
> +
> +       err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt,
> redirect,
> +                       LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> LOOKUP_NO_XDEV,
> +                       datapath);
> +       pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d,
> err=%i)\n",
> +                dentry, redirect, layer->idx, err);
> +
> +       if (err)
> +               return err;
> +
> +       err = -EREMOTE;
> +       if (ovl_dentry_weird(datapath->dentry))
> +               goto out_path_put;
> +
> +       err = -ENOENT;
> +       /* Only regular file is acceptable as lower data */
> +       if (!d_is_reg(datapath->dentry))
> +               goto out_path_put;
> +
> +       return 0;
> +
> +out_path_put:
> +       path_put(datapath);
> +
> +       return err;
> +}
> +
> +/* Lookup in data-only layers by absolute redirect to layer root */
> +static int ovl_lookup_data_layers(struct dentry *dentry, const char
> *redirect,
> +                                 struct ovl_path *lowerdata)
> +{
> +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> +       const struct ovl_layer *layer;
> +       struct path datapath;
> +       int err = -ENOENT;
> +       int i;
> +
> +       layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
> +       for (i = 0; i < ofs->numdatalayer; i++, layer++) {
> +               err = ovl_lookup_data_layer(dentry, redirect, layer,
> &datapath);
> +               if (!err) {
> +                       mntput(datapath.mnt);
> +                       lowerdata->dentry = datapath.dentry;
> +                       lowerdata->layer = layer;
> +                       return 0;
> +               }
> +       }
> +
> +       return err;
> +}
>  
>  int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool
> connected,
>                         struct dentry *upperdentry, struct ovl_path
> **stackp)
> @@ -907,7 +968,9 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>  
>         if (!d.stop && ovl_numlower(poe)) {
>                 err = -ENOMEM;
> -               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> +               /* May need to reserve space in lowerstack for
> lowerdata */
> +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs) +
> +                                       (!d.is_dir && !!ofs-
> >numdatalayer));

I think this runs into issues if ovl_numlower(poe) is zero, but we 
have a redirect into the lowerdata, due to the if (ovl_numlower(poe))
check above. We won't be allocating the lowerdata stack space in this
case.

>                 if (!stack)
>                         goto out_put_upper;
>         }
> @@ -917,7 +980,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>  
>                 if (!ofs->config.redirect_follow)
>                         d.last = i == ovl_numlower(poe) - 1;
> -               else
> +               else if (d.is_dir || !ofs->numdatalayer)
>                         d.last = lower.layer->idx ==
> ovl_numlower(roe);
>  
>                 d.mnt = lower.layer->mnt;
> @@ -1011,6 +1074,16 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>                 }
>         }
>  
> +       /* Lookup absolute redirect from lower metacopy in data-only
> layers */
> +       if (d.metacopy && ctr && ofs->numdatalayer &&
> d.absolute_redirect) {
> +               err = ovl_lookup_data_layers(dentry, d.redirect,
> +                                            &stack[ctr]);
> +               if (!err) {
> +                       d.metacopy = false;
> +                       ctr++;
> +               }
> +       }

This code runs even if ofs->config.redirect_follow is false. I think
this is probably correct, but maybe it should be mentioned in the docs?

> +
>         /*
>          * For regular non-metacopy upper dentries, there is no lower
>          * path based lookup, hence ctr will be zero. If a dentry is
> found

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a war-weary chivalrous boxer living undercover at Ringling Bros. 
Circus. She's a violent nymphomaniac mercenary fleeing from a Satanic 
cult. They fight crime! 

Re: [PATCH 4/5] ovl: prepare for lazy lookup of lowerdata inode

[Alexander Larsson]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> Make the code handle the case of numlower > 1 and missing lowerdata
> dentry gracefully.
> 
> Missing lowerdata dentry is an indication for lazy lookup of
> lowerdata
> and in that case the lowerdata_redirect path is stored in ovl_inode.
> 
> Following commits will defer lookup and perform the lazy lookup on
> acccess.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

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

> ---
>  fs/overlayfs/export.c |  2 +-
>  fs/overlayfs/file.c   |  7 +++++++
>  fs/overlayfs/inode.c  | 18 ++++++++++++++----
>  fs/overlayfs/super.c  |  3 +++
>  fs/overlayfs/util.c   |  2 +-
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 9951c504fb8d..2498fa8311e3 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -343,7 +343,7 @@ static struct dentry *ovl_dentry_real_at(struct
> dentry *dentry, int idx)
>         if (!idx)
>                 return ovl_dentry_upper(dentry);
>  
> -       for (i = 0; i < ovl_numlower(oe); i++) {
> +       for (i = 0; i < ovl_numlower(oe) && lowerstack[i].layer; i++)
> {
>                 if (lowerstack[i].layer->idx == idx)
>                         return lowerstack[i].dentry;
>         }
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 7c04f033aadd..951683a66ff6 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -115,6 +115,9 @@ static int ovl_real_fdget_meta(const struct file
> *file, struct fd *real,
>                 ovl_path_real(dentry, &realpath);
>         else
>                 ovl_path_realdata(dentry, &realpath);
> +       /* TODO: lazy lookup of lowerdata */
> +       if (!realpath.dentry)
> +               return -EIO;
>  
>         /* Has it been copied up since we'd opened it? */
>         if (unlikely(file_inode(real->file) !=
> d_inode(realpath.dentry))) {
> @@ -158,6 +161,10 @@ static int ovl_open(struct inode *inode, struct
> file *file)
>         file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>  
>         ovl_path_realdata(dentry, &realpath);
> +       /* TODO: lazy lookup of lowerdata */
> +       if (!realpath.dentry)
> +               return -EIO;
> +
>         realfile = ovl_open_realfile(file, &realpath);
>         if (IS_ERR(realfile))
>                 return PTR_ERR(realfile);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 35d51a6dced7..c29cbd9db64a 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -240,15 +240,22 @@ int ovl_getattr(struct mnt_idmap *idmap, const
> struct path *path,
>                         /*
>                          * If lower is not same as lowerdata or if
> there was
>                          * no origin on upper, we can end up here.
> +                        * With lazy lowerdata lookup, guess
> lowerdata blocks
> +                        * from size to avoid lowerdata lookup on
> stat(2).
>                          */
>                         struct kstat lowerdatastat;
>                         u32 lowermask = STATX_BLOCKS;
>  
>                         ovl_path_lowerdata(dentry, &realpath);
> -                       err = vfs_getattr(&realpath, &lowerdatastat,
> -                                         lowermask, flags);
> -                       if (err)
> -                               goto out;
> +                       if (realpath.dentry) {
> +                               err = vfs_getattr(&realpath,
> &lowerdatastat,
> +                                                 lowermask, flags);
> +                               if (err)
> +                                       goto out;
> +                       } else {
> +                               lowerdatastat.blocks =
> +                                       round_up(stat->size, stat-
> >blksize) >> 9;
> +                       }
>                         stat->blocks = lowerdatastat.blocks;
>                 }
>         }
> @@ -710,6 +717,9 @@ static int ovl_fiemap(struct inode *inode, struct
> fiemap_extent_info *fieinfo,
>         struct inode *realinode = ovl_inode_realdata(inode);
>         const struct cred *old_cred;
>  
> +       if (!realinode)
> +               return -EIO;
> +
>
>         if (!realinode->i_op->fiemap)
>                 return -EOPNOTSUPP;
>  
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 3484f39a8f27..ef78abc21998 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -103,6 +103,9 @@ static int ovl_revalidate_real(struct dentry *d,
> unsigned int flags, bool weak)
>  {
>         int ret = 1;
>  
> +       if (!d)
> +               return 1;
> +
>         if (weak) {
>                 if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
>                         ret =  d->d_op->d_weak_revalidate(d, flags);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index fe2e5a8b216b..284b5ba4fcf6 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -179,7 +179,7 @@ void ovl_dentry_init_flags(struct dentry *dentry,
> struct dentry *upperdentry,
>  
>         if (upperdentry)
>                 flags |= upperdentry->d_flags;
> -       for (i = 0; i < ovl_numlower(oe); i++)
> +       for (i = 0; i < ovl_numlower(oe) && lowerstack[i].dentry;
> i++)
>                 flags |= lowerstack[i].dentry->d_flags;
>  
>         spin_lock(&dentry->d_lock);

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a time-tossed Amish vagrant who knows the secret of the alien 
invasion. She's a high-kicking gypsy barmaid from a family of eight
older 
brothers. They fight crime! 

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

[Alexander Larsson]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> Defer lookup of lowerdata in the data-only layers to first data
> access
> or before copy up.
> 
> We perform lowerdata lookup before copy up even if copy up is
> metadata
> only copy up.  We can further optimize this lookup later if needed.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

LGTM 

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

> ---
>  fs/overlayfs/copy_up.c   |  9 +++++++
>  fs/overlayfs/file.c      | 18 ++++++++++---
>  fs/overlayfs/namei.c     | 56 +++++++++++++++++++++++++++++++++++---
> --
>  fs/overlayfs/overlayfs.h |  2 ++
>  fs/overlayfs/ovl_entry.h |  2 +-
>  fs/overlayfs/util.c      | 31 +++++++++++++++++++++-
>  6 files changed, 105 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 7bf101e756c8..eb266fb68730 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -1074,6 +1074,15 @@ static int ovl_copy_up_flags(struct dentry
> *dentry, int flags)
>         if (WARN_ON(disconnected && d_is_dir(dentry)))
>                 return -EIO;
>  
> +       /*
> +        * We may not need lowerdata if we are only doing metacopy
> up, but it is
> +        * not very important to optimize this case, so do lazy
> lowerdata lookup
> +        * before any copy up, so we can do it before taking
> ovl_inode_lock().
> +        */
> +       err = ovl_maybe_lookup_lowerdata(dentry);
> +       if (err)
> +               return err;
> +
>         old_cred = ovl_override_creds(dentry->d_sb);
>         while (!err) {
>                 struct dentry *next;
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 951683a66ff6..39737c2aaa84 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -107,15 +107,21 @@ static int ovl_real_fdget_meta(const struct
> file *file, struct fd *real,
>  {
>         struct dentry *dentry = file_dentry(file);
>         struct path realpath;
> +       int err;
>  
>         real->flags = 0;
>         real->file = file->private_data;
>  
> -       if (allow_meta)
> +       if (allow_meta) {
>                 ovl_path_real(dentry, &realpath);
> -       else
> +       } else {
> +               /* lazy lookup of lowerdata */
> +               err = ovl_maybe_lookup_lowerdata(dentry);
> +               if (err)
> +                       return err;
> +
>                 ovl_path_realdata(dentry, &realpath);
> -       /* TODO: lazy lookup of lowerdata */
> +       }
>         if (!realpath.dentry)
>                 return -EIO;
>  
> @@ -153,6 +159,11 @@ static int ovl_open(struct inode *inode, struct
> file *file)
>         struct path realpath;
>         int err;
>  
> +       /* lazy lookup of lowerdata */
> +       err = ovl_maybe_lookup_lowerdata(dentry);
> +       if (err)
> +               return err;
> +
>         err = ovl_maybe_copy_up(dentry, file->f_flags);
>         if (err)
>                 return err;
> @@ -161,7 +172,6 @@ static int ovl_open(struct inode *inode, struct
> file *file)
>         file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>  
>         ovl_path_realdata(dentry, &realpath);
> -       /* TODO: lazy lookup of lowerdata */
>         if (!realpath.dentry)
>                 return -EIO;
>  
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 82e103e2308b..ba2b156162ca 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -889,6 +889,52 @@ static int ovl_fix_origin(struct ovl_fs *ofs,
> struct dentry *dentry,
>         return err;
>  }
>  
> +/* Lazy lookup of lowerdata */
> +int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> +{
> +       struct inode *inode = d_inode(dentry);
> +       const char *redirect = ovl_lowerdata_redirect(inode);
> +       struct ovl_path datapath = {};
> +       const struct cred *old_cred;
> +       int err;
> +
> +       if (!redirect || ovl_dentry_lowerdata(dentry))
> +               return 0;
> +
> +       if (redirect[0] != '/')
> +               return -EIO;
> +
> +       err = ovl_inode_lock_interruptible(inode);
> +       if (err)
> +               return err;
> +
> +       err = 0;
> +       /* Someone got here before us? */
> +       if (ovl_dentry_lowerdata(dentry))
> +               goto out;
> +
> +       old_cred = ovl_override_creds(dentry->d_sb);
> +       err = ovl_lookup_data_layers(dentry, redirect, &datapath);
> +       revert_creds(old_cred);
> +       if (err)
> +               goto out_err;
> +
> +       err = ovl_dentry_set_lowerdata(dentry, &datapath);
> +       if (err)
> +               goto out_err;
> +
> +out:
> +       ovl_inode_unlock(inode);
> +       dput(datapath.dentry);
> +
> +       return err;
> +
> +out_err:
> +       pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2,
> err=%i)\n",
> +                           dentry, err);
> +       goto out;
> +}
> +
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags)
>  {
> @@ -1074,14 +1120,10 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>                 }
>         }
>  
> -       /* Lookup absolute redirect from lower metacopy in data-only
> layers */
> +       /* Defer lookup of lowerdata in data-only layers to first
> access */
>         if (d.metacopy && ctr && ofs->numdatalayer &&
> d.absolute_redirect) {
> -               err = ovl_lookup_data_layers(dentry, d.redirect,
> -                                            &stack[ctr]);
> -               if (!err) {
> -                       d.metacopy = false;
> -                       ctr++;
> -               }
> +               d.metacopy = false;
> +               ctr++;
>         }
>  
>         /*
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 011b7b466f70..4e327665c316 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -400,6 +400,7 @@ enum ovl_path_type ovl_path_realdata(struct
> dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>  struct dentry *ovl_dentry_lower(struct dentry *dentry);
>  struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
> +int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path
> *datapath);
>  const struct ovl_layer *ovl_i_layer_lower(struct inode *inode);
>  const struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
>  struct dentry *ovl_dentry_real(struct dentry *dentry);
> @@ -562,6 +563,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs
> *ofs, struct ovl_fh *fh);
>  struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry
> *upper,
>                                 struct dentry *origin, bool verify);
>  int ovl_path_next(int idx, struct dentry *dentry, struct path
> *path);
> +int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags);
>  bool ovl_lower_positive(struct dentry *dentry);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 25fabb3175cf..a7b1006c5321 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -145,7 +145,7 @@ static inline struct dentry
> *ovl_lowerdata_dentry(struct ovl_entry *oe)
>  {
>         struct ovl_path *lowerdata = ovl_lowerdata(oe);
>  
> -       return lowerdata ? lowerdata->dentry : NULL;
> +       return lowerdata ? READ_ONCE(lowerdata->dentry) : NULL;
>  }
>  
>  /* private information held for every overlayfs dentry */
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 284b5ba4fcf6..9a042768013e 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -250,7 +250,13 @@ void ovl_path_lowerdata(struct dentry *dentry,
> struct path *path)
>  
>         if (lowerdata_dentry) {
>                 path->dentry = lowerdata_dentry;
> -               path->mnt = lowerdata->layer->mnt;
> +               /*
> +                * Pairs with smp_wmb() in
> ovl_dentry_set_lowerdata().
> +                * Make sure that if lowerdata->dentry is visible,
> then
> +                * datapath->layer is visible as well.
> +                */
> +               smp_rmb();
> +               path->mnt = READ_ONCE(lowerdata->layer)->mnt;
>         } else {
>                 *path = (struct path) { };
>         }
> @@ -312,6 +318,29 @@ struct dentry *ovl_dentry_lowerdata(struct
> dentry *dentry)
>         return ovl_lowerdata_dentry(OVL_E(dentry));
>  }
>  
> +int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path
> *datapath)
> +{
> +       struct ovl_entry *oe = OVL_E(dentry);
> +       struct ovl_path *lowerdata = ovl_lowerdata(oe);
> +       struct dentry *datadentry = datapath->dentry;
> +
> +       if (WARN_ON_ONCE(ovl_numlower(oe) <= 1))
> +               return -EIO;
> +
> +       WRITE_ONCE(lowerdata->layer, datapath->layer);
> +       /*
> +        * Pairs with smp_rmb() in ovl_path_lowerdata().
> +        * Make sure that if lowerdata->dentry is visible, then
> +        * lowerdata->layer is visible as well.
> +        */
> +       smp_wmb();
> +       WRITE_ONCE(lowerdata->dentry, dget(datadentry));
> +
> +       ovl_dentry_update_reval(dentry, datadentry);
> +
> +       return 0;
> +}
> +
>  struct dentry *ovl_dentry_real(struct dentry *dentry)
>  {
>         return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an unconventional arachnophobic ex-con fleeing from a secret 
government programme. She's a strong-willed motormouth politician from 
out of town. They fight crime! 

Re: [PATCH 2/5] ovl: introduce data-only lower layers

[Amir Goldstein]

On Tue, Apr 18, 2023 at 3:02 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> > Introduce the format lowerdir=lower1:lower2::lowerdata1:lowerdata2
> > where the lower layers after the :: separator are not merged into the
> > overlayfs merge dirs.
> >
> > The files in those layers are only meant to be accessible via
> > absolute
> > redirect from metacopy files in lower layers.  Following changes will
> > implement lookup in the data layers.
> >
> > This feature was requested for composefs ostree use case, where the
> > lower data layer should only be accessiable via absolute redirects
> > from metacopy inodes.
> >
> > The lower data layers are not required to a have a unique uuid or any
> > uuid at all, because they are never used to compose the overlayfs
> > inode
> > st_ino/st_dev.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Reviewed-by: Alexander Larsson <alexl@redhat.com>
>
> > ---
> >  Documentation/filesystems/overlayfs.rst | 32 +++++++++++++++++
> >  fs/overlayfs/namei.c                    |  4 +--
> >  fs/overlayfs/ovl_entry.h                |  9 +++++
> >  fs/overlayfs/super.c                    | 46 +++++++++++++++++++++--
> > --
> >  4 files changed, 82 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst
> > b/Documentation/filesystems/overlayfs.rst
> > index 4c76fda07645..c8e04a4f0e21 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -371,6 +371,38 @@ conflict with metacopy=on, and will result in an
> > error.
> >  [*] redirect_dir=follow only conflicts with metacopy=on if
> > upperdir=... is
> >  given.
> >
> > +
> > +Data-only lower layers
> > +----------------------
> > +
> > +With "metacopy" feature enabled, an overlayfs regular file may be a
> > composition
> > +of information from up to three different layers:
> > +
> > + 1) metadata from a file in the upper layer
> > +
> > + 2) st_ino and st_dev object identifier from a file in a lower layer
> > +
> > + 3) data from a file in another lower layer (further below)
> > +
> > +The "lower data" file can be on any lower layer, except from the top
> > most
> > +lower layer.
> > +
> > +Below the top most lower layer, any number of lower most layers may
> > be defined
> > +as "data-only" lower layers, using the double collon ("::")
> > separator.
>
> "colon", not "collon"
>
> > +
> > +For example:
> > +
> > +  mount -t overlay overlay -olowerdir=/lower1::/lower2:/lower3
> > /merged
> > +
> > +The paths of files in the "data-only" lower layers are not visible
> > in the
> > +merged overlayfs directories and the metadata and st_ino/st_dev of
> > files
> > +in the "data-only" lower layers are not visible in overlayfs inodes.
> > +
> > +Only the data of the files in the "data-only" lower layers may be
> > visible
> > +when a "metacopy" file in one of the lower layers above it, has a
> > "redirect"
> > +to the absolute path of the "lower data" file in the "data-only"
> > lower layer.
> > +
> > +
> >  Sharing and copying layers
> >  --------------------------
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index b629261324f1..ff82155b4f7e 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -356,7 +356,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs,
> > struct ovl_fh *fh, bool connected,
> >         struct dentry *origin = NULL;
> >         int i;
> >
> > -       for (i = 1; i < ofs->numlayer; i++) {
> > +       for (i = 1; i <= ovl_numlowerlayer(ofs); i++) {
> >                 /*
> >                  * If lower fs uuid is not unique among lower fs we
> > cannot match
> >                  * fh->uuid to layer.
> > @@ -907,7 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > struct dentry *dentry,
> >
> >         if (!d.stop && ovl_numlower(poe)) {
> >                 err = -ENOMEM;
> > -               stack = ovl_stack_alloc(ofs->numlayer - 1);
> > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> >                 if (!stack)
> >                         goto out_put_upper;
> >         }
>
> Again, surely ovl_numlower(poe) is a better size here?

Intentional. that is changed in the following patch.
(to ovl_numlowerlayer(ofs) + 1)
As the commit message says:
"Following changes will implement lookup in the data layers."

>
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index 221f0cbe748e..25fabb3175cf 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -62,6 +62,8 @@ struct ovl_fs {
> >         unsigned int numlayer;
> >         /* Number of unique fs among layers including upper fs */
> >         unsigned int numfs;
> > +       /* Number of data-only lower layers */
> > +       unsigned int numdatalayer;
> >         const struct ovl_layer *layers;
> >         struct ovl_sb *fs;
> >         /* workbasedir is the path at workdir= mount option */
> > @@ -95,6 +97,13 @@ struct ovl_fs {
> >         errseq_t errseq;
> >  };
> >
> > +
> > +/* Number of lower layers, not including data-only layers */
> > +static inline unsigned int ovl_numlowerlayer(struct ovl_fs *ofs)
> > +{
> > +       return ofs->numlayer - ofs->numdatalayer - 1;
> > +}
> > +
> >  static inline struct vfsmount *ovl_upper_mnt(struct ovl_fs *ofs)
> >  {
> >         return ofs->layers[0].mnt;
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 7742aef3f3b3..3484f39a8f27 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1579,6 +1579,16 @@ static int ovl_get_fsid(struct ovl_fs *ofs,
> > const struct path *path)
> >         return ofs->numfs++;
> >  }
> >
> > +/*
> > + * The fsid after the last lower fsid is used for the data layers.
> > + * It is a "null fs" with a null sb, null uuid, and no pseudo dev.
> > + */
> > +static int ovl_get_data_fsid(struct ovl_fs *ofs)
> > +{
> > +       return ofs->numfs;
> > +}
> > +
> > +
> >  static int ovl_get_layers(struct super_block *sb, struct ovl_fs
> > *ofs,
> >                           struct path *stack, unsigned int numlower,
> >                           struct ovl_layer *layers)
> > @@ -1586,11 +1596,14 @@ static int ovl_get_layers(struct super_block
> > *sb, struct ovl_fs *ofs,
> >         int err;
> >         unsigned int i;
> >
> > -       ofs->fs = kcalloc(numlower + 1, sizeof(struct ovl_sb),
> > GFP_KERNEL);
> > +       ofs->fs = kcalloc(numlower + 2, sizeof(struct ovl_sb),
> > GFP_KERNEL);
> >         if (ofs->fs == NULL)
> >                 return -ENOMEM;
> >
> > -       /* idx/fsid 0 are reserved for upper fs even with lower only
> > overlay */
> > +       /*
> > +        * idx/fsid 0 are reserved for upper fs even with lower only
> > overlay
> > +        * and the last fsid is reserved for "null fs" of the data
> > layers.
> > +        */
> >         ofs->numfs++;
> >
> >         /*
> > @@ -1615,7 +1628,10 @@ static int ovl_get_layers(struct super_block
> > *sb, struct ovl_fs *ofs,
> >                 struct inode *trap;
> >                 int fsid;
> >
> > -               fsid = ovl_get_fsid(ofs, &stack[i]);
> > +               if (i < numlower - ofs->numdatalayer)
> > +                       fsid = ovl_get_fsid(ofs, &stack[i]);
> > +               else
> > +                       fsid = ovl_get_data_fsid(ofs);
> >                 if (fsid < 0)
> >                         return fsid;
> >
> > @@ -1703,6 +1719,7 @@ static int ovl_get_lowerstack(struct
> > super_block *sb, struct ovl_entry *oe,
> >         int err;
> >         struct path *stack = NULL;
> >         struct ovl_path *lowerstack;
> > +       unsigned int numlowerdata = 0;
> >         unsigned int i;
> >
> >         if (!ofs->config.upperdir && numlower == 1) {
> > @@ -1714,13 +1731,27 @@ static int ovl_get_lowerstack(struct
> > super_block *sb, struct ovl_entry *oe,
> >         if (!stack)
> >                 return -ENOMEM;
> >
> > -       err = -EINVAL;
> > -       for (i = 0; i < numlower; i++) {
> > +       for (i = 0; i < numlower;) {
> >                 err = ovl_lower_dir(lower, &stack[i], ofs, &sb-
> > >s_stack_depth);
> >                 if (err)
> >                         goto out_err;
> >
> >                 lower = strchr(lower, '\0') + 1;
> > +
> > +               i++;
> > +               err = -EINVAL;
> > +               /* :: seperator indicates the start of lower data
> > layers */
> > +               if (!*lower && i < numlower && !numlowerdata) {
> > +                       if (!ofs->config.metacopy) {
> > +                               pr_err("lower data-only dirs require
> > metacopy support.\n");
> > +                               goto out_err;
> > +                       }
> > +                       lower++;
> > +                       numlower--;
> > +                       ofs->numdatalayer = numlowerdata = numlower -
> > i;
> > +                       pr_info("using the lowest %d of %d lowerdirs
> > as data layers\n",
> > +                               numlowerdata, numlower);
> > +               }
> >         }
>
> This will handle a "::" at the end of the string as an error. Maybe it
> would be better to treat it as "zero lower data layera", to make code
> that generates mount options more regular? Not a huge issue though.
>

No reason to do that.
Code that prepares lowerdata layers should know what it is doing.

Thanks,
Amir.

Re: [PATCH 3/5] ovl: implement lookup in data-only layers

[Amir Goldstein]

On Tue, Apr 18, 2023 at 3:40 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> > Lookup in data-only layers only for a lower metacopy with an absolute
> > redirect xattr.
> >
> > The metacopy xattr is not checked on files found in the data-only
> > layers
> > and redirect xattr are not followed in the data-only layers.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Reviewed-by: Alexander Larsson <alexl@redhat.com>
>
> > ---
> >  fs/overlayfs/namei.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 75 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index ff82155b4f7e..82e103e2308b 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/exportfs.h>
> >  #include "overlayfs.h"
> >
> > +#include "../internal.h"       /* for vfs_path_lookup */
> > +
> >  struct ovl_lookup_data {
> >         struct super_block *sb;
> >         struct vfsmount *mnt;
> > @@ -24,6 +26,8 @@ struct ovl_lookup_data {
> >         bool last;
> >         char *redirect;
> >         bool metacopy;
> > +       /* Referring to last redirect xattr */
> > +       bool absolute_redirect;
> >  };
> >
> >  static int ovl_check_redirect(const struct path *path, struct
> > ovl_lookup_data *d,
> > @@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path
> > *path, struct ovl_lookup_data *d
> >         char *buf;
> >         struct ovl_fs *ofs = OVL_FS(d->sb);
> >
> > +       d->absolute_redirect = false;
> >         buf = ovl_get_redirect_xattr(ofs, path, prelen +
> > strlen(post));
> >         if (IS_ERR_OR_NULL(buf))
> >                 return PTR_ERR(buf);
> >
> >         if (buf[0] == '/') {
> > +               d->absolute_redirect = true;
> >                 /*
> >                  * One of the ancestor path elements in an absolute
> > path
> >                  * lookup in ovl_lookup_layer() could have been
> > opaque and
> > @@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base,
> > struct ovl_lookup_data *d,
> >         return 0;
> >  }
> >
> > +static int ovl_lookup_data_layer(struct dentry *dentry, const char
> > *redirect,
> > +                                const struct ovl_layer *layer,
> > +                                struct path *datapath)
> > +{
> > +       int err;
> > +
> > +       err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt,
> > redirect,
> > +                       LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> > LOOKUP_NO_XDEV,
> > +                       datapath);
> > +       pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d,
> > err=%i)\n",
> > +                dentry, redirect, layer->idx, err);
> > +
> > +       if (err)
> > +               return err;
> > +
> > +       err = -EREMOTE;
> > +       if (ovl_dentry_weird(datapath->dentry))
> > +               goto out_path_put;
> > +
> > +       err = -ENOENT;
> > +       /* Only regular file is acceptable as lower data */
> > +       if (!d_is_reg(datapath->dentry))
> > +               goto out_path_put;
> > +
> > +       return 0;
> > +
> > +out_path_put:
> > +       path_put(datapath);
> > +
> > +       return err;
> > +}
> > +
> > +/* Lookup in data-only layers by absolute redirect to layer root */
> > +static int ovl_lookup_data_layers(struct dentry *dentry, const char
> > *redirect,
> > +                                 struct ovl_path *lowerdata)
> > +{
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > +       const struct ovl_layer *layer;
> > +       struct path datapath;
> > +       int err = -ENOENT;
> > +       int i;
> > +
> > +       layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
> > +       for (i = 0; i < ofs->numdatalayer; i++, layer++) {
> > +               err = ovl_lookup_data_layer(dentry, redirect, layer,
> > &datapath);
> > +               if (!err) {
> > +                       mntput(datapath.mnt);
> > +                       lowerdata->dentry = datapath.dentry;
> > +                       lowerdata->layer = layer;
> > +                       return 0;
> > +               }
> > +       }
> > +
> > +       return err;
> > +}
> >
> >  int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool
> > connected,
> >                         struct dentry *upperdentry, struct ovl_path
> > **stackp)
> > @@ -907,7 +968,9 @@ struct dentry *ovl_lookup(struct inode *dir,
> > struct dentry *dentry,
> >
> >         if (!d.stop && ovl_numlower(poe)) {
> >                 err = -ENOMEM;
> > -               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > +               /* May need to reserve space in lowerstack for
> > lowerdata */
> > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs) +
> > +                                       (!d.is_dir && !!ofs-
> > >numdatalayer));
>
> I think this runs into issues if ovl_numlower(poe) is zero, but we
> have a redirect into the lowerdata, due to the if (ovl_numlower(poe))
> check above. We won't be allocating the lowerdata stack space in this
> case.
>

Redirect from upper directly into data-only is not valid.
As the comment below says:
/* Lookup absolute redirect from lower metacopy in data-only layers */
data-only can only be accessed by absolute redirect from
lower layer.
Is that a problem for your use case?

> >                 if (!stack)
> >                         goto out_put_upper;
> >         }
> > @@ -917,7 +980,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > struct dentry *dentry,
> >
> >                 if (!ofs->config.redirect_follow)
> >                         d.last = i == ovl_numlower(poe) - 1;
> > -               else
> > +               else if (d.is_dir || !ofs->numdatalayer)
> >                         d.last = lower.layer->idx ==
> > ovl_numlower(roe);
> >
> >                 d.mnt = lower.layer->mnt;
> > @@ -1011,6 +1074,16 @@ struct dentry *ovl_lookup(struct inode *dir,
> > struct dentry *dentry,
> >                 }
> >         }
> >
> > +       /* Lookup absolute redirect from lower metacopy in data-only
> > layers */
> > +       if (d.metacopy && ctr && ofs->numdatalayer &&
> > d.absolute_redirect) {
> > +               err = ovl_lookup_data_layers(dentry, d.redirect,
> > +                                            &stack[ctr]);
> > +               if (!err) {
> > +                       d.metacopy = false;
> > +                       ctr++;
> > +               }
> > +       }
>
> This code runs even if ofs->config.redirect_follow is false. I think
> this is probably correct, but maybe it should be mentioned in the docs?
>

ofs->numdatalayer > 0 depends on ofs->config.metacopy
which depends on ofs->config.redirect or ofs->config.redirect_follow.

Thanks,
Amir.

Re: [PATCH 3/5] ovl: implement lookup in data-only layers

[Alexander Larsson]

On Tue, Apr 18, 2023 at 3:41 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 18, 2023 at 3:40 PM Alexander Larsson <alexl@redhat.com> wrote:
> >
> > On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> > > Lookup in data-only layers only for a lower metacopy with an absolute
> > > redirect xattr.
> > >
> > > The metacopy xattr is not checked on files found in the data-only
> > > layers
> > > and redirect xattr are not followed in the data-only layers.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Reviewed-by: Alexander Larsson <alexl@redhat.com>
> >
> > > ---
> > >  fs/overlayfs/namei.c | 77
> > > ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 75 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index ff82155b4f7e..82e103e2308b 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -14,6 +14,8 @@
> > >  #include <linux/exportfs.h>
> > >  #include "overlayfs.h"
> > >
> > > +#include "../internal.h"       /* for vfs_path_lookup */
> > > +
> > >  struct ovl_lookup_data {
> > >         struct super_block *sb;
> > >         struct vfsmount *mnt;
> > > @@ -24,6 +26,8 @@ struct ovl_lookup_data {
> > >         bool last;
> > >         char *redirect;
> > >         bool metacopy;
> > > +       /* Referring to last redirect xattr */
> > > +       bool absolute_redirect;
> > >  };
> > >
> > >  static int ovl_check_redirect(const struct path *path, struct
> > > ovl_lookup_data *d,
> > > @@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path
> > > *path, struct ovl_lookup_data *d
> > >         char *buf;
> > >         struct ovl_fs *ofs = OVL_FS(d->sb);
> > >
> > > +       d->absolute_redirect = false;
> > >         buf = ovl_get_redirect_xattr(ofs, path, prelen +
> > > strlen(post));
> > >         if (IS_ERR_OR_NULL(buf))
> > >                 return PTR_ERR(buf);
> > >
> > >         if (buf[0] == '/') {
> > > +               d->absolute_redirect = true;
> > >                 /*
> > >                  * One of the ancestor path elements in an absolute
> > > path
> > >                  * lookup in ovl_lookup_layer() could have been
> > > opaque and
> > > @@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base,
> > > struct ovl_lookup_data *d,
> > >         return 0;
> > >  }
> > >
> > > +static int ovl_lookup_data_layer(struct dentry *dentry, const char
> > > *redirect,
> > > +                                const struct ovl_layer *layer,
> > > +                                struct path *datapath)
> > > +{
> > > +       int err;
> > > +
> > > +       err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt,
> > > redirect,
> > > +                       LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> > > LOOKUP_NO_XDEV,
> > > +                       datapath);
> > > +       pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d,
> > > err=%i)\n",
> > > +                dentry, redirect, layer->idx, err);
> > > +
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       err = -EREMOTE;
> > > +       if (ovl_dentry_weird(datapath->dentry))
> > > +               goto out_path_put;
> > > +
> > > +       err = -ENOENT;
> > > +       /* Only regular file is acceptable as lower data */
> > > +       if (!d_is_reg(datapath->dentry))
> > > +               goto out_path_put;
> > > +
> > > +       return 0;
> > > +
> > > +out_path_put:
> > > +       path_put(datapath);
> > > +
> > > +       return err;
> > > +}
> > > +
> > > +/* Lookup in data-only layers by absolute redirect to layer root */
> > > +static int ovl_lookup_data_layers(struct dentry *dentry, const char
> > > *redirect,
> > > +                                 struct ovl_path *lowerdata)
> > > +{
> > > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > > +       const struct ovl_layer *layer;
> > > +       struct path datapath;
> > > +       int err = -ENOENT;
> > > +       int i;
> > > +
> > > +       layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
> > > +       for (i = 0; i < ofs->numdatalayer; i++, layer++) {
> > > +               err = ovl_lookup_data_layer(dentry, redirect, layer,
> > > &datapath);
> > > +               if (!err) {
> > > +                       mntput(datapath.mnt);
> > > +                       lowerdata->dentry = datapath.dentry;
> > > +                       lowerdata->layer = layer;
> > > +                       return 0;
> > > +               }
> > > +       }
> > > +
> > > +       return err;
> > > +}
> > >
> > >  int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool
> > > connected,
> > >                         struct dentry *upperdentry, struct ovl_path
> > > **stackp)
> > > @@ -907,7 +968,9 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >
> > >         if (!d.stop && ovl_numlower(poe)) {
> > >                 err = -ENOMEM;
> > > -               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > > +               /* May need to reserve space in lowerstack for
> > > lowerdata */
> > > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs) +
> > > +                                       (!d.is_dir && !!ofs-
> > > >numdatalayer));
> >
> > I think this runs into issues if ovl_numlower(poe) is zero, but we
> > have a redirect into the lowerdata, due to the if (ovl_numlower(poe))
> > check above. We won't be allocating the lowerdata stack space in this
> > case.
> >
>
> Redirect from upper directly into data-only is not valid.

Ah.

> As the comment below says:
> /* Lookup absolute redirect from lower metacopy in data-only layers */
> data-only can only be accessed by absolute redirect from
> lower layer.
> Is that a problem for your use case?

No, just something I noticed that looked wrong.

> > >                 if (!stack)
> > >                         goto out_put_upper;
> > >         }
> > > @@ -917,7 +980,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >
> > >                 if (!ofs->config.redirect_follow)
> > >                         d.last = i == ovl_numlower(poe) - 1;
> > > -               else
> > > +               else if (d.is_dir || !ofs->numdatalayer)
> > >                         d.last = lower.layer->idx ==
> > > ovl_numlower(roe);
> > >
> > >                 d.mnt = lower.layer->mnt;
> > > @@ -1011,6 +1074,16 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > >                 }
> > >         }
> > >
> > > +       /* Lookup absolute redirect from lower metacopy in data-only
> > > layers */
> > > +       if (d.metacopy && ctr && ofs->numdatalayer &&
> > > d.absolute_redirect) {
> > > +               err = ovl_lookup_data_layers(dentry, d.redirect,
> > > +                                            &stack[ctr]);
> > > +               if (!err) {
> > > +                       d.metacopy = false;
> > > +                       ctr++;
> > > +               }
> > > +       }
> >
> > This code runs even if ofs->config.redirect_follow is false. I think
> > this is probably correct, but maybe it should be mentioned in the docs?
> >
>
> ofs->numdatalayer > 0 depends on ofs->config.metacopy
> which depends on ofs->config.redirect or ofs->config.redirect_follow.
>
> Thanks,
> Amir.
>


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

Re: [PATCH 3/5] ovl: implement lookup in data-only layers

[Amir Goldstein]

On Tue, Apr 18, 2023 at 5:00 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Tue, Apr 18, 2023 at 3:41 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Apr 18, 2023 at 3:40 PM Alexander Larsson <alexl@redhat.com> wrote:
> > >
> > > On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> > > > Lookup in data-only layers only for a lower metacopy with an absolute
> > > > redirect xattr.
> > > >
> > > > The metacopy xattr is not checked on files found in the data-only
> > > > layers
> > > > and redirect xattr are not followed in the data-only layers.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > Reviewed-by: Alexander Larsson <alexl@redhat.com>
> > >
> > > > ---
> > > >  fs/overlayfs/namei.c | 77
> > > > ++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 75 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > > index ff82155b4f7e..82e103e2308b 100644
> > > > --- a/fs/overlayfs/namei.c
> > > > +++ b/fs/overlayfs/namei.c
> > > > @@ -14,6 +14,8 @@
> > > >  #include <linux/exportfs.h>
> > > >  #include "overlayfs.h"
> > > >
> > > > +#include "../internal.h"       /* for vfs_path_lookup */
> > > > +
> > > >  struct ovl_lookup_data {
> > > >         struct super_block *sb;
> > > >         struct vfsmount *mnt;
> > > > @@ -24,6 +26,8 @@ struct ovl_lookup_data {
> > > >         bool last;
> > > >         char *redirect;
> > > >         bool metacopy;
> > > > +       /* Referring to last redirect xattr */
> > > > +       bool absolute_redirect;
> > > >  };
> > > >
> > > >  static int ovl_check_redirect(const struct path *path, struct
> > > > ovl_lookup_data *d,
> > > > @@ -33,11 +37,13 @@ static int ovl_check_redirect(const struct path
> > > > *path, struct ovl_lookup_data *d
> > > >         char *buf;
> > > >         struct ovl_fs *ofs = OVL_FS(d->sb);
> > > >
> > > > +       d->absolute_redirect = false;
> > > >         buf = ovl_get_redirect_xattr(ofs, path, prelen +
> > > > strlen(post));
> > > >         if (IS_ERR_OR_NULL(buf))
> > > >                 return PTR_ERR(buf);
> > > >
> > > >         if (buf[0] == '/') {
> > > > +               d->absolute_redirect = true;
> > > >                 /*
> > > >                  * One of the ancestor path elements in an absolute
> > > > path
> > > >                  * lookup in ovl_lookup_layer() could have been
> > > > opaque and
> > > > @@ -349,6 +355,61 @@ static int ovl_lookup_layer(struct dentry *base,
> > > > struct ovl_lookup_data *d,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int ovl_lookup_data_layer(struct dentry *dentry, const char
> > > > *redirect,
> > > > +                                const struct ovl_layer *layer,
> > > > +                                struct path *datapath)
> > > > +{
> > > > +       int err;
> > > > +
> > > > +       err = vfs_path_lookup(layer->mnt->mnt_root, layer->mnt,
> > > > redirect,
> > > > +                       LOOKUP_BENEATH | LOOKUP_NO_SYMLINKS |
> > > > LOOKUP_NO_XDEV,
> > > > +                       datapath);
> > > > +       pr_debug("lookup lowerdata (%pd2, redirect=\"%s\", layer=%d,
> > > > err=%i)\n",
> > > > +                dentry, redirect, layer->idx, err);
> > > > +
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > > +       err = -EREMOTE;
> > > > +       if (ovl_dentry_weird(datapath->dentry))
> > > > +               goto out_path_put;
> > > > +
> > > > +       err = -ENOENT;
> > > > +       /* Only regular file is acceptable as lower data */
> > > > +       if (!d_is_reg(datapath->dentry))
> > > > +               goto out_path_put;
> > > > +
> > > > +       return 0;
> > > > +
> > > > +out_path_put:
> > > > +       path_put(datapath);
> > > > +
> > > > +       return err;
> > > > +}
> > > > +
> > > > +/* Lookup in data-only layers by absolute redirect to layer root */
> > > > +static int ovl_lookup_data_layers(struct dentry *dentry, const char
> > > > *redirect,
> > > > +                                 struct ovl_path *lowerdata)
> > > > +{
> > > > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > > > +       const struct ovl_layer *layer;
> > > > +       struct path datapath;
> > > > +       int err = -ENOENT;
> > > > +       int i;
> > > > +
> > > > +       layer = &ofs->layers[ofs->numlayer - ofs->numdatalayer];
> > > > +       for (i = 0; i < ofs->numdatalayer; i++, layer++) {
> > > > +               err = ovl_lookup_data_layer(dentry, redirect, layer,
> > > > &datapath);
> > > > +               if (!err) {
> > > > +                       mntput(datapath.mnt);
> > > > +                       lowerdata->dentry = datapath.dentry;
> > > > +                       lowerdata->layer = layer;
> > > > +                       return 0;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       return err;
> > > > +}
> > > >
> > > >  int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool
> > > > connected,
> > > >                         struct dentry *upperdentry, struct ovl_path
> > > > **stackp)
> > > > @@ -907,7 +968,9 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > > struct dentry *dentry,
> > > >
> > > >         if (!d.stop && ovl_numlower(poe)) {
> > > >                 err = -ENOMEM;
> > > > -               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > > > +               /* May need to reserve space in lowerstack for
> > > > lowerdata */
> > > > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs) +
> > > > +                                       (!d.is_dir && !!ofs-
> > > > >numdatalayer));
> > >
> > > I think this runs into issues if ovl_numlower(poe) is zero, but we
> > > have a redirect into the lowerdata, due to the if (ovl_numlower(poe))
> > > check above. We won't be allocating the lowerdata stack space in this
> > > case.
> > >
> >
> > Redirect from upper directly into data-only is not valid.
>
> Ah.
>
> > As the comment below says:
> > /* Lookup absolute redirect from lower metacopy in data-only layers */
> > data-only can only be accessed by absolute redirect from
> > lower layer.
> > Is that a problem for your use case?
>
> No, just something I noticed that looked wrong.

I haven't actually explained why that limitation exists.

Apart from the fact that your use case does not need
redirect from upper to data-only, when the stack is composed
of only upper and lower (not 2 lowers) then the lower is used
for the overlay ino/index/fh.

data-only layers cannot be used for ovelray ino/index/fh because
they are data-only and ino/fh is metadata.

That's also the reason that data-only layers do not need to go through
the ovl_lower_uuid_ok() check, which allows greater flexibility
with the filesystems being used for the backing store of the data blobs.

Thanks,
Amir.

Re: [PATCH 2/5] ovl: introduce data-only lower layers

[Alexander Larsson]

On Tue, 2023-04-18 at 16:33 +0300, Amir Goldstein wrote:
> On Tue, Apr 18, 2023 at 3:02 PM Alexander Larsson <alexl@redhat.com>
> wrote:
> > 
> > 
> > >                  * fh->uuid to layer.
> > > @@ -907,7 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > > 
> > >         if (!d.stop && ovl_numlower(poe)) {
> > >                 err = -ENOMEM;
> > > -               stack = ovl_stack_alloc(ofs->numlayer - 1);
> > > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > >                 if (!stack)
> > >                         goto out_put_upper;
> > >         }
> > 
> > Again, surely ovl_numlower(poe) is a better size here?
> 
> Intentional. that is changed in the following patch.
> (to ovl_numlowerlayer(ofs) + 1)
> As the commit message says:
> "Following changes will implement lookup in the data layers."

Still, you might have 10 lower layers in the overlay mount overall, but
this particular parent may only have 1 lower layer, no? So
numlower(poe) would be smaller that numlowerlayer(ofs).

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an all-American vegetarian romance novelist who must take
medication 
to keep him sane. She's a vivacious extravagent stripper from Mars.
They 
fight crime! 

Re: [PATCH 2/5] ovl: introduce data-only lower layers

[Amir Goldstein]

On Tue, Apr 18, 2023 at 5:20 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Tue, 2023-04-18 at 16:33 +0300, Amir Goldstein wrote:
> > On Tue, Apr 18, 2023 at 3:02 PM Alexander Larsson <alexl@redhat.com>
> > wrote:
> > >
> > >
> > > >                  * fh->uuid to layer.
> > > > @@ -907,7 +907,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> > > > struct dentry *dentry,
> > > >
> > > >         if (!d.stop && ovl_numlower(poe)) {
> > > >                 err = -ENOMEM;
> > > > -               stack = ovl_stack_alloc(ofs->numlayer - 1);
> > > > +               stack = ovl_stack_alloc(ovl_numlowerlayer(ofs));
> > > >                 if (!stack)
> > > >                         goto out_put_upper;
> > > >         }
> > >
> > > Again, surely ovl_numlower(poe) is a better size here?
> >
> > Intentional. that is changed in the following patch.
> > (to ovl_numlowerlayer(ofs) + 1)
> > As the commit message says:
> > "Following changes will implement lookup in the data layers."
>
> Still, you might have 10 lower layers in the overlay mount overall, but
> this particular parent may only have 1 lower layer, no? So
> numlower(poe) would be smaller that numlowerlayer(ofs).

Ah, I understand your question.
The answer is that absolute redirect (a.k.a poe = roe) can change
numlower(poe) after the stack was allocated.

There are several "optimizations" that could be done, but they
are useless, because stack is a temporary allocation.
The permanent stack allocation is done at the end of lookup
with ovl_alloc_entry(ctr).

Which means I could have also left it ofs->numlayer - 1
this change ends up being just semantic.

Thanks,
Amir.

Re: [PATCH 4/5] ovl: prepare for lazy lookup of lowerdata inode

[Miklos Szeredi]

On Wed, 12 Apr 2023 at 15:54, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Make the code handle the case of numlower > 1 and missing lowerdata
> dentry gracefully.
>
> Missing lowerdata dentry is an indication for lazy lookup of lowerdata
> and in that case the lowerdata_redirect path is stored in ovl_inode.
>
> Following commits will defer lookup and perform the lazy lookup on
> acccess.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/export.c |  2 +-
>  fs/overlayfs/file.c   |  7 +++++++
>  fs/overlayfs/inode.c  | 18 ++++++++++++++----
>  fs/overlayfs/super.c  |  3 +++
>  fs/overlayfs/util.c   |  2 +-
>  5 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 9951c504fb8d..2498fa8311e3 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -343,7 +343,7 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
>         if (!idx)
>                 return ovl_dentry_upper(dentry);
>
> -       for (i = 0; i < ovl_numlower(oe); i++) {
> +       for (i = 0; i < ovl_numlower(oe) && lowerstack[i].layer; i++) {

Metacopy and NFS export are mutually exclusive, so this doesn't make sense.


> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 3484f39a8f27..ef78abc21998 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c

ovl_d_real() calls ovl_dentry_lowerdata().  If triggered from
file_dentry() it should be okay, since that is done on an open file
(lazy lookup already perfromed).   But it can also be called from
d_real_inode(), the only caller of which is trace_uprobe.  Is this
going to be okay?

In any case a comment is needed at least.

Thanks,
Miklos

Re: [PATCH 4/5] ovl: prepare for lazy lookup of lowerdata inode

[Amir Goldstein]

On Wed, Apr 26, 2023 at 5:57 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 12 Apr 2023 at 15:54, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Make the code handle the case of numlower > 1 and missing lowerdata
> > dentry gracefully.
> >
> > Missing lowerdata dentry is an indication for lazy lookup of lowerdata
> > and in that case the lowerdata_redirect path is stored in ovl_inode.
> >
> > Following commits will defer lookup and perform the lazy lookup on
> > acccess.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/export.c |  2 +-
> >  fs/overlayfs/file.c   |  7 +++++++
> >  fs/overlayfs/inode.c  | 18 ++++++++++++++----
> >  fs/overlayfs/super.c  |  3 +++
> >  fs/overlayfs/util.c   |  2 +-
> >  5 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 9951c504fb8d..2498fa8311e3 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -343,7 +343,7 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> >         if (!idx)
> >                 return ovl_dentry_upper(dentry);
> >
> > -       for (i = 0; i < ovl_numlower(oe); i++) {
> > +       for (i = 0; i < ovl_numlower(oe) && lowerstack[i].layer; i++) {
>
> Metacopy and NFS export are mutually exclusive, so this doesn't make sense.
>

OK.

>
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 3484f39a8f27..ef78abc21998 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
>
> ovl_d_real() calls ovl_dentry_lowerdata().  If triggered from
> file_dentry() it should be okay, since that is done on an open file
> (lazy lookup already perfromed).   But it can also be called from
> d_real_inode(), the only caller of which is trace_uprobe.  Is this
> going to be okay?
>

Not sure.
It's hard to imagine that trace_uprobe_create() is being called to place
a probe on a file at offset X without reading the content of symbols first.

I wonder if lazy lookup from within ovl_d_real(d, NULL) is acceptable?
It does look like the context of trace_uprobe() callers should be fine for lazy
lowerdata lookup.

> In any case a comment is needed at least.
>

I will leave it as is and add a comment.

Thanks,
Amir.

Re: [PATCH 4/5] ovl: prepare for lazy lookup of lowerdata inode

[Amir Goldstein]

On Thu, Apr 27, 2023 at 2:12 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Apr 26, 2023 at 5:57 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 12 Apr 2023 at 15:54, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Make the code handle the case of numlower > 1 and missing lowerdata
> > > dentry gracefully.
> > >
> > > Missing lowerdata dentry is an indication for lazy lookup of lowerdata
> > > and in that case the lowerdata_redirect path is stored in ovl_inode.
> > >
> > > Following commits will defer lookup and perform the lazy lookup on
> > > acccess.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/overlayfs/export.c |  2 +-
> > >  fs/overlayfs/file.c   |  7 +++++++
> > >  fs/overlayfs/inode.c  | 18 ++++++++++++++----
> > >  fs/overlayfs/super.c  |  3 +++
> > >  fs/overlayfs/util.c   |  2 +-
> > >  5 files changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index 9951c504fb8d..2498fa8311e3 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -343,7 +343,7 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> > >         if (!idx)
> > >                 return ovl_dentry_upper(dentry);
> > >
> > > -       for (i = 0; i < ovl_numlower(oe); i++) {
> > > +       for (i = 0; i < ovl_numlower(oe) && lowerstack[i].layer; i++) {
> >
> > Metacopy and NFS export are mutually exclusive, so this doesn't make sense.
> >
>
> OK.
>
> >
> > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > > index 3484f39a8f27..ef78abc21998 100644
> > > --- a/fs/overlayfs/super.c
> > > +++ b/fs/overlayfs/super.c
> >
> > ovl_d_real() calls ovl_dentry_lowerdata().  If triggered from
> > file_dentry() it should be okay, since that is done on an open file
> > (lazy lookup already perfromed).   But it can also be called from
> > d_real_inode(), the only caller of which is trace_uprobe.  Is this
> > going to be okay?
> >
>
> Not sure.
> It's hard to imagine that trace_uprobe_create() is being called to place
> a probe on a file at offset X without reading the content of symbols first.
>
> I wonder if lazy lookup from within ovl_d_real(d, NULL) is acceptable?
> It does look like the context of trace_uprobe() callers should be fine for lazy
> lowerdata lookup.
>
> > In any case a comment is needed at least.
> >
>
> I will leave it as is and add a comment.

On second thought, I will add best effort lazy lowerdata lookup
in addition to the warning.
d_real_inode() does not expect errors or NULL and trace_uprobe
does not check for them either.

Thanks,
Amir.