[PATCH v2 0/5] Prep patches for porting overlayfs to new mount api

[PATCH v2 0/5] Prep patches for porting overlayfs to new mount api

[Amir Goldstein]

Miklos,

Following some more cleanup patches that make Christian's new mount api
patches smaller and easier to review.

I had rebased Christain's patches over these cleanups and pushed the
result to github branch fs-overlayfs-mount_api [1].

The v1 prep patches had a bug with xino option parsing that resulted in
some tests being skipped (not failing) and I had only noticed the
skipped test after posting v1.

The v2 prep patches + new mount api patches have passed all the tests
with no new tests skipped.

In addition to running the tests with the default kernel config, I also
ran the tests with the following non-default configs (individually):

1) CONFIG_OVERLAY_FS_REDIRECT_DIR=y
2) CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW=n
3) CONFIG_OVERLAY_FS_XINO_AUTO=y


Thanks.
Amir.

Changes since v1:
- Fix xino opt name table vs. enum order
- Add cleanup patch for xino
- Add cleanup of share_whiteout state
- Add cleanup patch for ovl_get_root()

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

Amir Goldstein (5):
  ovl: negate the ofs->share_whiteout boolean
  ovl: clarify ovl_get_root() semantics
  ovl: pass ovl_fs to xino helpers
  ovl: store enum redirect_mode in config instead of a string
  ovl: factor out ovl_parse_options() helper

 Documentation/filesystems/overlayfs.rst |   9 +-
 fs/overlayfs/dir.c                      |   6 +-
 fs/overlayfs/inode.c                    |  18 +-
 fs/overlayfs/namei.c                    |   6 +-
 fs/overlayfs/overlayfs.h                |  63 ++--
 fs/overlayfs/ovl_entry.h                |   8 +-
 fs/overlayfs/readdir.c                  |  19 +-
 fs/overlayfs/super.c                    | 364 +++++++++++++-----------
 fs/overlayfs/util.c                     |   7 -
 9 files changed, 274 insertions(+), 226 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] ovl: negate the ofs->share_whiteout boolean

[Amir Goldstein]

The default common case is that whiteout sharing is enabled.
Change to storing the negated no_shared_whiteout state, so we will not
need to initialize it.

This is the first step towards removing all config and feature
initializations out of ovl_fill_super().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 4 ++--
 fs/overlayfs/ovl_entry.h | 4 ++--
 fs/overlayfs/super.c     | 3 ---
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 92bdcedfaaec..0da45727099b 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -83,7 +83,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 		ofs->whiteout = whiteout;
 	}
 
-	if (ofs->share_whiteout) {
+	if (!ofs->no_shared_whiteout) {
 		whiteout = ovl_lookup_temp(ofs, workdir);
 		if (IS_ERR(whiteout))
 			goto out;
@@ -95,7 +95,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 		if (err != -EMLINK) {
 			pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
 				ofs->whiteout->d_inode->i_nlink, err);
-			ofs->share_whiteout = false;
+			ofs->no_shared_whiteout = true;
 		}
 		dput(whiteout);
 	}
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index e5207c4bf5b8..40232b056be8 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -86,7 +86,6 @@ struct ovl_fs {
 	/* Did we take the inuse lock? */
 	bool upperdir_locked;
 	bool workdir_locked;
-	bool share_whiteout;
 	/* Traps in ovl inode cache */
 	struct inode *workbasedir_trap;
 	struct inode *workdir_trap;
@@ -95,8 +94,9 @@ struct ovl_fs {
 	int xino_mode;
 	/* For allocation of non-persistent inode numbers */
 	atomic_long_t last_ino;
-	/* Whiteout dentry cache */
+	/* Shared whiteout cache */
 	struct dentry *whiteout;
+	bool no_shared_whiteout;
 	/* r/o snapshot of upperdir sb's only taken on volatile mounts */
 	errseq_t errseq;
 };
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 14a2ebdc8126..ee9adb413d0e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1954,9 +1954,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!cred)
 		goto out_err;
 
-	/* Is there a reason anyone would want not to share whiteouts? */
-	ofs->share_whiteout = true;
-
 	ofs->config.index = ovl_index_def;
 	ofs->config.uuid = true;
 	ofs->config.nfs_export = ovl_nfs_export_def;
-- 
2.34.1

[PATCH v2 2/5] ovl: clarify ovl_get_root() semantics

[Amir Goldstein]

Change the semantics to take a reference on upperdentry instead
of transferrig the reference.

This is needed for upcoming port to new mount api.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ee9adb413d0e..280f2aa2f356 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1922,6 +1922,8 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 	ovl_set_upperdata(d_inode(root));
 	ovl_inode_init(d_inode(root), &oip, ino, fsid);
 	ovl_dentry_init_flags(root, upperdentry, oe, DCACHE_OP_WEAK_REVALIDATE);
+	/* root keeps a reference of upperdentry */
+	dget(upperdentry);
 
 	return root;
 }
@@ -2100,7 +2102,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!root_dentry)
 		goto out_free_oe;
 
-	mntput(upperpath.mnt);
+	path_put(&upperpath);
 	kfree(splitlower);
 
 	sb->s_root = root_dentry;
-- 
2.34.1

[PATCH v2 3/5] ovl: pass ovl_fs to xino helpers

[Amir Goldstein]

Internal ovl methods should use ovl_fs and not sb as much as
possible.

Use a constant_table to translate from enum xino mode to string
in preperation for new mount api option parsing.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     | 18 ++++++++++--------
 fs/overlayfs/overlayfs.h | 16 ++++++++--------
 fs/overlayfs/readdir.c   | 19 +++++++++++--------
 fs/overlayfs/super.c     | 21 ++++++++++++++-------
 4 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index bf47a0a94d1e..a63e57447be9 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -97,8 +97,9 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 
 static void ovl_map_dev_ino(struct dentry *dentry, struct kstat *stat, int fsid)
 {
-	bool samefs = ovl_same_fs(dentry->d_sb);
-	unsigned int xinobits = ovl_xino_bits(dentry->d_sb);
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
+	bool samefs = ovl_same_fs(ofs);
+	unsigned int xinobits = ovl_xino_bits(ofs);
 	unsigned int xinoshift = 64 - xinobits;
 
 	if (samefs) {
@@ -123,7 +124,7 @@ static void ovl_map_dev_ino(struct dentry *dentry, struct kstat *stat, int fsid)
 			stat->ino |= ((u64)fsid) << (xinoshift + 1);
 			stat->dev = dentry->d_sb->s_dev;
 			return;
-		} else if (ovl_xino_warn(dentry->d_sb)) {
+		} else if (ovl_xino_warn(ofs)) {
 			pr_warn_ratelimited("inode number too big (%pd2, ino=%llu, xinobits=%d)\n",
 					    dentry, stat->ino, xinobits);
 		}
@@ -149,7 +150,7 @@ static void ovl_map_dev_ino(struct dentry *dentry, struct kstat *stat, int fsid)
 		 * is unique per underlying fs, so we use the unique anonymous
 		 * bdev assigned to the underlying fs.
 		 */
-		stat->dev = OVL_FS(dentry->d_sb)->fs[fsid].pseudo_dev;
+		stat->dev = ofs->fs[fsid].pseudo_dev;
 	}
 }
 
@@ -186,7 +187,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 	 * If lower filesystem supports NFS file handles, this also guaranties
 	 * persistent st_ino across mount cycle.
 	 */
-	if (!is_dir || ovl_same_dev(dentry->d_sb)) {
+	if (!is_dir || ovl_same_dev(OVL_FS(dentry->d_sb))) {
 		if (!OVL_TYPE_UPPER(type)) {
 			fsid = ovl_layer_lower(dentry)->fsid;
 		} else if (OVL_TYPE_ORIGIN(type)) {
@@ -961,7 +962,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
 
 static void ovl_next_ino(struct inode *inode)
 {
-	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
 
 	inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
 	if (unlikely(!inode->i_ino))
@@ -970,7 +971,8 @@ static void ovl_next_ino(struct inode *inode)
 
 static void ovl_map_ino(struct inode *inode, unsigned long ino, int fsid)
 {
-	int xinobits = ovl_xino_bits(inode->i_sb);
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
+	int xinobits = ovl_xino_bits(ofs);
 	unsigned int xinoshift = 64 - xinobits;
 
 	/*
@@ -981,7 +983,7 @@ static void ovl_map_ino(struct inode *inode, unsigned long ino, int fsid)
 	 * with d_ino also causes nfsd readdirplus to fail.
 	 */
 	inode->i_ino = ino;
-	if (ovl_same_fs(inode->i_sb)) {
+	if (ovl_same_fs(ofs)) {
 		return;
 	} else if (xinobits && likely(!(ino >> xinoshift))) {
 		inode->i_ino |= (unsigned long)fsid << (xinoshift + 1);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index fcac4e2c56ab..05e9acfe1590 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -494,26 +494,26 @@ static inline bool ovl_is_impuredir(struct super_block *sb,
  * d_ino consistent with st_ino.
  * With xino=on, we do the same effort but we warn if we failed.
  */
-static inline bool ovl_xino_warn(struct super_block *sb)
+static inline bool ovl_xino_warn(struct ovl_fs *ofs)
 {
-	return OVL_FS(sb)->config.xino == OVL_XINO_ON;
+	return ofs->config.xino == OVL_XINO_ON;
 }
 
 /* All layers on same fs? */
-static inline bool ovl_same_fs(struct super_block *sb)
+static inline bool ovl_same_fs(struct ovl_fs *ofs)
 {
-	return OVL_FS(sb)->xino_mode == 0;
+	return ofs->xino_mode == 0;
 }
 
 /* All overlay inodes have same st_dev? */
-static inline bool ovl_same_dev(struct super_block *sb)
+static inline bool ovl_same_dev(struct ovl_fs *ofs)
 {
-	return OVL_FS(sb)->xino_mode >= 0;
+	return ofs->xino_mode >= 0;
 }
 
-static inline unsigned int ovl_xino_bits(struct super_block *sb)
+static inline unsigned int ovl_xino_bits(struct ovl_fs *ofs)
 {
-	return ovl_same_dev(sb) ? OVL_FS(sb)->xino_mode : 0;
+	return ovl_same_dev(ofs) ? ofs->xino_mode : 0;
 }
 
 static inline void ovl_inode_lock(struct inode *inode)
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index b6952b21a7ee..ee5c4736480f 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -118,7 +118,7 @@ static bool ovl_calc_d_ino(struct ovl_readdir_data *rdd,
 		return false;
 
 	/* Always recalc d_ino when remapping lower inode numbers */
-	if (ovl_xino_bits(rdd->dentry->d_sb))
+	if (ovl_xino_bits(OVL_FS(rdd->dentry->d_sb)))
 		return true;
 
 	/* Always recalc d_ino for parent */
@@ -460,13 +460,14 @@ static int ovl_cache_update_ino(const struct path *path, struct ovl_cache_entry
 
 {
 	struct dentry *dir = path->dentry;
+	struct ovl_fs *ofs = OVL_FS(dir->d_sb);
 	struct dentry *this = NULL;
 	enum ovl_path_type type;
 	u64 ino = p->real_ino;
-	int xinobits = ovl_xino_bits(dir->d_sb);
+	int xinobits = ovl_xino_bits(ofs);
 	int err = 0;
 
-	if (!ovl_same_dev(dir->d_sb))
+	if (!ovl_same_dev(ofs))
 		goto out;
 
 	if (p->name[0] == '.') {
@@ -515,7 +516,7 @@ static int ovl_cache_update_ino(const struct path *path, struct ovl_cache_entry
 		ino = ovl_remap_lower_ino(ino, xinobits,
 					  ovl_layer_lower(this)->fsid,
 					  p->name, p->len,
-					  ovl_xino_warn(dir->d_sb));
+					  ovl_xino_warn(ofs));
 	}
 
 out:
@@ -694,12 +695,13 @@ static int ovl_iterate_real(struct file *file, struct dir_context *ctx)
 	int err;
 	struct ovl_dir_file *od = file->private_data;
 	struct dentry *dir = file->f_path.dentry;
+	struct ovl_fs *ofs = OVL_FS(dir->d_sb);
 	const struct ovl_layer *lower_layer = ovl_layer_lower(dir);
 	struct ovl_readdir_translate rdt = {
 		.ctx.actor = ovl_fill_real,
 		.orig_ctx = ctx,
-		.xinobits = ovl_xino_bits(dir->d_sb),
-		.xinowarn = ovl_xino_warn(dir->d_sb),
+		.xinobits = ovl_xino_bits(ofs),
+		.xinowarn = ovl_xino_warn(ofs),
 	};
 
 	if (rdt.xinobits && lower_layer)
@@ -735,6 +737,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 {
 	struct ovl_dir_file *od = file->private_data;
 	struct dentry *dentry = file->f_path.dentry;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct ovl_cache_entry *p;
 	const struct cred *old_cred;
 	int err;
@@ -749,8 +752,8 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 		 * dir is impure then need to adjust d_ino for copied up
 		 * entries.
 		 */
-		if (ovl_xino_bits(dentry->d_sb) ||
-		    (ovl_same_fs(dentry->d_sb) &&
+		if (ovl_xino_bits(ofs) ||
+		    (ovl_same_fs(ofs) &&
 		     (ovl_is_impure_dir(file) ||
 		      OVL_TYPE_MERGE(ovl_path_type(dentry->d_parent))))) {
 			err = ovl_iterate_real(file, ctx);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 280f2aa2f356..5bcb26528408 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -16,6 +16,7 @@
 #include <linux/posix_acl_xattr.h>
 #include <linux/exportfs.h>
 #include <linux/file.h>
+#include <linux/fs_parser.h>
 #include "overlayfs.h"
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
@@ -334,12 +335,18 @@ static const char *ovl_redirect_mode_def(void)
 	return ovl_redirect_dir_def ? "on" : "off";
 }
 
-static const char * const ovl_xino_str[] = {
-	"off",
-	"auto",
-	"on",
+static const struct constant_table ovl_parameter_xino[] = {
+	{ "off",	OVL_XINO_OFF  },
+	{ "auto",	OVL_XINO_AUTO },
+	{ "on",		OVL_XINO_ON   },
+	{}
 };
 
+static const char *ovl_xino_mode(struct ovl_config *config)
+{
+	return ovl_parameter_xino[config->xino].name;
+}
+
 static inline int ovl_xino_def(void)
 {
 	return ovl_xino_auto_def ? OVL_XINO_AUTO : OVL_XINO_OFF;
@@ -374,8 +381,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.nfs_export != ovl_nfs_export_def)
 		seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
 						"on" : "off");
-	if (ofs->config.xino != ovl_xino_def() && !ovl_same_fs(sb))
-		seq_printf(m, ",xino=%s", ovl_xino_str[ofs->config.xino]);
+	if (ofs->config.xino != ovl_xino_def() && !ovl_same_fs(ofs))
+		seq_printf(m, ",xino=%s", ovl_xino_mode(&ofs->config));
 	if (ofs->config.metacopy != ovl_metacopy_def)
 		seq_printf(m, ",metacopy=%s",
 			   ofs->config.metacopy ? "on" : "off");
@@ -1566,7 +1573,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
 			pr_warn("%s uuid detected in lower fs '%pd2', falling back to xino=%s,index=off,nfs_export=off.\n",
 				uuid_is_null(&sb->s_uuid) ? "null" :
 							    "conflicting",
-				path->dentry, ovl_xino_str[ofs->config.xino]);
+				path->dentry, ovl_xino_mode(&ofs->config));
 		}
 	}
 
-- 
2.34.1

[PATCH v2 4/5] ovl: store enum redirect_mode in config instead of a string

[Amir Goldstein]

Do all the logic to set the mode during mount options parsing and
do not keep the option string around.

Use a constant_table to translate from enum redirect mode to string
in preperation for new mount api option parsing.

The mount option "off" is translated to either "follow" or "nofollow",
depending on the "redirect_always_follow" build/module config, so
in effect, there are only three possible redirect modes.

This results in a minor change to the string that is displayed
in show_options() - when redirect_dir is enabled by default and the user
mounts with the option "redirect_dir=off", instead of displaying the mode
"redirect_dir=off" in show_options(), the displayed mode will be either
"redirect_dir=follow" or "redirect_dir=nofollow", depending on the value
of "redirect_always_follow" build/module config.

The displayed mode reflects the effective mode, so mounting overlayfs
again with the dispalyed redirect_dir option will result with the same
effective and displayed mode.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst |   9 +-
 fs/overlayfs/dir.c                      |   2 +-
 fs/overlayfs/namei.c                    |   6 +-
 fs/overlayfs/overlayfs.h                |  39 ++++---
 fs/overlayfs/ovl_entry.h                |   4 +-
 fs/overlayfs/super.c                    | 129 +++++++++++++-----------
 fs/overlayfs/util.c                     |   7 --
 7 files changed, 107 insertions(+), 89 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 4f36b8919f7c..eb7d2c88ddec 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -231,12 +231,11 @@ Mount options:
     Redirects are enabled.
 - "redirect_dir=follow":
     Redirects are not created, but followed.
-- "redirect_dir=off":
-    Redirects are not created and only followed if "redirect_always_follow"
-    feature is enabled in the kernel/module config.
 - "redirect_dir=nofollow":
-    Redirects are not created and not followed (equivalent to "redirect_dir=off"
-    if "redirect_always_follow" feature is not enabled).
+    Redirects are not created and not followed.
+- "redirect_dir=off":
+    If "redirect_always_follow" is enabled in the kernel/module config,
+    this "off" traslates to "follow", otherwise it translates to "nofollow".
 
 When the NFS export feature is enabled, every copied up directory is
 indexed by the file handle of the lower inode and a file handle of the
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 0da45727099b..033fc0458a3d 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -952,7 +952,7 @@ static bool ovl_type_merge_or_lower(struct dentry *dentry)
 
 static bool ovl_can_move(struct dentry *dentry)
 {
-	return ovl_redirect_dir(dentry->d_sb) ||
+	return ovl_redirect_dir(OVL_FS(dentry->d_sb)) ||
 		!d_is_dir(dentry) || !ovl_type_merge_or_lower(dentry);
 }
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 292b8a948f1a..288e3c0ee0e6 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -961,7 +961,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.is_dir = false,
 		.opaque = false,
 		.stop = false,
-		.last = ofs->config.redirect_follow ? false : !ovl_numlower(poe),
+		.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
 		.redirect = NULL,
 		.metacopy = false,
 	};
@@ -1022,7 +1022,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
 		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
-		if (!ofs->config.redirect_follow)
+		if (!ovl_redirect_follow(ofs))
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
 			d.last = lower.layer->idx == ovl_numlower(roe);
@@ -1102,7 +1102,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		 * this attack vector when not necessary.
 		 */
 		err = -EPERM;
-		if (d.redirect && !ofs->config.redirect_follow) {
+		if (d.redirect && !ovl_redirect_follow(ofs)) {
 			pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
 					    dentry);
 			goto out_put;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 05e9acfe1590..80c10228bd64 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -57,6 +57,13 @@ enum ovl_entry_flag {
 	OVL_E_CONNECTED,
 };
 
+enum {
+	OVL_REDIRECT_OFF,	/* "off" mode is never used. In effect	*/
+	OVL_REDIRECT_FOLLOW,	/* ...it translates to either "follow"	*/
+	OVL_REDIRECT_NOFOLLOW,	/* ...or "nofollow".			*/
+	OVL_REDIRECT_ON,
+};
+
 enum {
 	OVL_XINO_OFF,
 	OVL_XINO_AUTO,
@@ -352,17 +359,6 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
 	return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
 }
 
-static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
-{
-	/*
-	 * To avoid regressions in existing setups with overlay lower offline
-	 * changes, we allow lower changes only if none of the new features
-	 * are used.
-	 */
-	return (!ofs->config.index && !ofs->config.metacopy &&
-		!ofs->config.redirect_dir && ofs->config.xino != OVL_XINO_ON);
-}
-
 
 /* util.c */
 int ovl_want_write(struct dentry *dentry);
@@ -421,7 +417,6 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags);
 bool ovl_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags);
 bool ovl_has_upperdata(struct inode *inode);
 void ovl_set_upperdata(struct inode *inode);
-bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
 void ovl_inode_update(struct inode *inode, struct dentry *upperdentry);
@@ -489,6 +484,16 @@ static inline bool ovl_is_impuredir(struct super_block *sb,
 	return ovl_path_check_dir_xattr(ofs, &upperpath, OVL_XATTR_IMPURE);
 }
 
+static inline bool ovl_redirect_follow(struct ovl_fs *ofs)
+{
+	return ofs->config.redirect_mode != OVL_REDIRECT_NOFOLLOW;
+}
+
+static inline bool ovl_redirect_dir(struct ovl_fs *ofs)
+{
+	return ofs->config.redirect_mode == OVL_REDIRECT_ON;
+}
+
 /*
  * With xino=auto, we do best effort to keep all inodes on same st_dev and
  * d_ino consistent with st_ino.
@@ -499,6 +504,16 @@ static inline bool ovl_xino_warn(struct ovl_fs *ofs)
 	return ofs->config.xino == OVL_XINO_ON;
 }
 
+/*
+ * To avoid regressions in existing setups with overlay lower offline changes,
+ * we allow lower changes only if none of the new features are used.
+ */
+static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
+{
+	return (!ofs->config.index && !ofs->config.metacopy &&
+		!ovl_redirect_dir(ofs) && !ovl_xino_warn(ofs));
+}
+
 /* All layers on same fs? */
 static inline bool ovl_same_fs(struct ovl_fs *ofs)
 {
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 40232b056be8..b4eb0bd5d0b6 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -10,9 +10,7 @@ struct ovl_config {
 	char *upperdir;
 	char *workdir;
 	bool default_permissions;
-	bool redirect_dir;
-	bool redirect_follow;
-	const char *redirect_mode;
+	int redirect_mode;
 	bool index;
 	bool uuid;
 	bool nfs_export;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5bcb26528408..5a84af92c91e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -244,7 +244,6 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 	kfree(ofs->config.lowerdir);
 	kfree(ofs->config.upperdir);
 	kfree(ofs->config.workdir);
-	kfree(ofs->config.redirect_mode);
 	if (ofs->creator_cred)
 		put_cred(ofs->creator_cred);
 	kfree(ofs);
@@ -330,9 +329,24 @@ static bool ovl_force_readonly(struct ovl_fs *ofs)
 	return (!ovl_upper_mnt(ofs) || !ofs->workdir);
 }
 
-static const char *ovl_redirect_mode_def(void)
+static const struct constant_table ovl_parameter_redirect_dir[] = {
+	{ "off",	OVL_REDIRECT_OFF      },
+	{ "follow",	OVL_REDIRECT_FOLLOW   },
+	{ "nofollow",	OVL_REDIRECT_NOFOLLOW },
+	{ "on",		OVL_REDIRECT_ON       },
+	{}
+};
+
+static const char *ovl_redirect_mode(struct ovl_config *config)
 {
-	return ovl_redirect_dir_def ? "on" : "off";
+	return ovl_parameter_redirect_dir[config->redirect_mode].name;
+}
+
+static int ovl_redirect_mode_def(void)
+{
+	return ovl_redirect_dir_def ? OVL_REDIRECT_ON :
+		ovl_redirect_always_follow ? OVL_REDIRECT_FOLLOW :
+					     OVL_REDIRECT_NOFOLLOW;
 }
 
 static const struct constant_table ovl_parameter_xino[] = {
@@ -372,8 +386,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	}
 	if (ofs->config.default_permissions)
 		seq_puts(m, ",default_permissions");
-	if (strcmp(ofs->config.redirect_mode, ovl_redirect_mode_def()) != 0)
-		seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
+	if (ofs->config.redirect_mode != ovl_redirect_mode_def())
+		seq_printf(m, ",redirect_dir=%s",
+			   ovl_redirect_mode(&ofs->config));
 	if (ofs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
 	if (!ofs->config.uuid)
@@ -431,7 +446,10 @@ enum {
 	OPT_UPPERDIR,
 	OPT_WORKDIR,
 	OPT_DEFAULT_PERMISSIONS,
-	OPT_REDIRECT_DIR,
+	OPT_REDIRECT_DIR_ON,
+	OPT_REDIRECT_DIR_OFF,
+	OPT_REDIRECT_DIR_FOLLOW,
+	OPT_REDIRECT_DIR_NOFOLLOW,
 	OPT_INDEX_ON,
 	OPT_INDEX_OFF,
 	OPT_UUID_ON,
@@ -453,7 +471,10 @@ static const match_table_t ovl_tokens = {
 	{OPT_UPPERDIR,			"upperdir=%s"},
 	{OPT_WORKDIR,			"workdir=%s"},
 	{OPT_DEFAULT_PERMISSIONS,	"default_permissions"},
-	{OPT_REDIRECT_DIR,		"redirect_dir=%s"},
+	{OPT_REDIRECT_DIR_ON,		"redirect_dir=on"},
+	{OPT_REDIRECT_DIR_OFF,		"redirect_dir=off"},
+	{OPT_REDIRECT_DIR_FOLLOW,	"redirect_dir=follow"},
+	{OPT_REDIRECT_DIR_NOFOLLOW,	"redirect_dir=nofollow"},
 	{OPT_INDEX_ON,			"index=on"},
 	{OPT_INDEX_OFF,			"index=off"},
 	{OPT_USERXATTR,			"userxattr"},
@@ -493,40 +514,12 @@ static char *ovl_next_opt(char **s)
 	return sbegin;
 }
 
-static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
-{
-	if (strcmp(mode, "on") == 0) {
-		config->redirect_dir = true;
-		/*
-		 * Does not make sense to have redirect creation without
-		 * redirect following.
-		 */
-		config->redirect_follow = true;
-	} else if (strcmp(mode, "follow") == 0) {
-		config->redirect_follow = true;
-	} else if (strcmp(mode, "off") == 0) {
-		if (ovl_redirect_always_follow)
-			config->redirect_follow = true;
-	} else if (strcmp(mode, "nofollow") != 0) {
-		pr_err("bad mount option \"redirect_dir=%s\"\n",
-		       mode);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int ovl_parse_opt(char *opt, struct ovl_config *config)
 {
 	char *p;
-	int err;
 	bool metacopy_opt = false, redirect_opt = false;
 	bool nfs_export_opt = false, index_opt = false;
 
-	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
-	if (!config->redirect_mode)
-		return -ENOMEM;
-
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;
 		substring_t args[MAX_OPT_ARGS];
@@ -561,11 +554,25 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->default_permissions = true;
 			break;
 
-		case OPT_REDIRECT_DIR:
-			kfree(config->redirect_mode);
-			config->redirect_mode = match_strdup(&args[0]);
-			if (!config->redirect_mode)
-				return -ENOMEM;
+		case OPT_REDIRECT_DIR_ON:
+			config->redirect_mode = OVL_REDIRECT_ON;
+			redirect_opt = true;
+			break;
+
+		case OPT_REDIRECT_DIR_OFF:
+			config->redirect_mode = ovl_redirect_always_follow ?
+						OVL_REDIRECT_FOLLOW :
+						OVL_REDIRECT_NOFOLLOW;
+			redirect_opt = true;
+			break;
+
+		case OPT_REDIRECT_DIR_FOLLOW:
+			config->redirect_mode = OVL_REDIRECT_FOLLOW;
+			redirect_opt = true;
+			break;
+
+		case OPT_REDIRECT_DIR_NOFOLLOW:
+			config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
 			redirect_opt = true;
 			break;
 
@@ -654,22 +661,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		config->ovl_volatile = false;
 	}
 
-	err = ovl_parse_redirect_mode(config, config->redirect_mode);
-	if (err)
-		return err;
-
 	/*
 	 * This is to make the logic below simpler.  It doesn't make any other
-	 * difference, since config->redirect_dir is only used for upper.
+	 * difference, since redirect_dir=on is only used for upper.
 	 */
-	if (!config->upperdir && config->redirect_follow)
-		config->redirect_dir = true;
+	if (!config->upperdir && config->redirect_mode == OVL_REDIRECT_FOLLOW)
+		config->redirect_mode = OVL_REDIRECT_ON;
 
 	/* Resolve metacopy -> redirect_dir dependency */
-	if (config->metacopy && !config->redirect_dir) {
+	if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
 		if (metacopy_opt && redirect_opt) {
 			pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
-			       config->redirect_mode);
+			       ovl_redirect_mode(config));
 			return -EINVAL;
 		}
 		if (redirect_opt) {
@@ -678,17 +681,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			 * in this conflict.
 			 */
 			pr_info("disabling metacopy due to redirect_dir=%s\n",
-				config->redirect_mode);
+				ovl_redirect_mode(config));
 			config->metacopy = false;
 		} else {
 			/* Automatically enable redirect otherwise. */
-			config->redirect_follow = config->redirect_dir = true;
+			config->redirect_mode = OVL_REDIRECT_ON;
 		}
 	}
 
 	/* Resolve nfs_export -> index dependency */
 	if (config->nfs_export && !config->index) {
-		if (!config->upperdir && config->redirect_follow) {
+		if (!config->upperdir &&
+		    config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
 			pr_info("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
 			config->nfs_export = false;
 		} else if (nfs_export_opt && index_opt) {
@@ -733,9 +737,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve userxattr -> !redirect && !metacopy dependency */
 	if (config->userxattr) {
-		if (config->redirect_follow && redirect_opt) {
+		if (redirect_opt &&
+		    config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
 			pr_err("conflicting options: userxattr,redirect_dir=%s\n",
-			       config->redirect_mode);
+			       ovl_redirect_mode(config));
 			return -EINVAL;
 		}
 		if (config->metacopy && metacopy_opt) {
@@ -748,7 +753,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		 * options must be explicitly enabled if used together with
 		 * userxattr.
 		 */
-		config->redirect_dir = config->redirect_follow = false;
+		config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
 		config->metacopy = false;
 	}
 
@@ -1332,10 +1337,17 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 	if (err) {
 		pr_warn("failed to set xattr on upper\n");
 		ofs->noxattr = true;
-		if (ofs->config.index || ofs->config.metacopy) {
-			ofs->config.index = false;
+		if (ovl_redirect_follow(ofs)) {
+			ofs->config.redirect_mode = OVL_REDIRECT_NOFOLLOW;
+			pr_warn("...falling back to redirect_dir=nofollow.\n");
+		}
+		if (ofs->config.metacopy) {
 			ofs->config.metacopy = false;
-			pr_warn("...falling back to index=off,metacopy=off.\n");
+			pr_warn("...falling back to metacopy=off.\n");
+		}
+		if (ofs->config.index) {
+			ofs->config.index = false;
+			pr_warn("...falling back to index=off.\n");
 		}
 		/*
 		 * xattr support is required for persistent st_ino.
@@ -1963,6 +1975,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!cred)
 		goto out_err;
 
+	ofs->config.redirect_mode = ovl_redirect_mode_def();
 	ofs->config.index = ovl_index_def;
 	ofs->config.uuid = true;
 	ofs->config.nfs_export = ovl_nfs_export_def;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 939e4d586ec2..7ef9e13c404a 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -506,13 +506,6 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags)
 	return !ovl_has_upperdata(d_inode(dentry));
 }
 
-bool ovl_redirect_dir(struct super_block *sb)
-{
-	struct ovl_fs *ofs = sb->s_fs_info;
-
-	return ofs->config.redirect_dir && !ofs->noxattr;
-}
-
 const char *ovl_dentry_get_redirect(struct dentry *dentry)
 {
 	return OVL_I(d_inode(dentry))->redirect;
-- 
2.34.1

[PATCH v2 5/5] ovl: factor out ovl_parse_options() helper

[Amir Goldstein]

For parsing a single mount option.

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

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 80c10228bd64..30227ccc758d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -70,6 +70,14 @@ enum {
 	OVL_XINO_ON,
 };
 
+/* The set of options that user requested explicitly via mount options */
+struct ovl_opt_set {
+	bool metacopy;
+	bool redirect;
+	bool nfs_export;
+	bool index;
+};
+
 /*
  * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
  * where:
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5a84af92c91e..dbd9151dc073 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -514,131 +514,142 @@ static char *ovl_next_opt(char **s)
 	return sbegin;
 }
 
-static int ovl_parse_opt(char *opt, struct ovl_config *config)
+static int ovl_parse_opt(char *opt, struct ovl_config *config,
+			 struct ovl_opt_set *set)
 {
-	char *p;
-	bool metacopy_opt = false, redirect_opt = false;
-	bool nfs_export_opt = false, index_opt = false;
-
-	while ((p = ovl_next_opt(&opt)) != NULL) {
-		int token;
-		substring_t args[MAX_OPT_ARGS];
-
-		if (!*p)
-			continue;
+	int err = 0;
+	int token;
+	substring_t args[MAX_OPT_ARGS];
 
-		token = match_token(p, ovl_tokens, args);
-		switch (token) {
-		case OPT_UPPERDIR:
-			kfree(config->upperdir);
-			config->upperdir = match_strdup(&args[0]);
-			if (!config->upperdir)
-				return -ENOMEM;
-			break;
+	if (!*opt)
+		return 0;
 
-		case OPT_LOWERDIR:
-			kfree(config->lowerdir);
-			config->lowerdir = match_strdup(&args[0]);
-			if (!config->lowerdir)
-				return -ENOMEM;
-			break;
+	token = match_token(opt, ovl_tokens, args);
+	switch (token) {
+	case OPT_UPPERDIR:
+		kfree(config->upperdir);
+		config->upperdir = match_strdup(&args[0]);
+		if (!config->upperdir)
+			return -ENOMEM;
+		break;
+
+	case OPT_LOWERDIR:
+		kfree(config->lowerdir);
+		config->lowerdir = match_strdup(&args[0]);
+		if (!config->lowerdir)
+			return -ENOMEM;
+		break;
+
+	case OPT_WORKDIR:
+		kfree(config->workdir);
+		config->workdir = match_strdup(&args[0]);
+		if (!config->workdir)
+			return -ENOMEM;
+		break;
+
+	case OPT_DEFAULT_PERMISSIONS:
+		config->default_permissions = true;
+		break;
+
+	case OPT_REDIRECT_DIR_ON:
+		config->redirect_mode = OVL_REDIRECT_ON;
+		set->redirect = true;
+		break;
+
+	case OPT_REDIRECT_DIR_OFF:
+		config->redirect_mode = ovl_redirect_always_follow ?
+			OVL_REDIRECT_FOLLOW :
+			OVL_REDIRECT_NOFOLLOW;
+		set->redirect = true;
+		break;
+
+	case OPT_REDIRECT_DIR_FOLLOW:
+		config->redirect_mode = OVL_REDIRECT_FOLLOW;
+		set->redirect = true;
+		break;
+
+	case OPT_REDIRECT_DIR_NOFOLLOW:
+		config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
+		set->redirect = true;
+		break;
 
-		case OPT_WORKDIR:
-			kfree(config->workdir);
-			config->workdir = match_strdup(&args[0]);
-			if (!config->workdir)
-				return -ENOMEM;
-			break;
+	case OPT_INDEX_ON:
+		config->index = true;
+		set->index = true;
+		break;
 
-		case OPT_DEFAULT_PERMISSIONS:
-			config->default_permissions = true;
-			break;
+	case OPT_INDEX_OFF:
+		config->index = false;
+		set->index = true;
+		break;
 
-		case OPT_REDIRECT_DIR_ON:
-			config->redirect_mode = OVL_REDIRECT_ON;
-			redirect_opt = true;
-			break;
+	case OPT_UUID_ON:
+		config->uuid = true;
+		break;
 
-		case OPT_REDIRECT_DIR_OFF:
-			config->redirect_mode = ovl_redirect_always_follow ?
-						OVL_REDIRECT_FOLLOW :
-						OVL_REDIRECT_NOFOLLOW;
-			redirect_opt = true;
-			break;
+	case OPT_UUID_OFF:
+		config->uuid = false;
+		break;
 
-		case OPT_REDIRECT_DIR_FOLLOW:
-			config->redirect_mode = OVL_REDIRECT_FOLLOW;
-			redirect_opt = true;
-			break;
+	case OPT_NFS_EXPORT_ON:
+		config->nfs_export = true;
+		set->nfs_export = true;
+		break;
 
-		case OPT_REDIRECT_DIR_NOFOLLOW:
-			config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
-			redirect_opt = true;
-			break;
+	case OPT_NFS_EXPORT_OFF:
+		config->nfs_export = false;
+		set->nfs_export = true;
+		break;
 
-		case OPT_INDEX_ON:
-			config->index = true;
-			index_opt = true;
-			break;
+	case OPT_XINO_ON:
+		config->xino = OVL_XINO_ON;
+		break;
 
-		case OPT_INDEX_OFF:
-			config->index = false;
-			index_opt = true;
-			break;
+	case OPT_XINO_OFF:
+		config->xino = OVL_XINO_OFF;
+		break;
 
-		case OPT_UUID_ON:
-			config->uuid = true;
-			break;
+	case OPT_XINO_AUTO:
+		config->xino = OVL_XINO_AUTO;
+		break;
 
-		case OPT_UUID_OFF:
-			config->uuid = false;
-			break;
+	case OPT_METACOPY_ON:
+		config->metacopy = true;
+		set->metacopy = true;
+		break;
 
-		case OPT_NFS_EXPORT_ON:
-			config->nfs_export = true;
-			nfs_export_opt = true;
-			break;
-
-		case OPT_NFS_EXPORT_OFF:
-			config->nfs_export = false;
-			nfs_export_opt = true;
-			break;
-
-		case OPT_XINO_ON:
-			config->xino = OVL_XINO_ON;
-			break;
-
-		case OPT_XINO_OFF:
-			config->xino = OVL_XINO_OFF;
-			break;
+	case OPT_METACOPY_OFF:
+		config->metacopy = false;
+		set->metacopy = true;
+		break;
 
-		case OPT_XINO_AUTO:
-			config->xino = OVL_XINO_AUTO;
-			break;
+	case OPT_VOLATILE:
+		config->ovl_volatile = true;
+		break;
 
-		case OPT_METACOPY_ON:
-			config->metacopy = true;
-			metacopy_opt = true;
-			break;
+	case OPT_USERXATTR:
+		config->userxattr = true;
+		break;
 
-		case OPT_METACOPY_OFF:
-			config->metacopy = false;
-			metacopy_opt = true;
-			break;
+	default:
+		pr_err("unrecognized mount option \"%s\" or missing value\n",
+		       opt);
+		return -EINVAL;
+	}
 
-		case OPT_VOLATILE:
-			config->ovl_volatile = true;
-			break;
+	return err;
+}
 
-		case OPT_USERXATTR:
-			config->userxattr = true;
-			break;
+static int ovl_parse_options(char *opt, struct ovl_config *config)
+{
+	char *p;
+	int err;
+	struct ovl_opt_set set = {};
 
-		default:
-			pr_err("unrecognized mount option \"%s\" or missing value\n",
-					p);
-			return -EINVAL;
-		}
+	while ((p = ovl_next_opt(&opt)) != NULL) {
+		err = ovl_parse_opt(p, config, &set);
+		if (err)
+			return err;
 	}
 
 	/* Workdir/index are useless in non-upper mount */
@@ -649,9 +660,9 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			kfree(config->workdir);
 			config->workdir = NULL;
 		}
-		if (config->index && index_opt) {
+		if (config->index && set.index) {
 			pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
-			index_opt = false;
+			set.index = false;
 		}
 		config->index = false;
 	}
@@ -670,12 +681,12 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve metacopy -> redirect_dir dependency */
 	if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
-		if (metacopy_opt && redirect_opt) {
+		if (set.metacopy && set.redirect) {
 			pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
 			       ovl_redirect_mode(config));
 			return -EINVAL;
 		}
-		if (redirect_opt) {
+		if (set.redirect) {
 			/*
 			 * There was an explicit redirect_dir=... that resulted
 			 * in this conflict.
@@ -695,10 +706,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		    config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
 			pr_info("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
 			config->nfs_export = false;
-		} else if (nfs_export_opt && index_opt) {
+		} else if (set.nfs_export && set.index) {
 			pr_err("conflicting options: nfs_export=on,index=off\n");
 			return -EINVAL;
-		} else if (index_opt) {
+		} else if (set.index) {
 			/*
 			 * There was an explicit index=off that resulted
 			 * in this conflict.
@@ -713,11 +724,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve nfs_export -> !metacopy dependency */
 	if (config->nfs_export && config->metacopy) {
-		if (nfs_export_opt && metacopy_opt) {
+		if (set.nfs_export && set.metacopy) {
 			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
 			return -EINVAL;
 		}
-		if (metacopy_opt) {
+		if (set.metacopy) {
 			/*
 			 * There was an explicit metacopy=on that resulted
 			 * in this conflict.
@@ -737,13 +748,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve userxattr -> !redirect && !metacopy dependency */
 	if (config->userxattr) {
-		if (redirect_opt &&
+		if (set.redirect &&
 		    config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
 			pr_err("conflicting options: userxattr,redirect_dir=%s\n",
 			       ovl_redirect_mode(config));
 			return -EINVAL;
 		}
-		if (config->metacopy && metacopy_opt) {
+		if (config->metacopy && set.metacopy) {
 			pr_err("conflicting options: userxattr,metacopy=on\n");
 			return -EINVAL;
 		}
@@ -1981,7 +1992,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ofs->config.nfs_export = ovl_nfs_export_def;
 	ofs->config.xino = ovl_xino_def();
 	ofs->config.metacopy = ovl_metacopy_def;
-	err = ovl_parse_opt((char *) data, &ofs->config);
+	err = ovl_parse_options((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
 
-- 
2.34.1

Re: [PATCH v2 4/5] ovl: store enum redirect_mode in config instead of a string

[Miklos Szeredi]

On Sat, 17 Jun 2023 at 10:47, Amir Goldstein <amir73il@gmail.com> wrote:

> @@ -1332,10 +1337,17 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
>         if (err) {
>                 pr_warn("failed to set xattr on upper\n");
>                 ofs->noxattr = true;
> -               if (ofs->config.index || ofs->config.metacopy) {
> -                       ofs->config.index = false;
> +               if (ovl_redirect_follow(ofs)) {
> +                       ofs->config.redirect_mode = OVL_REDIRECT_NOFOLLOW;
> +                       pr_warn("...falling back to redirect_dir=nofollow.\n");


So if there's no xattr support, then there won't be any redirects to
follow.   Is this just asserting the fact?

Should this be a separate patch?

Thanks,
Miklos

Re: [PATCH v2 4/5] ovl: store enum redirect_mode in config instead of a string

[Miklos Szeredi]

On Tue, 20 Jun 2023 at 10:48, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 17 Jun 2023 at 10:47, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > @@ -1332,10 +1337,17 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
> >         if (err) {
> >                 pr_warn("failed to set xattr on upper\n");
> >                 ofs->noxattr = true;
> > -               if (ofs->config.index || ofs->config.metacopy) {
> > -                       ofs->config.index = false;
> > +               if (ovl_redirect_follow(ofs)) {
> > +                       ofs->config.redirect_mode = OVL_REDIRECT_NOFOLLOW;
> > +                       pr_warn("...falling back to redirect_dir=nofollow.\n");
>
>
> So if there's no xattr support, then there won't be any redirects to
> follow.   Is this just asserting the fact?
>
> Should this be a separate patch?

Oh, I see you are changing the ovl_redirect_dir() logic.   Ack.

Thanks,
Miklos

Re: [PATCH v2 5/5] ovl: factor out ovl_parse_options() helper

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:47:02AM +0300, Amir Goldstein wrote:
> For parsing a single mount option.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Fine by me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 1/5] ovl: negate the ofs->share_whiteout boolean

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:46:58AM +0300, Amir Goldstein wrote:
> The default common case is that whiteout sharing is enabled.
> Change to storing the negated no_shared_whiteout state, so we will not
> need to initialize it.
> 
> This is the first step towards removing all config and feature
> initializations out of ovl_fill_super().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 2/5] ovl: clarify ovl_get_root() semantics

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:46:59AM +0300, Amir Goldstein wrote:
> Change the semantics to take a reference on upperdentry instead
> of transferrig the reference.
> 
> This is needed for upcoming port to new mount api.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 3/5] ovl: pass ovl_fs to xino helpers

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:47:00AM +0300, Amir Goldstein wrote:
> Internal ovl methods should use ovl_fs and not sb as much as
> possible.
> 
> Use a constant_table to translate from enum xino mode to string
> in preperation for new mount api option parsing.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 4/5] ovl: store enum redirect_mode in config instead of a string

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:47:01AM +0300, Amir Goldstein wrote:
> Do all the logic to set the mode during mount options parsing and
> do not keep the option string around.
> 
> Use a constant_table to translate from enum redirect mode to string
> in preperation for new mount api option parsing.
> 
> The mount option "off" is translated to either "follow" or "nofollow",
> depending on the "redirect_always_follow" build/module config, so
> in effect, there are only three possible redirect modes.
> 
> This results in a minor change to the string that is displayed
> in show_options() - when redirect_dir is enabled by default and the user
> mounts with the option "redirect_dir=off", instead of displaying the mode
> "redirect_dir=off" in show_options(), the displayed mode will be either
> "redirect_dir=follow" or "redirect_dir=nofollow", depending on the value
> of "redirect_always_follow" build/module config.
> 
> The displayed mode reflects the effective mode, so mounting overlayfs
> again with the dispalyed redirect_dir option will result with the same
> effective and displayed mode.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 0/5] Prep patches for porting overlayfs to new mount api

[Christian Brauner]

On Sat, Jun 17, 2023 at 11:46:57AM +0300, Amir Goldstein wrote:
> Miklos,
> 
> Following some more cleanup patches that make Christian's new mount api
> patches smaller and easier to review.
> 
> I had rebased Christain's patches over these cleanups and pushed the
> result to github branch fs-overlayfs-mount_api [1].
> 
> The v1 prep patches had a bug with xino option parsing that resulted in
> some tests being skipped (not failing) and I had only noticed the
> skipped test after posting v1.
> 
> The v2 prep patches + new mount api patches have passed all the tests
> with no new tests skipped.
> 
> In addition to running the tests with the default kernel config, I also
> ran the tests with the following non-default configs (individually):
> 
> 1) CONFIG_OVERLAY_FS_REDIRECT_DIR=y
> 2) CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW=n
> 3) CONFIG_OVERLAY_FS_XINO_AUTO=y

Thanks for splitting some work into preparatory patches. I'm not sure
how worthwhile this actually is given they aren't marked as backports
for LTS releases so the overall delta ould still the same between LTSes
and mainline but it might make bisection easier.

Re: [PATCH v2 0/5] Prep patches for porting overlayfs to new mount api

[Amir Goldstein]

On Tue, Jun 20, 2023 at 12:26 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Jun 17, 2023 at 11:46:57AM +0300, Amir Goldstein wrote:
> > Miklos,
> >
> > Following some more cleanup patches that make Christian's new mount api
> > patches smaller and easier to review.
> >
> > I had rebased Christain's patches over these cleanups and pushed the
> > result to github branch fs-overlayfs-mount_api [1].
> >
> > The v1 prep patches had a bug with xino option parsing that resulted in
> > some tests being skipped (not failing) and I had only noticed the
> > skipped test after posting v1.
> >
> > The v2 prep patches + new mount api patches have passed all the tests
> > with no new tests skipped.
> >
> > In addition to running the tests with the default kernel config, I also
> > ran the tests with the following non-default configs (individually):
> >
> > 1) CONFIG_OVERLAY_FS_REDIRECT_DIR=y
> > 2) CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW=n
> > 3) CONFIG_OVERLAY_FS_XINO_AUTO=y
>
> Thanks for splitting some work into preparatory patches. I'm not sure
> how worthwhile this actually is given they aren't marked as backports
> for LTS releases so the overall delta ould still the same between LTSes
> and mainline but it might make bisection easier.

Yeh, bisection, review, all the usual reasons for keep unrelated changes
split. I am not usually fanatic about splitting hairs on this, but the
mount api porting patch was already a big change that was hard for me to
review and it grew all those extra additions like redirect_mode which
were good changes, but not related, so I did this to make my own (and others)
review of your patches easier.

I am glad if we are all happy with the end result.

Thanks,
Amir.

Re: [PATCH v2 0/5] Prep patches for porting overlayfs to new mount api

[Christian Brauner]

On Tue, Jun 20, 2023 at 12:46:52PM +0300, Amir Goldstein wrote:
> On Tue, Jun 20, 2023 at 12:26 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Sat, Jun 17, 2023 at 11:46:57AM +0300, Amir Goldstein wrote:
> > > Miklos,
> > >
> > > Following some more cleanup patches that make Christian's new mount api
> > > patches smaller and easier to review.
> > >
> > > I had rebased Christain's patches over these cleanups and pushed the
> > > result to github branch fs-overlayfs-mount_api [1].
> > >
> > > The v1 prep patches had a bug with xino option parsing that resulted in
> > > some tests being skipped (not failing) and I had only noticed the
> > > skipped test after posting v1.
> > >
> > > The v2 prep patches + new mount api patches have passed all the tests
> > > with no new tests skipped.
> > >
> > > In addition to running the tests with the default kernel config, I also
> > > ran the tests with the following non-default configs (individually):
> > >
> > > 1) CONFIG_OVERLAY_FS_REDIRECT_DIR=y
> > > 2) CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW=n
> > > 3) CONFIG_OVERLAY_FS_XINO_AUTO=y
> >
> > Thanks for splitting some work into preparatory patches. I'm not sure
> > how worthwhile this actually is given they aren't marked as backports
> > for LTS releases so the overall delta ould still the same between LTSes
> > and mainline but it might make bisection easier.
> 
> Yeh, bisection, review, all the usual reasons for keep unrelated changes
> split. I am not usually fanatic about splitting hairs on this, but the
> mount api porting patch was already a big change that was hard for me to
> review and it grew all those extra additions like redirect_mode which
> were good changes, but not related, so I did this to make my own (and others)
> review of your patches easier.
> 
> I am glad if we are all happy with the end result.

Yeah, I appreciate the work and wasn't trying to critique it. I just
wouldn't have bothered because the mount api port in itself is
cumbersome enough. :)