[PATCH 0/4] ovl: allow virtiofs as upper

[PATCH 0/4] ovl: allow virtiofs as upper

[Miklos Szeredi]

The purpose of this mini-series is to allow virtiofs as upper layer of
overlayfs.

Applies on top of #overlayfs-next (commit 1a980b8cbf00 ("ovl: add splice
file read write helper")).

Git tree is:

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git#ovl-remote-upper

Thanks,
Miklos

---
Miklos Szeredi (4):
  ovl: restructure dentry revalidation
  ovl: separate detection of remote upper layer from stacked overlay
  ovl: decide if revalidate needed on a per-dentry bases
  ovl: alllow remote upper

 fs/overlayfs/dir.c       |  3 ++
 fs/overlayfs/export.c    |  2 +
 fs/overlayfs/namei.c     |  5 ++-
 fs/overlayfs/overlayfs.h |  3 +-
 fs/overlayfs/super.c     | 90 +++++++++++++++++++---------------------
 fs/overlayfs/util.c      | 18 ++++++--
 6 files changed, 68 insertions(+), 53 deletions(-)

-- 
2.21.1

[PATCH 1/4] ovl: restructure dentry revalidation

[Miklos Szeredi]

Use a common loop for plain and weak revalidation.  This will aid doing
revalidation on upper layer.

This patch doesn't change behavior.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/super.c | 51 ++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 319fe0d355b0..852a1816fea1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -113,47 +113,48 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	return dentry;
 }
 
-static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
+static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
-	unsigned int i;
 	int ret = 1;
 
-	for (i = 0; i < oe->numlower; i++) {
-		struct dentry *d = oe->lowerstack[i].dentry;
-
-		if (d->d_flags & DCACHE_OP_REVALIDATE) {
-			ret = d->d_op->d_revalidate(d, flags);
-			if (ret < 0)
-				return ret;
-			if (!ret) {
-				if (!(flags & LOOKUP_RCU))
-					d_invalidate(d);
-				return -ESTALE;
-			}
+	if (weak) {
+		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
+			ret =  d->d_op->d_weak_revalidate(d, flags);
+	} else if (d->d_flags & DCACHE_OP_REVALIDATE) {
+		ret = d->d_op->d_revalidate(d, flags);
+		if (!ret) {
+			if (!(flags & LOOKUP_RCU))
+				d_invalidate(d);
+			ret = -ESTALE;
 		}
 	}
-	return 1;
+	return ret;
 }
 
-static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
+static int ovl_dentry_revalidate_common(struct dentry *dentry,
+					unsigned int flags, bool weak)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
 	unsigned int i;
 	int ret = 1;
 
-	for (i = 0; i < oe->numlower; i++) {
-		struct dentry *d = oe->lowerstack[i].dentry;
-
-		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
-			ret = d->d_op->d_weak_revalidate(d, flags);
-			if (ret <= 0)
-				break;
-		}
+	for (i = 0; ret > 0 && i < oe->numlower; i++) {
+		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
+					  weak);
 	}
 	return ret;
 }
 
+static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
+{
+	return ovl_dentry_revalidate_common(dentry, flags, false);
+}
+
+static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
+{
+	return ovl_dentry_revalidate_common(dentry, flags, true);
+}
+
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_real = ovl_d_real,
-- 
2.21.1

[PATCH 2/4] ovl: separate detection of remote upper layer from stacked overlay

[Miklos Szeredi]

Following patch will allow remote as upper layer, but not overlay stacked
on upper layer.  Separate the two concepts.

This patch is doesn't change behavior.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/namei.c |  3 ++-
 fs/overlayfs/super.c | 14 +++++++-------
 fs/overlayfs/util.c  |  3 +--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ed9e129fae04..a5b998a93a24 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -845,7 +845,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (err)
 			goto out;
 
-		if (upperdentry && unlikely(ovl_dentry_remote(upperdentry))) {
+		if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL ||
+				    unlikely(ovl_dentry_remote(upperdentry)))) {
 			dput(upperdentry);
 			err = -EREMOTE;
 			goto out;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 852a1816fea1..7e294bf719ff 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -752,13 +752,13 @@ static int ovl_mount_dir(const char *name, struct path *path)
 		ovl_unescape(tmp);
 		err = ovl_mount_dir_noesc(tmp, path);
 
-		if (!err)
-			if (ovl_dentry_remote(path->dentry)) {
-				pr_err("filesystem on '%s' not supported as upperdir\n",
-				       tmp);
-				path_put_init(path);
-				err = -EINVAL;
-			}
+		if (!err && (ovl_dentry_remote(path->dentry) ||
+			     path->dentry->d_flags & DCACHE_OP_REAL)) {
+			pr_err("filesystem on '%s' not supported as upperdir\n",
+			       tmp);
+			path_put_init(path);
+			err = -EINVAL;
+		}
 		kfree(tmp);
 	}
 	return err;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index ea005085803f..67cd2866aaa2 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -93,8 +93,7 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 bool ovl_dentry_remote(struct dentry *dentry)
 {
 	return dentry->d_flags &
-		(DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE |
-		 DCACHE_OP_REAL);
+		(DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
 }
 
 bool ovl_dentry_weird(struct dentry *dentry)
-- 
2.21.1

[PATCH 3/4] ovl: decide if revalidate needed on a per-dentry bases

[Miklos Szeredi]

Allow completely skipping ->revalidate() on a per-dentry bases, in case the
underlying layers used for a dentry do not themselves have ->revalidate().

E.g. negative overlay dentry has no underlying layers, hence revalidate is
unnecessary.  Or if lower layer is remote but overlay dentry is pure-upper,
then can skip revalidate.

The following places need to update whether the dentry needs revalidate or
not:

 - fill-super (root dentry)
 - lookup
 - create
 - fh_to_dentry

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c       |  3 +++
 fs/overlayfs/export.c    |  2 ++
 fs/overlayfs/namei.c     |  3 +++
 fs/overlayfs/overlayfs.h |  3 ++-
 fs/overlayfs/super.c     | 23 +++++++----------------
 fs/overlayfs/util.c      | 15 ++++++++++++---
 6 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 8e57d5372b8f..b3471ef51440 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -243,6 +243,9 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 
 	ovl_dir_modified(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
+	ovl_dentry_update_reval(dentry, newdentry,
+			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+
 	if (!hardlink) {
 		/*
 		 * ovl_obtain_alias() can be called after ovl_create_real()
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 6f54d70cef27..a58b3d9b06b9 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -324,6 +324,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 		if (upper_alias)
 			ovl_dentry_set_upper_alias(dentry);
 	}
+	ovl_dentry_update_reval(dentry, upper,
+			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
 
 	return d_instantiate_anon(dentry, inode);
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index a5b998a93a24..76e61cc27822 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1077,6 +1077,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_free_oe;
 	}
 
+	ovl_dentry_update_reval(dentry, upperdentry,
+			DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+
 	revert_creds(old_cred);
 	if (origin_path) {
 		dput(origin_path->dentry);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3623d28aa4fa..68124a4f8f9b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -229,7 +229,8 @@ struct dentry *ovl_indexdir(struct super_block *sb);
 bool ovl_index_all(struct super_block *sb);
 bool ovl_verify_lower(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
-bool ovl_dentry_remote(struct dentry *dentry);
+void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
+			     unsigned int mask);
 bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7e294bf719ff..26d4153240a8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -158,11 +158,6 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_real = ovl_d_real,
-};
-
-static const struct dentry_operations ovl_reval_dentry_operations = {
-	.d_release = ovl_dentry_release,
-	.d_real = ovl_d_real,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
@@ -779,7 +774,7 @@ static int ovl_check_namelen(struct path *path, struct ovl_fs *ofs,
 }
 
 static int ovl_lower_dir(const char *name, struct path *path,
-			 struct ovl_fs *ofs, int *stack_depth, bool *remote)
+			 struct ovl_fs *ofs, int *stack_depth)
 {
 	int fh_type;
 	int err;
@@ -794,9 +789,6 @@ static int ovl_lower_dir(const char *name, struct path *path,
 
 	*stack_depth = max(*stack_depth, path->mnt->mnt_sb->s_stack_depth);
 
-	if (ovl_dentry_remote(path->dentry))
-		*remote = true;
-
 	/*
 	 * The inodes index feature and NFS export need to encode and decode
 	 * file handles, so they require that all layers support them.
@@ -1439,7 +1431,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	char *lowertmp, *lower;
 	struct path *stack = NULL;
 	unsigned int stacklen, numlower = 0, i;
-	bool remote = false;
 	struct ovl_entry *oe;
 
 	err = -ENOMEM;
@@ -1471,7 +1462,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	lower = lowertmp;
 	for (numlower = 0; numlower < stacklen; numlower++) {
 		err = ovl_lower_dir(lower, &stack[numlower], ofs,
-				    &sb->s_stack_depth, &remote);
+				    &sb->s_stack_depth);
 		if (err)
 			goto out_err;
 
@@ -1499,11 +1490,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		oe->lowerstack[i].layer = &ofs->layers[i+1];
 	}
 
-	if (remote)
-		sb->s_d_op = &ovl_reval_dentry_operations;
-	else
-		sb->s_d_op = &ovl_dentry_operations;
-
 out:
 	for (i = 0; i < numlower; i++)
 		path_put(&stack[i]);
@@ -1597,6 +1583,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	struct cred *cred;
 	int err;
 
+	sb->s_d_op = &ovl_dentry_operations;
+
 	err = -ENOMEM;
 	ofs = kzalloc(sizeof(struct ovl_fs), GFP_KERNEL);
 	if (!ofs)
@@ -1724,6 +1712,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ovl_inode_init(d_inode(root_dentry), upperpath.dentry,
 		       ovl_dentry_lower(root_dentry), NULL);
 
+	ovl_dentry_update_reval(root_dentry, upperpath.dentry,
+				DCACHE_OP_WEAK_REVALIDATE);
+
 	sb->s_root = root_dentry;
 
 	return 0;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 67cd2866aaa2..3ad8fb291f7d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -90,10 +90,19 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 	return oe;
 }
 
-bool ovl_dentry_remote(struct dentry *dentry)
+void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
+			     unsigned int mask)
 {
-	return dentry->d_flags &
-		(DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
+	struct ovl_entry *oe = OVL_E(dentry);
+	unsigned int i, flags = 0;
+
+	for (i = 0; i < oe->numlower; i++)
+		flags |= oe->lowerstack[i].dentry->d_flags;
+
+	spin_lock(&dentry->d_lock);
+	dentry->d_flags &= ~mask;
+	dentry->d_flags |= flags & mask;
+	spin_unlock(&dentry->d_lock);
 }
 
 bool ovl_dentry_weird(struct dentry *dentry)
-- 
2.21.1

[PATCH 4/4] ovl: alllow remote upper

[Miklos Szeredi]

No reason to prevent upper layer being a remote filesystem.  Do the
revalidation in that case, just as we already do for lower layers.

This lets virtiofs be used as upper layer, which appears to be a real use
case.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/namei.c | 3 +--
 fs/overlayfs/super.c | 8 ++++++--
 fs/overlayfs/util.c  | 2 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 76e61cc27822..0db23baf98e7 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (err)
 			goto out;
 
-		if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL ||
-				    unlikely(ovl_dentry_remote(upperdentry)))) {
+		if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) {
 			dput(upperdentry);
 			err = -EREMOTE;
 			goto out;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 26d4153240a8..ed3a11db9039 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 					unsigned int flags, bool weak)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+	struct dentry *upper;
 	unsigned int i;
 	int ret = 1;
 
+	upper = ovl_dentry_upper(dentry);
+	if (upper)
+		ret = ovl_revalidate_real(upper, flags, weak);
+
 	for (i = 0; ret > 0 && i < oe->numlower; i++) {
 		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
 					  weak);
@@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path)
 		ovl_unescape(tmp);
 		err = ovl_mount_dir_noesc(tmp, path);
 
-		if (!err && (ovl_dentry_remote(path->dentry) ||
-			     path->dentry->d_flags & DCACHE_OP_REAL)) {
+		if (!err && path->dentry->d_flags & DCACHE_OP_REAL) {
 			pr_err("filesystem on '%s' not supported as upperdir\n",
 			       tmp);
 			path_put_init(path);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3ad8fb291f7d..c793722739e1 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
 	struct ovl_entry *oe = OVL_E(dentry);
 	unsigned int i, flags = 0;
 
+	if (upperdentry)
+		flags |= upperdentry->d_flags;
 	for (i = 0; i < oe->numlower; i++)
 		flags |= oe->lowerstack[i].dentry->d_flags;
 
-- 
2.21.1

Re: [PATCH 3/4] ovl: decide if revalidate needed on a per-dentry bases

[Amir Goldstein]

On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> Allow completely skipping ->revalidate() on a per-dentry bases, in case the
> underlying layers used for a dentry do not themselves have ->revalidate().
>
> E.g. negative overlay dentry has no underlying layers, hence revalidate is
> unnecessary.  Or if lower layer is remote but overlay dentry is pure-upper,
> then can skip revalidate.
>
> The following places need to update whether the dentry needs revalidate or
> not:
>
>  - fill-super (root dentry)
>  - lookup
>  - create
>  - fh_to_dentry
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/dir.c       |  3 +++
>  fs/overlayfs/export.c    |  2 ++
>  fs/overlayfs/namei.c     |  3 +++
>  fs/overlayfs/overlayfs.h |  3 ++-
>  fs/overlayfs/super.c     | 23 +++++++----------------
>  fs/overlayfs/util.c      | 15 ++++++++++++---
>  6 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 8e57d5372b8f..b3471ef51440 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -243,6 +243,9 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
>
>         ovl_dir_modified(dentry->d_parent, false);
>         ovl_dentry_set_upper_alias(dentry);
> +       ovl_dentry_update_reval(dentry, newdentry,
> +                       DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
> +
>         if (!hardlink) {
>                 /*
>                  * ovl_obtain_alias() can be called after ovl_create_real()
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 6f54d70cef27..a58b3d9b06b9 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -324,6 +324,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>                 if (upper_alias)
>                         ovl_dentry_set_upper_alias(dentry);
>         }
> +       ovl_dentry_update_reval(dentry, upper,
> +                       DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
>
>         return d_instantiate_anon(dentry, inode);
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index a5b998a93a24..76e61cc27822 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1077,6 +1077,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_free_oe;
>         }
>
> +       ovl_dentry_update_reval(dentry, upperdentry,
> +                       DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE);
> +
>         revert_creds(old_cred);
>         if (origin_path) {
>                 dput(origin_path->dentry);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 3623d28aa4fa..68124a4f8f9b 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -229,7 +229,8 @@ struct dentry *ovl_indexdir(struct super_block *sb);
>  bool ovl_index_all(struct super_block *sb);
>  bool ovl_verify_lower(struct super_block *sb);
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
> -bool ovl_dentry_remote(struct dentry *dentry);
> +void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
> +                            unsigned int mask);
>  bool ovl_dentry_weird(struct dentry *dentry);
>  enum ovl_path_type ovl_path_type(struct dentry *dentry);
>  void ovl_path_upper(struct dentry *dentry, struct path *path);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 7e294bf719ff..26d4153240a8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -158,11 +158,6 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  static const struct dentry_operations ovl_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_real = ovl_d_real,
> -};
> -
> -static const struct dentry_operations ovl_reval_dentry_operations = {
> -       .d_release = ovl_dentry_release,
> -       .d_real = ovl_d_real,
>         .d_revalidate = ovl_dentry_revalidate,
>         .d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
> @@ -779,7 +774,7 @@ static int ovl_check_namelen(struct path *path, struct ovl_fs *ofs,
>  }
>
>  static int ovl_lower_dir(const char *name, struct path *path,
> -                        struct ovl_fs *ofs, int *stack_depth, bool *remote)
> +                        struct ovl_fs *ofs, int *stack_depth)
>  {
>         int fh_type;
>         int err;
> @@ -794,9 +789,6 @@ static int ovl_lower_dir(const char *name, struct path *path,
>
>         *stack_depth = max(*stack_depth, path->mnt->mnt_sb->s_stack_depth);
>
> -       if (ovl_dentry_remote(path->dentry))
> -               *remote = true;
> -
>         /*
>          * The inodes index feature and NFS export need to encode and decode
>          * file handles, so they require that all layers support them.
> @@ -1439,7 +1431,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         char *lowertmp, *lower;
>         struct path *stack = NULL;
>         unsigned int stacklen, numlower = 0, i;
> -       bool remote = false;
>         struct ovl_entry *oe;
>
>         err = -ENOMEM;
> @@ -1471,7 +1462,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         lower = lowertmp;
>         for (numlower = 0; numlower < stacklen; numlower++) {
>                 err = ovl_lower_dir(lower, &stack[numlower], ofs,
> -                                   &sb->s_stack_depth, &remote);
> +                                   &sb->s_stack_depth);
>                 if (err)
>                         goto out_err;
>
> @@ -1499,11 +1490,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>                 oe->lowerstack[i].layer = &ofs->layers[i+1];
>         }
>
> -       if (remote)
> -               sb->s_d_op = &ovl_reval_dentry_operations;
> -       else
> -               sb->s_d_op = &ovl_dentry_operations;
> -
>  out:
>         for (i = 0; i < numlower; i++)
>                 path_put(&stack[i]);
> @@ -1597,6 +1583,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         struct cred *cred;
>         int err;
>
> +       sb->s_d_op = &ovl_dentry_operations;
> +
>         err = -ENOMEM;
>         ofs = kzalloc(sizeof(struct ovl_fs), GFP_KERNEL);
>         if (!ofs)
> @@ -1724,6 +1712,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         ovl_inode_init(d_inode(root_dentry), upperpath.dentry,
>                        ovl_dentry_lower(root_dentry), NULL);
>
> +       ovl_dentry_update_reval(root_dentry, upperpath.dentry,
> +                               DCACHE_OP_WEAK_REVALIDATE);
> +
>         sb->s_root = root_dentry;
>
>         return 0;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 67cd2866aaa2..3ad8fb291f7d 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -90,10 +90,19 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
>         return oe;
>  }
>
> -bool ovl_dentry_remote(struct dentry *dentry)

Removed too early. It still has users.
Otherwise looks ok.

Thanks,
Amir.

Re: [PATCH 3/4] ovl: decide if revalidate needed on a per-dentry bases

[Miklos Szeredi]

> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 67cd2866aaa2..3ad8fb291f7d 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -90,10 +90,19 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
> >         return oe;
> >  }
> >
> > -bool ovl_dentry_remote(struct dentry *dentry)
>
> Removed too early. It still has users.
> Otherwise looks ok.

Thanks, fixed ones pushed to @ovl-remote-upper-v2.

Thanks,
Miklos

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> No reason to prevent upper layer being a remote filesystem.  Do the
> revalidation in that case, just as we already do for lower layers.
>

No reason to prevent upper layer from being a remote filesystem, but
the !remote criteria for upper fs kept away a lot of filesystems from
upper. Those filesystems have never been tested as upper and many
of them are probably not fit for upper.

The goal is to lift the !remote limitation, not to allow for lots of new
types of upper fs's.

What can we do to minimize damages?

We can assert that is upper is remote, it must qualify for a more strict
criteria as upper fs, that is:
- support d_type
- support xattr
- support RENAME_EXCHANGE|RENAME_WHITEOUT

I have a patch on branch ovl-strict which implements those restrictions.

Now I know fuse doesn't support RENAME_WHITEOUT, but it does
support RENAME_EXCHANGE, which already proves to be a very narrow
filter for remote fs: afs, fuse, gfs2.
Did not check if afs, gfs2 qualify for the rest of the criteria.

Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs?
Is it not a problem to rely on an upper fs for modern systems
that does not support RENAME_WHITEOUT?

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Miklos Szeredi]

On Fri, Jan 31, 2020 at 4:30 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > No reason to prevent upper layer being a remote filesystem.  Do the
> > revalidation in that case, just as we already do for lower layers.
> >
>
> No reason to prevent upper layer from being a remote filesystem, but
> the !remote criteria for upper fs kept away a lot of filesystems from
> upper. Those filesystems have never been tested as upper and many
> of them are probably not fit for upper.
>
> The goal is to lift the !remote limitation, not to allow for lots of new
> types of upper fs's.
>
> What can we do to minimize damages?
>
> We can assert that is upper is remote, it must qualify for a more strict
> criteria as upper fs, that is:
> - support d_type
> - support xattr
> - support RENAME_EXCHANGE|RENAME_WHITEOUT
>
> I have a patch on branch ovl-strict which implements those restrictions.

Sounds good.  Not sure how much this is this going to be a
compatibility headache.  If it does, then we can conditionally enable
this with a config/module option.

>
> Now I know fuse doesn't support RENAME_WHITEOUT, but it does
> support RENAME_EXCHANGE, which already proves to be a very narrow
> filter for remote fs: afs, fuse, gfs2.
> Did not check if afs, gfs2 qualify for the rest of the criteria.
>
> Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs?

Trivial.

> Is it not a problem to rely on an upper fs for modern systems
> that does not support RENAME_WHITEOUT?

Limited r/w overlay functionality is still available without
whiteout/xattr support, so it could turn out to be something people
already rely on.  Can't tell without trying...

Thanks,
Miklos

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Fri, Jan 31, 2020 at 5:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, Jan 31, 2020 at 4:30 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Jan 31, 2020 at 1:51 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > >
> > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > revalidation in that case, just as we already do for lower layers.
> > >
> >
> > No reason to prevent upper layer from being a remote filesystem, but
> > the !remote criteria for upper fs kept away a lot of filesystems from
> > upper. Those filesystems have never been tested as upper and many
> > of them are probably not fit for upper.
> >
> > The goal is to lift the !remote limitation, not to allow for lots of new
> > types of upper fs's.
> >
> > What can we do to minimize damages?
> >
> > We can assert that is upper is remote, it must qualify for a more strict
> > criteria as upper fs, that is:
> > - support d_type
> > - support xattr
> > - support RENAME_EXCHANGE|RENAME_WHITEOUT
> >
> > I have a patch on branch ovl-strict which implements those restrictions.
>
> Sounds good.  Not sure how much this is this going to be a
> compatibility headache.  If it does, then we can conditionally enable
> this with a config/module option.
>

No headache at all:
- For now, do not change criteria for !remote fs
- Only remote fs needs to meet the most strict criteria
- We can add the 'strict' config later if we want impose
  same criteria also for local fs

> >
> > Now I know fuse doesn't support RENAME_WHITEOUT, but it does
> > support RENAME_EXCHANGE, which already proves to be a very narrow
> > filter for remote fs: afs, fuse, gfs2.
> > Did not check if afs, gfs2 qualify for the rest of the criteria.
> >

I checked - afs has d_automount and gfs2 is d_hash.
They do not qualify as any layer.

> > Is it simple to implement RENAME_WHITEOUT for fuse/virtiofs?
>
> Trivial.
>

So that leaves only fuse after implementing RENAME_WHITEOUT.
We are back in control ;-)

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Miklos Szeredi]

On Fri, Jan 31, 2020 at 4:50 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> I checked - afs has d_automount and gfs2 is d_hash.
> They do not qualify as any layer.

DCACHE_NEED_AUTOMOUNT doesn't work that way: it's set on specific
automount-point dentries only.  So AFS won't be disqualified based on
that criteria.   But afs also does not have RENAME_WHITEOUT, so that's
fine.

Thanks,
MIklos

Re: [PATCH 4/4] ovl: alllow remote upper

[Vivek Goyal]

On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> No reason to prevent upper layer being a remote filesystem.  Do the
> revalidation in that case, just as we already do for lower layers.
> 
> This lets virtiofs be used as upper layer, which appears to be a real use
> case.

Hi Miklos,

I have couple of very basic questions.

- So with this change, we will allow NFS to be upper layer also?

- What does revalidation on lower/upper mean? Does that mean that
  lower/upper can now change underneath overlayfs and overlayfs will
  cope with it. If we still expect underlying layers not to change, then
  what's the point of calling ->revalidate().

Thanks
Vivek

> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/namei.c | 3 +--
>  fs/overlayfs/super.c | 8 ++++++--
>  fs/overlayfs/util.c  | 2 ++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 76e61cc27822..0db23baf98e7 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  		if (err)
>  			goto out;
>  
> -		if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL ||
> -				    unlikely(ovl_dentry_remote(upperdentry)))) {
> +		if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) {
>  			dput(upperdentry);
>  			err = -EREMOTE;
>  			goto out;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 26d4153240a8..ed3a11db9039 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
>  					unsigned int flags, bool weak)
>  {
>  	struct ovl_entry *oe = dentry->d_fsdata;
> +	struct dentry *upper;
>  	unsigned int i;
>  	int ret = 1;
>  
> +	upper = ovl_dentry_upper(dentry);
> +	if (upper)
> +		ret = ovl_revalidate_real(upper, flags, weak);
> +
>  	for (i = 0; ret > 0 && i < oe->numlower; i++) {
>  		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
>  					  weak);
> @@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path)
>  		ovl_unescape(tmp);
>  		err = ovl_mount_dir_noesc(tmp, path);
>  
> -		if (!err && (ovl_dentry_remote(path->dentry) ||
> -			     path->dentry->d_flags & DCACHE_OP_REAL)) {
> +		if (!err && path->dentry->d_flags & DCACHE_OP_REAL) {
>  			pr_err("filesystem on '%s' not supported as upperdir\n",
>  			       tmp);
>  			path_put_init(path);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 3ad8fb291f7d..c793722739e1 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
>  	struct ovl_entry *oe = OVL_E(dentry);
>  	unsigned int i, flags = 0;
>  
> +	if (upperdentry)
> +		flags |= upperdentry->d_flags;
>  	for (i = 0; i < oe->numlower; i++)
>  		flags |= oe->lowerstack[i].dentry->d_flags;
>  
> -- 
> 2.21.1
> 

Re: [PATCH 4/4] ovl: alllow remote upper

[Miklos Szeredi]

On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > No reason to prevent upper layer being a remote filesystem.  Do the
> > revalidation in that case, just as we already do for lower layers.
> >
> > This lets virtiofs be used as upper layer, which appears to be a real use
> > case.
>
> Hi Miklos,
>
> I have couple of very basic questions.
>
> - So with this change, we will allow NFS to be upper layer also?

I haven't tested, but I think it will fail on the d_type test.

> - What does revalidation on lower/upper mean? Does that mean that
>   lower/upper can now change underneath overlayfs and overlayfs will
>   cope with it.

No, that's a more complicated thing.  Especially with redirected
layers (i.e. revalidating a redirect actually means revalidating all
the path components of that redirect).

> If we still expect underlying layers not to change, then
>   what's the point of calling ->revalidate().

That's a good question; I guess because that's what the filesystem
expects.  OTOH, it's probably unnecessary in most cases, since the
path could come from an open file descriptor, in which case the vfs
will not do any revalidation on that path.

So this is basically done to be on the safe side, but it might not be necessary.

Thanks,
Miklos

> Thanks
> Vivek
>
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 3 +--
> >  fs/overlayfs/super.c | 8 ++++++--
> >  fs/overlayfs/util.c  | 2 ++
> >  3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 76e61cc27822..0db23baf98e7 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -845,8 +845,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >               if (err)
> >                       goto out;
> >
> > -             if (upperdentry && (upperdentry->d_flags & DCACHE_OP_REAL ||
> > -                                 unlikely(ovl_dentry_remote(upperdentry)))) {
> > +             if (upperdentry && upperdentry->d_flags & DCACHE_OP_REAL) {
> >                       dput(upperdentry);
> >                       err = -EREMOTE;
> >                       goto out;
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 26d4153240a8..ed3a11db9039 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -135,9 +135,14 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
> >                                       unsigned int flags, bool weak)
> >  {
> >       struct ovl_entry *oe = dentry->d_fsdata;
> > +     struct dentry *upper;
> >       unsigned int i;
> >       int ret = 1;
> >
> > +     upper = ovl_dentry_upper(dentry);
> > +     if (upper)
> > +             ret = ovl_revalidate_real(upper, flags, weak);
> > +
> >       for (i = 0; ret > 0 && i < oe->numlower; i++) {
> >               ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
> >                                         weak);
> > @@ -747,8 +752,7 @@ static int ovl_mount_dir(const char *name, struct path *path)
> >               ovl_unescape(tmp);
> >               err = ovl_mount_dir_noesc(tmp, path);
> >
> > -             if (!err && (ovl_dentry_remote(path->dentry) ||
> > -                          path->dentry->d_flags & DCACHE_OP_REAL)) {
> > +             if (!err && path->dentry->d_flags & DCACHE_OP_REAL) {
> >                       pr_err("filesystem on '%s' not supported as upperdir\n",
> >                              tmp);
> >                       path_put_init(path);
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 3ad8fb291f7d..c793722739e1 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -96,6 +96,8 @@ void ovl_dentry_update_reval(struct dentry *dentry, struct dentry *upperdentry,
> >       struct ovl_entry *oe = OVL_E(dentry);
> >       unsigned int i, flags = 0;
> >
> > +     if (upperdentry)
> > +             flags |= upperdentry->d_flags;
> >       for (i = 0; i < oe->numlower; i++)
> >               flags |= oe->lowerstack[i].dentry->d_flags;
> >
> > --
> > 2.21.1
> >
>

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > revalidation in that case, just as we already do for lower layers.
> > >
> > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > case.
> >
> > Hi Miklos,
> >
> > I have couple of very basic questions.
> >
> > - So with this change, we will allow NFS to be upper layer also?
>
> I haven't tested, but I think it will fail on the d_type test.

But we do not fail mount on no d_type support...
Besides, I though you were going to add the RENAME_WHITEOUT
test to avert untested network fs as upper.

>
> > - What does revalidation on lower/upper mean? Does that mean that
> >   lower/upper can now change underneath overlayfs and overlayfs will
> >   cope with it.
>
> No, that's a more complicated thing.  Especially with redirected
> layers (i.e. revalidating a redirect actually means revalidating all
> the path components of that redirect).
>
> > If we still expect underlying layers not to change, then
> >   what's the point of calling ->revalidate().
>
> That's a good question; I guess because that's what the filesystem
> expects.  OTOH, it's probably unnecessary in most cases, since the
> path could come from an open file descriptor, in which case the vfs
> will not do any revalidation on that path.
>

Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs
will never actually redo the lookup, but instead return -ESTALE
to userspace. Right? This makes some sense considering that underlying
layers are not expected to change.

The question is, with virtiofs, can we know that the server/host will not
invalidate entries? And if it does, should it cause a permanent error
in overlayfs or a transient error? If we do not want a permanent error,
then ->revalidate() needs to be called to invalidate the overlay dentry. No?

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Vivek Goyal]

On Tue, Feb 04, 2020 at 07:02:05PM +0200, Amir Goldstein wrote:
> On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > revalidation in that case, just as we already do for lower layers.
> > > >
> > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > case.
> > >
> > > Hi Miklos,
> > >
> > > I have couple of very basic questions.
> > >
> > > - So with this change, we will allow NFS to be upper layer also?
> >
> > I haven't tested, but I think it will fail on the d_type test.
> 
> But we do not fail mount on no d_type support...
> Besides, I though you were going to add the RENAME_WHITEOUT
> test to avert untested network fs as upper.
> 
> >
> > > - What does revalidation on lower/upper mean? Does that mean that
> > >   lower/upper can now change underneath overlayfs and overlayfs will
> > >   cope with it.
> >
> > No, that's a more complicated thing.  Especially with redirected
> > layers (i.e. revalidating a redirect actually means revalidating all
> > the path components of that redirect).
> >
> > > If we still expect underlying layers not to change, then
> > >   what's the point of calling ->revalidate().
> >
> > That's a good question; I guess because that's what the filesystem
> > expects.  OTOH, it's probably unnecessary in most cases, since the
> > path could come from an open file descriptor, in which case the vfs
> > will not do any revalidation on that path.
> >
> 
> Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs
> will never actually redo the lookup, but instead return -ESTALE
> to userspace. Right? This makes some sense considering that underlying
> layers are not expected to change.
> 
> The question is, with virtiofs, can we know that the server/host will not
> invalidate entries?

I don't think virtiofs will ensure that server/host will not invalidate
entries. It will be more of a configuration thing where we will expect
users to configure things in such a way that shared directory does not
change.

So that means, if user does not configure it properly and things change
unexpectedly, then overlayfs should be able to detect it and flag error
to user space?

> And if it does, should it cause a permanent error
> in overlayfs or a transient error? If we do not want a permanent error,
> then ->revalidate() needs to be called to invalidate the overlay dentry. No?

So as of now user space will get -ESTALE and that will get cleared when
user space retries after corresponding ovl dentry has been dropped from
cache (either dentry is evicted, cache is cleared forcibly or overlayfs
is remounted)? If yes, that kind of makes sense. Overlay does not expect
underlying layers to change and if a change it detected it is flagged
to user space (and overlayfs does not try to fix it)?

Thanks
Vivek

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Tue, Feb 4, 2020 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Feb 04, 2020 at 07:02:05PM +0200, Amir Goldstein wrote:
> > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > > revalidation in that case, just as we already do for lower layers.
> > > > >
> > > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > > case.
> > > >
> > > > Hi Miklos,
> > > >
> > > > I have couple of very basic questions.
> > > >
> > > > - So with this change, we will allow NFS to be upper layer also?
> > >
> > > I haven't tested, but I think it will fail on the d_type test.
> >
> > But we do not fail mount on no d_type support...
> > Besides, I though you were going to add the RENAME_WHITEOUT
> > test to avert untested network fs as upper.
> >
> > >
> > > > - What does revalidation on lower/upper mean? Does that mean that
> > > >   lower/upper can now change underneath overlayfs and overlayfs will
> > > >   cope with it.
> > >
> > > No, that's a more complicated thing.  Especially with redirected
> > > layers (i.e. revalidating a redirect actually means revalidating all
> > > the path components of that redirect).
> > >
> > > > If we still expect underlying layers not to change, then
> > > >   what's the point of calling ->revalidate().
> > >
> > > That's a good question; I guess because that's what the filesystem
> > > expects.  OTOH, it's probably unnecessary in most cases, since the
> > > path could come from an open file descriptor, in which case the vfs
> > > will not do any revalidation on that path.
> > >
> >
> > Note that ovl_dentry_revalidate() never returns 0 and therefore, vfs
> > will never actually redo the lookup, but instead return -ESTALE
> > to userspace. Right? This makes some sense considering that underlying
> > layers are not expected to change.
> >
> > The question is, with virtiofs, can we know that the server/host will not
> > invalidate entries?
>
> I don't think virtiofs will ensure that server/host will not invalidate
> entries. It will be more of a configuration thing where we will expect
> users to configure things in such a way that shared directory does not
> change.
>
> So that means, if user does not configure it properly and things change
> unexpectedly, then overlayfs should be able to detect it and flag error
> to user space?
>
> > And if it does, should it cause a permanent error
> > in overlayfs or a transient error? If we do not want a permanent error,
> > then ->revalidate() needs to be called to invalidate the overlay dentry. No?
>
> So as of now user space will get -ESTALE and that will get cleared when
> user space retries after corresponding ovl dentry has been dropped from
> cache (either dentry is evicted, cache is cleared forcibly or overlayfs
> is remounted)? If yes, that kind of makes sense. Overlay does not expect
> underlying layers to change and if a change it detected it is flagged
> to user space (and overlayfs does not try to fix it)?
>

I looks like it. I don't really understand why overlayfs shouldn't drop
the dentry on failure to revalidate. Maybe I am missing something.

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Miklos Szeredi]

On Tue, Feb 4, 2020 at 8:11 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Feb 4, 2020 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> > So as of now user space will get -ESTALE and that will get cleared when
> > user space retries after corresponding ovl dentry has been dropped from
> > cache (either dentry is evicted, cache is cleared forcibly or overlayfs
> > is remounted)? If yes, that kind of makes sense. Overlay does not expect
> > underlying layers to change and if a change it detected it is flagged
> > to user space (and overlayfs does not try to fix it)?
> >
>
> I looks like it. I don't really understand why overlayfs shouldn't drop
> the dentry on failure to revalidate. Maybe I am missing something.

I don't remember the exact reason.   Maybe it's just that it makes
little sense to redo the lookup on remote change, but not on local
change...

Note:  I'm not against detection of changing underlying layers and
redoing the lookup in that case.  Maybe we can do it optionally
(because it could be expensive), but first there needs to be a use
case and we seem to lack that.

Thanks,
Miklos

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > revalidation in that case, just as we already do for lower layers.
> > > >
> > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > case.
> > >
> > > Hi Miklos,
> > >
> > > I have couple of very basic questions.
> > >
> > > - So with this change, we will allow NFS to be upper layer also?
> >
> > I haven't tested, but I think it will fail on the d_type test.
>
> But we do not fail mount on no d_type support...
> Besides, I though you were going to add the RENAME_WHITEOUT
> test to avert untested network fs as upper.
>

Pushed strict remote upper check to:
https://github.com/amir73il/linux/commits/ovl-strict-upper

FWIW, overlayfs-next+ovl-strict-upper passes the quick xfstests,
except for overlay/031 - it fails because the RENAME_WHITEOUT check
leaves behind a whiteout in workdir.
I think it it is not worth to cleanup that whiteout leftover and
easier to fix the test.

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > > revalidation in that case, just as we already do for lower layers.
> > > > >
> > > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > > case.
> > > >
> > > > Hi Miklos,
> > > >
> > > > I have couple of very basic questions.
> > > >
> > > > - So with this change, we will allow NFS to be upper layer also?
> > >
> > > I haven't tested, but I think it will fail on the d_type test.
> >
> > But we do not fail mount on no d_type support...
> > Besides, I though you were going to add the RENAME_WHITEOUT
> > test to avert untested network fs as upper.
> >
>
> Pushed strict remote upper check to:
> https://github.com/amir73il/linux/commits/ovl-strict-upper
>
> FWIW, overlayfs-next+ovl-strict-upper passes the quick xfstests,
> except for overlay/031 - it fails because the RENAME_WHITEOUT check
> leaves behind a whiteout in workdir.
> I think it it is not worth to cleanup that whiteout leftover and
> easier to fix the test.

Nevermind. Fixed the whiteout cleanup and re-pushed.

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > > > revalidation in that case, just as we already do for lower layers.
> > > > > >
> > > > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > > > case.
> > > > >
> > > > > Hi Miklos,
> > > > >
> > > > > I have couple of very basic questions.
> > > > >
> > > > > - So with this change, we will allow NFS to be upper layer also?
> > > >
> > > > I haven't tested, but I think it will fail on the d_type test.
> > >
> > > But we do not fail mount on no d_type support...
> > > Besides, I though you were going to add the RENAME_WHITEOUT
> > > test to avert untested network fs as upper.
> > >
> >
> > Pushed strict remote upper check to:
> > https://github.com/amir73il/linux/commits/ovl-strict-upper
> >

Vivek,

Could you please make sure that the code in ovl-strict-upper branch
works as expected for virtio as upper fs?
I have rebased it on latest overlayfs-next merge into current master.

I would very much prefer that the code merged to v5.7-rc1 will be more
restrictive than the current overlayfs-next.

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Vivek Goyal]

On Sat, Mar 14, 2020 at 03:16:28PM +0200, Amir Goldstein wrote:
> On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > > > > revalidation in that case, just as we already do for lower layers.
> > > > > > >
> > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > > > > case.
> > > > > >
> > > > > > Hi Miklos,
> > > > > >
> > > > > > I have couple of very basic questions.
> > > > > >
> > > > > > - So with this change, we will allow NFS to be upper layer also?
> > > > >
> > > > > I haven't tested, but I think it will fail on the d_type test.
> > > >
> > > > But we do not fail mount on no d_type support...
> > > > Besides, I though you were going to add the RENAME_WHITEOUT
> > > > test to avert untested network fs as upper.
> > > >
> > >
> > > Pushed strict remote upper check to:
> > > https://github.com/amir73il/linux/commits/ovl-strict-upper
> > >
> 
> Vivek,
> 
> Could you please make sure that the code in ovl-strict-upper branch
> works as expected for virtio as upper fs?

Hi Amir,

Right now it fails becuase virtiofs doesn't seem to support tmpfile yet.

overlayfs: upper fs does not support tmpfile
overlayfs: upper fs missing required features.

Will have to check what's required to support it.

I also wanted to run either overlay xfstests or unionmount-testsuite. But
none of these seem to give me enough flexibility where I can specify 
that overlayfs needs to be mounted on top of virtiofs.

I feel that atleast for unionmount-testsuite, there should be an
option where we can simply give a target directory and tests run
on that directory and user mounts that directory as needed.

> I have rebased it on latest overlayfs-next merge into current master.
> 
> I would very much prefer that the code merged to v5.7-rc1 will be more
> restrictive than the current overlayfs-next.

In general I agree that if we want to not support some configuration
with remote upper, this is the time to introduce that restriction
otherwise we will later run into backward compatibility issue.

Having said that, tmpfile support for upper sounds like a nice to
have feature. Not sure why to make it mandatory.

Vivek

Re: [PATCH 4/4] ovl: alllow remote upper

[Amir Goldstein]

On Mon, Mar 16, 2020 at 8:15 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Sat, Mar 14, 2020 at 03:16:28PM +0200, Amir Goldstein wrote:
> > On Thu, Feb 20, 2020 at 10:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Thu, Feb 20, 2020 at 9:52 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 4, 2020 at 7:02 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Tue, Feb 4, 2020 at 6:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > On Tue, Feb 4, 2020 at 3:59 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jan 31, 2020 at 12:50:04PM +0100, Miklos Szeredi wrote:
> > > > > > > > No reason to prevent upper layer being a remote filesystem.  Do the
> > > > > > > > revalidation in that case, just as we already do for lower layers.
> > > > > > > >
> > > > > > > > This lets virtiofs be used as upper layer, which appears to be a real use
> > > > > > > > case.
> > > > > > >
> > > > > > > Hi Miklos,
> > > > > > >
> > > > > > > I have couple of very basic questions.
> > > > > > >
> > > > > > > - So with this change, we will allow NFS to be upper layer also?
> > > > > >
> > > > > > I haven't tested, but I think it will fail on the d_type test.
> > > > >
> > > > > But we do not fail mount on no d_type support...
> > > > > Besides, I though you were going to add the RENAME_WHITEOUT
> > > > > test to avert untested network fs as upper.
> > > > >
> > > >
> > > > Pushed strict remote upper check to:
> > > > https://github.com/amir73il/linux/commits/ovl-strict-upper
> > > >
> >
> > Vivek,
> >
> > Could you please make sure that the code in ovl-strict-upper branch
> > works as expected for virtio as upper fs?
>
> Hi Amir,
>
> Right now it fails becuase virtiofs doesn't seem to support tmpfile yet.
>
> overlayfs: upper fs does not support tmpfile
> overlayfs: upper fs missing required features.
>
> Will have to check what's required to support it.
>
> I also wanted to run either overlay xfstests or unionmount-testsuite. But
> none of these seem to give me enough flexibility where I can specify
> that overlayfs needs to be mounted on top of virtiofs.
>
> I feel that atleast for unionmount-testsuite, there should be an
> option where we can simply give a target directory and tests run
> on that directory and user mounts that directory as needed.
>

Need to see how patches look.
Don't want too much configuration complexity, but I agree that some
flexibly is needed.
Maybe the provided target directory should be the upper/work basedir?

> > I have rebased it on latest overlayfs-next merge into current master.
> >
> > I would very much prefer that the code merged to v5.7-rc1 will be more
> > restrictive than the current overlayfs-next.
>
> In general I agree that if we want to not support some configuration
> with remote upper, this is the time to introduce that restriction
> otherwise we will later run into backward compatibility issue.
>
> Having said that, tmpfile support for upper sounds like a nice to
> have feature. Not sure why to make it mandatory.
>

Agreed, I just went automatic on all the warnings.
tmpfile should not be a requirement for upper.
Could you please verify that if dropping the tmpfile strict check,
virtio can be used as upper.

Thanks,
Amir.

Re: [PATCH 4/4] ovl: alllow remote upper

[Vivek Goyal]

On Mon, Mar 16, 2020 at 09:02:32PM +0200, Amir Goldstein wrote:
[..]
> > > Could you please make sure that the code in ovl-strict-upper branch
> > > works as expected for virtio as upper fs?
> >
> > Hi Amir,
> >
> > Right now it fails becuase virtiofs doesn't seem to support tmpfile yet.
> >
> > overlayfs: upper fs does not support tmpfile
> > overlayfs: upper fs missing required features.
> >
> > Will have to check what's required to support it.
> >
> > I also wanted to run either overlay xfstests or unionmount-testsuite. But
> > none of these seem to give me enough flexibility where I can specify
> > that overlayfs needs to be mounted on top of virtiofs.
> >
> > I feel that atleast for unionmount-testsuite, there should be an
> > option where we can simply give a target directory and tests run
> > on that directory and user mounts that directory as needed.
> >
> 
> Need to see how patches look.
> Don't want too much configuration complexity, but I agree that some
> flexibly is needed.
> Maybe the provided target directory should be the upper/work basedir?
> 
> > > I have rebased it on latest overlayfs-next merge into current master.
> > >
> > > I would very much prefer that the code merged to v5.7-rc1 will be more
> > > restrictive than the current overlayfs-next.
> >
> > In general I agree that if we want to not support some configuration
> > with remote upper, this is the time to introduce that restriction
> > otherwise we will later run into backward compatibility issue.
> >
> > Having said that, tmpfile support for upper sounds like a nice to
> > have feature. Not sure why to make it mandatory.
> >
> 
> Agreed, I just went automatic on all the warnings.
> tmpfile should not be a requirement for upper.
> Could you please verify that if dropping the tmpfile strict check,
> virtio can be used as upper.

I dropped tmpfile strict check and now I can mount overlayfs using
virtiofs as upper. Tried few basic file operations and these are
working.

Vivek

Re: unionmount testsuite with upper virtiofs

[Amir Goldstein]

> > I also wanted to run either overlay xfstests or unionmount-testsuite. But
> > none of these seem to give me enough flexibility where I can specify
> > that overlayfs needs to be mounted on top of virtiofs.
> >
> > I feel that atleast for unionmount-testsuite, there should be an
> > option where we can simply give a target directory and tests run
> > on that directory and user mounts that directory as needed.
> >
>
> Need to see how patches look.
> Don't want too much configuration complexity, but I agree that some
> flexibly is needed.
> Maybe the provided target directory should be the upper/work basedir?
>

Vivek,

I was going to see what's the best way to add the needed flexibility,
but then I realized I had already implemented this undocumented
feature.

I have been using this to test overlay over XFS as documented here:
https://github.com/amir73il/overlayfs/wiki/Overlayfs-testing#Setup_overlayfs_mount_over_XFS_with_reflink_support

That's an example of how to configure a custom /base mount for
--samefs to be xfs.
Similar hidden feature exists for configuring a custom /lower and
/upper mounts via fstab, but I don't think I ever tested those, so not
sure if they work as expected. unionmount testsuite will first try to
mount the entry from fstab and fallback to mounting tmpfs.

I admit this a lousy configuration method, but we could make it
official using env vars or something. I also never liked the fact
that unionmount testsuite hard codes the /lower /upper /mnt paths.

The reason we 'need' the instructions how to mount the fs as opposed
to an already mounted dir is that unmounting the underlying fs exposes
dentry/inode reference leaks by overlayfs.
But it is nice to have and xfstests has support for configuring an already
mounted overlayfs for the generic tests.

So if you think that you cannot use the existing hack and that pointing
to an already mounted /upper or mounted overlay is needed, I suggest
that you experiment with patches to make that change.

Thanks,
Amir.

Re: unionmount testsuite with upper virtiofs

[Vivek Goyal]

On Wed, Mar 18, 2020 at 03:36:44PM +0200, Amir Goldstein wrote:
> > > I also wanted to run either overlay xfstests or unionmount-testsuite. But
> > > none of these seem to give me enough flexibility where I can specify
> > > that overlayfs needs to be mounted on top of virtiofs.
> > >
> > > I feel that atleast for unionmount-testsuite, there should be an
> > > option where we can simply give a target directory and tests run
> > > on that directory and user mounts that directory as needed.
> > >
> >
> > Need to see how patches look.
> > Don't want too much configuration complexity, but I agree that some
> > flexibly is needed.
> > Maybe the provided target directory should be the upper/work basedir?
> >
> 
> Vivek,
> 
> I was going to see what's the best way to add the needed flexibility,
> but then I realized I had already implemented this undocumented
> feature.
> 
> I have been using this to test overlay over XFS as documented here:
> https://github.com/amir73il/overlayfs/wiki/Overlayfs-testing#Setup_overlayfs_mount_over_XFS_with_reflink_support
> 
> That's an example of how to configure a custom /base mount for
> --samefs to be xfs.

Hi Amir,

This seems to work for me. Thanks for the idea.

I put following entries in /etc/fstab.

myfs	/mnt/virtiofs-lower_layer	virtiofs	defaults 0 0
/mnt/virtiofs-lower_layer	/base	none	bind 0 0

After that tests seem to start but soon I hit failures. Now its time
to debug the failures one at a time.

Vivek