[PATCH v2] ovl: sharing inode with different whiteout files

[PATCH v2] ovl: sharing inode with different whiteout files

[Chengguang Xu]

Sharing inode with different whiteout files for saving
inode and speeding up delete operation.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/dir.c       | 50 ++++++++++++++++++++++++++++++++--------
 fs/overlayfs/overlayfs.h | 11 +++++++--
 fs/overlayfs/ovl_entry.h |  4 ++++
 fs/overlayfs/readdir.c   |  3 ++-
 fs/overlayfs/super.c     | 10 ++++++++
 fs/overlayfs/util.c      |  3 ++-
 6 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 279009dee366..e48ba7de1bcb 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -62,35 +62,66 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
 }
 
 /* caller holds i_mutex on workdir */
-static struct dentry *ovl_whiteout(struct dentry *workdir)
+static struct dentry *ovl_whiteout(struct ovl_fs *ofs, struct dentry *workdir)
 {
 	int err;
+	bool same = true;
+	bool again = true;
 	struct dentry *whiteout;
 	struct inode *wdir = workdir->d_inode;
 
+	if (ofs->workdir != workdir)
+		same = false;
+retry:
 	whiteout = ovl_lookup_temp(workdir);
 	if (IS_ERR(whiteout))
 		return whiteout;
 
+	if (same && ofs->whiteout) {
+		if (ovl_whiteout_linkable(ofs, workdir, ofs->whiteout)) {
+			err = ovl_do_link(ofs->whiteout, wdir, whiteout);
+			if (!err)
+				return whiteout;
+
+			if (!again)
+				goto out;
+		}
+
+		dput(ofs->whiteout);
+		ofs->whiteout = NULL;
+	}
+
 	err = ovl_do_whiteout(wdir, whiteout);
-	if (err) {
-		dput(whiteout);
-		whiteout = ERR_PTR(err);
+	if (!err) {
+		if (!same || ofs->whiteout_link_max == 1)
+			return whiteout;
+
+		if (!again) {
+			WARN_ON_ONCE(1);
+			return whiteout;
+		}
+
+		dget(whiteout);
+		ofs->whiteout = whiteout;
+		again = false;
+		goto retry;
 	}
 
-	return whiteout;
+out:
+	dput(whiteout);
+	return ERR_PTR(err);
 }
 
 /* Caller must hold i_mutex on both workdir and dir */
-int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
-			     struct dentry *dentry)
+int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
+			     struct inode *dir, struct dentry *dentry)
 {
 	struct inode *wdir = workdir->d_inode;
 	struct dentry *whiteout;
 	int err;
 	int flags = 0;
 
-	whiteout = ovl_whiteout(workdir);
+	whiteout = ovl_whiteout(ofs, workdir);
 	err = PTR_ERR(whiteout);
 	if (IS_ERR(whiteout))
 		return err;
@@ -715,6 +746,7 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
 static int ovl_remove_and_whiteout(struct dentry *dentry,
 				   struct list_head *list)
 {
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct dentry *workdir = ovl_workdir(dentry);
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 	struct dentry *upper;
@@ -748,7 +780,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 		goto out_dput_upper;
 	}
 
-	err = ovl_cleanup_and_whiteout(workdir, d_inode(upperdir), upper);
+	err = ovl_cleanup_and_whiteout(ofs, workdir, d_inode(upperdir), upper);
 	if (err)
 		goto out_d_drop;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e6f3670146ed..cc7bcc3fb916 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -225,6 +225,13 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
 	return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
 }
 
+static inline bool ovl_whiteout_linkable(struct ovl_fs *ofs,
+					 struct dentry *workdir,
+					 struct dentry *dentry)
+{
+	return dentry->d_inode->i_nlink < ofs->whiteout_link_max;
+}
+
 /* util.c */
 int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
@@ -455,8 +462,8 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
-int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
-			     struct dentry *dentry);
+int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
+			     struct inode *dir, struct dentry *dentry);
 struct ovl_cattr {
 	dev_t rdev;
 	umode_t mode;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 5762d802fe01..de5f230b6e6b 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -77,6 +77,10 @@ struct ovl_fs {
 	int xino_mode;
 	/* For allocation of non-persistent inode numbers */
 	atomic_long_t last_ino;
+	/* Whiteout dentry cache */
+	struct dentry *whiteout;
+	/* Whiteout max link count */
+	unsigned int whiteout_link_max;
 };
 
 static inline struct ovl_fs *OVL_FS(struct super_block *sb)
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index e452ff7d583d..1e921115e6aa 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1146,7 +1146,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			 * Whiteout orphan index to block future open by
 			 * handle after overlay nlink dropped to zero.
 			 */
-			err = ovl_cleanup_and_whiteout(indexdir, dir, index);
+			err = ovl_cleanup_and_whiteout(ofs, indexdir, dir,
+						       index);
 		} else {
 			/* Cleanup orphan index entries */
 			err = ovl_cleanup(dir, index);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 732ad5495c92..340c4c05c410 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -26,6 +26,10 @@ struct ovl_dir_cache;
 
 #define OVL_MAX_STACK 500
 
+static unsigned int ovl_whiteout_link_max_def = 60000;
+module_param_named(whiteout_link_max, ovl_whiteout_link_max_def, uint, 0644);
+MODULE_PARM_DESC(whiteout_link_max, "Maximum count of whiteout file link");
+
 static bool ovl_redirect_dir_def = IS_ENABLED(CONFIG_OVERLAY_FS_REDIRECT_DIR);
 module_param_named(redirect_dir, ovl_redirect_dir_def, bool, 0644);
 MODULE_PARM_DESC(redirect_dir,
@@ -219,6 +223,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 	iput(ofs->upperdir_trap);
 	dput(ofs->indexdir);
 	dput(ofs->workdir);
+	dput(ofs->whiteout);
 	if (ofs->workdir_locked)
 		ovl_inuse_unlock(ofs->workbasedir);
 	dput(ofs->workbasedir);
@@ -1762,6 +1767,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 		if (!ofs->workdir)
 			sb->s_flags |= SB_RDONLY;
+		else
+			ofs->whiteout_link_max = min_not_zero(
+					ofs->workdir->d_sb->s_max_links,
+					ovl_whiteout_link_max_def ?
+					ovl_whiteout_link_max_def : 1);
 
 		sb->s_stack_depth = ofs->upper_mnt->mnt_sb->s_stack_depth;
 		sb->s_time_gran = ofs->upper_mnt->mnt_sb->s_time_gran;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 36b60788ee47..18df65ee81a8 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -669,6 +669,7 @@ bool ovl_need_index(struct dentry *dentry)
 /* Caller must hold OVL_I(inode)->lock */
 static void ovl_cleanup_index(struct dentry *dentry)
 {
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
 	struct inode *dir = indexdir->d_inode;
 	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
@@ -707,7 +708,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 		index = NULL;
 	} else if (ovl_index_all(dentry->d_sb)) {
 		/* Whiteout orphan index to block future open by handle */
-		err = ovl_cleanup_and_whiteout(indexdir, dir, index);
+		err = ovl_cleanup_and_whiteout(ofs, indexdir, dir, index);
 	} else {
 		/* Cleanup orphan index entries */
 		err = ovl_cleanup(dir, index);
-- 
2.20.1

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

On Fri, Apr 3, 2020 at 9:45 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Sharing inode with different whiteout files for saving
> inode and speeding up delete operation.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

A few more nits.
Please wait with v3 until I fix my patches, so you can test in top of
them.
Please run the overlay xfstests to test your patch.

I suspect this part of test overlay/031 will fail and will need to fix
the test to expect at most single whiteout 'residue' in work dir:

# try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
# it will not clean up the dir and lead to residue.
rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
ls $workdir/work

And you should start writing a test.
I suggest setting /sys/module/overlay/parameters/whiteout_link_max to 2
(test should _not_run if param does not exist)
removing a bunch of files and verify after unmount that:
- whiteouts have nlink 2
- there is no more than single residue whiteout in work dir

> ---
>  fs/overlayfs/dir.c       | 50 ++++++++++++++++++++++++++++++++--------
>  fs/overlayfs/overlayfs.h | 11 +++++++--
>  fs/overlayfs/ovl_entry.h |  4 ++++
>  fs/overlayfs/readdir.c   |  3 ++-
>  fs/overlayfs/super.c     | 10 ++++++++
>  fs/overlayfs/util.c      |  3 ++-
>  6 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 279009dee366..e48ba7de1bcb 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -62,35 +62,66 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
>  }
>
>  /* caller holds i_mutex on workdir */
> -static struct dentry *ovl_whiteout(struct dentry *workdir)
> +static struct dentry *ovl_whiteout(struct ovl_fs *ofs, struct dentry *workdir)
>  {
>         int err;
> +       bool same = true;

'same' is not the best name, but I expect you won't need it anyway.
I would replace this with:
bool should_link = (ofs->whiteout_link_max > 1);

> +       bool again = true;
>         struct dentry *whiteout;
>         struct inode *wdir = workdir->d_inode;
>
> +       if (ofs->workdir != workdir)
> +               same = false;
> +retry:
>         whiteout = ovl_lookup_temp(workdir);
>         if (IS_ERR(whiteout))
>                 return whiteout;
>
> +       if (same && ofs->whiteout) {
> +               if (ovl_whiteout_linkable(ofs, workdir, ofs->whiteout)) {
> +                       err = ovl_do_link(ofs->whiteout, wdir, whiteout);
> +                       if (!err)
> +                               return whiteout;
> +
> +                       if (!again)
> +                               goto out;
> +               }
> +

We actually need to also cleanup this whiteout:

ovl_cleanup(wdir, ofs->whiteout);

> +               dput(ofs->whiteout);
> +               ofs->whiteout = NULL;
> +       }
> +
>         err = ovl_do_whiteout(wdir, whiteout);
> -       if (err) {
> -               dput(whiteout);
> -               whiteout = ERR_PTR(err);
> +       if (!err) {

save the nesting:
if (err)
    goto out;

> +               if (!same || ofs->whiteout_link_max == 1)
> +                       return whiteout;
> +
> +               if (!again) {
> +                       WARN_ON_ONCE(1);

Definitely no WARN on this case.
We can consider setting whiteout_link_max to 0 and pr_warn()
about auto disabling whiteout linking due to unexpected failure.

> +                       return whiteout;
> +               }
> +
> +               dget(whiteout);
> +               ofs->whiteout = whiteout;

Shorter:
               ofs->whiteout = dget(whiteout);

> +               again = false;
> +               goto retry;
>         }
>
> -       return whiteout;
> +out:
> +       dput(whiteout);
> +       return ERR_PTR(err);
>  }
>
>  /* Caller must hold i_mutex on both workdir and dir */
> -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
> -                            struct dentry *dentry)
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
> +                            struct inode *dir, struct dentry *dentry)
>  {
>         struct inode *wdir = workdir->d_inode;
>         struct dentry *whiteout;
>         int err;
>         int flags = 0;
>
> -       whiteout = ovl_whiteout(workdir);
> +       whiteout = ovl_whiteout(ofs, workdir);
>         err = PTR_ERR(whiteout);
>         if (IS_ERR(whiteout))
>                 return err;
> @@ -715,6 +746,7 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
>  static int ovl_remove_and_whiteout(struct dentry *dentry,
>                                    struct list_head *list)
>  {
> +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>         struct dentry *workdir = ovl_workdir(dentry);
>         struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
>         struct dentry *upper;
> @@ -748,7 +780,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
>                 goto out_dput_upper;
>         }
>
> -       err = ovl_cleanup_and_whiteout(workdir, d_inode(upperdir), upper);
> +       err = ovl_cleanup_and_whiteout(ofs, workdir, d_inode(upperdir), upper);
>         if (err)
>                 goto out_d_drop;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index e6f3670146ed..cc7bcc3fb916 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -225,6 +225,13 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
>         return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
>  }
>
> +static inline bool ovl_whiteout_linkable(struct ovl_fs *ofs,
> +                                        struct dentry *workdir,

workdir unused.

> +                                        struct dentry *dentry)
> +{
> +       return dentry->d_inode->i_nlink < ofs->whiteout_link_max;
> +}
> +
>  /* util.c */
>  int ovl_want_write(struct dentry *dentry);
>  void ovl_drop_write(struct dentry *dentry);
> @@ -455,8 +462,8 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>
>  /* dir.c */
>  extern const struct inode_operations ovl_dir_inode_operations;
> -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
> -                            struct dentry *dentry);
> +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
> +                            struct inode *dir, struct dentry *dentry);
>  struct ovl_cattr {
>         dev_t rdev;
>         umode_t mode;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 5762d802fe01..de5f230b6e6b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -77,6 +77,10 @@ struct ovl_fs {
>         int xino_mode;
>         /* For allocation of non-persistent inode numbers */
>         atomic_long_t last_ino;
> +       /* Whiteout dentry cache */
> +       struct dentry *whiteout;
> +       /* Whiteout max link count */
> +       unsigned int whiteout_link_max;
>  };
>
>  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index e452ff7d583d..1e921115e6aa 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1146,7 +1146,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                          * Whiteout orphan index to block future open by
>                          * handle after overlay nlink dropped to zero.
>                          */
> -                       err = ovl_cleanup_and_whiteout(indexdir, dir, index);
> +                       err = ovl_cleanup_and_whiteout(ofs, indexdir, dir,
> +                                                      index);
>                 } else {
>                         /* Cleanup orphan index entries */
>                         err = ovl_cleanup(dir, index);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 732ad5495c92..340c4c05c410 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -26,6 +26,10 @@ struct ovl_dir_cache;
>
>  #define OVL_MAX_STACK 500
>
> +static unsigned int ovl_whiteout_link_max_def = 60000;
> +module_param_named(whiteout_link_max, ovl_whiteout_link_max_def, uint, 0644);
> +MODULE_PARM_DESC(whiteout_link_max, "Maximum count of whiteout file link");
> +
>  static bool ovl_redirect_dir_def = IS_ENABLED(CONFIG_OVERLAY_FS_REDIRECT_DIR);
>  module_param_named(redirect_dir, ovl_redirect_dir_def, bool, 0644);
>  MODULE_PARM_DESC(redirect_dir,
> @@ -219,6 +223,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>         iput(ofs->upperdir_trap);
>         dput(ofs->indexdir);
>         dput(ofs->workdir);
> +       dput(ofs->whiteout);
>         if (ofs->workdir_locked)
>                 ovl_inuse_unlock(ofs->workbasedir);
>         dput(ofs->workbasedir);
> @@ -1762,6 +1767,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>
>                 if (!ofs->workdir)
>                         sb->s_flags |= SB_RDONLY;
> +               else
> +                       ofs->whiteout_link_max = min_not_zero(
> +                                       ofs->workdir->d_sb->s_max_links,
> +                                       ovl_whiteout_link_max_def ?
> +                                       ovl_whiteout_link_max_def : 1);
>

ovl_whiteout_link_max_def ?: 1);

Thanks,
Amir.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Chengguang Xu]

 ---- 在 星期五, 2020-04-03 17:18:06 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Fri, Apr 3, 2020 at 9:45 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Sharing inode with different whiteout files for saving
 > > inode and speeding up delete operation.
 > >
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > A few more nits.
 > Please wait with v3 until I fix my patches, so you can test in top of
 > them.
 > Please run the overlay xfstests to test your patch.
 > 
 > I suspect this part of test overlay/031 will fail and will need to fix
 > the test to expect at most single whiteout 'residue' in work dir:
 > 
 > # try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
 > # it will not clean up the dir and lead to residue.
 > rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
 > ls $workdir/work

It seems no effect to current test case, I passed all test cases in overlay dir
except nfs_export and mmap related cases.

 
 > 
 > And you should start writing a test.
 > I suggest setting /sys/module/overlay/parameters/whiteout_link_max to 2
 > (test should _not_run if param does not exist)
 > removing a bunch of files and verify after unmount that:
 > - whiteouts have nlink 2
 > - there is no more than single residue whiteout in work dir
 > 

I‘ll do.

 > > ---
 > >  fs/overlayfs/dir.c       | 50 ++++++++++++++++++++++++++++++++--------
 > >  fs/overlayfs/overlayfs.h | 11 +++++++--
 > >  fs/overlayfs/ovl_entry.h |  4 ++++
 > >  fs/overlayfs/readdir.c   |  3 ++-
 > >  fs/overlayfs/super.c     | 10 ++++++++
 > >  fs/overlayfs/util.c      |  3 ++-
 > >  6 files changed, 68 insertions(+), 13 deletions(-)
 > >
 > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
 > > index 279009dee366..e48ba7de1bcb 100644
 > > --- a/fs/overlayfs/dir.c
 > > +++ b/fs/overlayfs/dir.c
 > > @@ -62,35 +62,66 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
 > >  }
 > >
 > >  /* caller holds i_mutex on workdir */
 > > -static struct dentry *ovl_whiteout(struct dentry *workdir)
 > > +static struct dentry *ovl_whiteout(struct ovl_fs *ofs, struct dentry *workdir)
 > >  {
 > >         int err;
 > > +       bool same = true;
 > 
 > 'same' is not the best name, but I expect you won't need it anyway.
 > I would replace this with:
 > bool should_link = (ofs->whiteout_link_max > 1);

I'll fix in V3.

 > 
 > > +       bool again = true;
 > >         struct dentry *whiteout;
 > >         struct inode *wdir = workdir->d_inode;
 > >
 > > +       if (ofs->workdir != workdir)
 > > +               same = false;
 > > +retry:
 > >         whiteout = ovl_lookup_temp(workdir);
 > >         if (IS_ERR(whiteout))
 > >                 return whiteout;
 > >
 > > +       if (same && ofs->whiteout) {
 > > +               if (ovl_whiteout_linkable(ofs, workdir, ofs->whiteout)) {
 > > +                       err = ovl_do_link(ofs->whiteout, wdir, whiteout);
 > > +                       if (!err)
 > > +                               return whiteout;
 > > +
 > > +                       if (!again)
 > > +                               goto out;
 > > +               }
 > > +
 > 
 > We actually need to also cleanup this whiteout:
 > 

I slightly changed the logic here and in this case it will be work just like before.

 > ovl_cleanup(wdir, ofs->whiteout);
 > 
 > > +               dput(ofs->whiteout);
 > > +               ofs->whiteout = NULL;
 > > +       }
 > > +
 > >         err = ovl_do_whiteout(wdir, whiteout);
 > > -       if (err) {
 > > -               dput(whiteout);
 > > -               whiteout = ERR_PTR(err);
 > > +       if (!err) {
 > 
 > save the nesting:
 > if (err)
 >     goto out;
 > 

I'll fix in V3.

 > > +               if (!same || ofs->whiteout_link_max == 1)
 > > +                       return whiteout;
 > > +
 > > +               if (!again) {
 > > +                       WARN_ON_ONCE(1);
 > 
 > Definitely no WARN on this case.
 > We can consider setting whiteout_link_max to 0 and pr_warn()
 > about auto disabling whiteout linking due to unexpected failure.
 > 

I'll fix in V3.

 > > +                       return whiteout;
 > > +               }
 > > +
 > > +               dget(whiteout);
 > > +               ofs->whiteout = whiteout;
 > 
 > Shorter:
 >                ofs->whiteout = dget(whiteout);
 > 

Here, we don't need to grab the dentry again, I think we have already got
the reference in lookup.


 > > +               again = false;
 > > +               goto retry;
 > >         }
 > >
 > > -       return whiteout;
 > > +out:
 > > +       dput(whiteout);
 > > +       return ERR_PTR(err);
 > >  }
 > >
 > >  /* Caller must hold i_mutex on both workdir and dir */
 > > -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 > > -                            struct dentry *dentry)
 > > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
 > > +                            struct inode *dir, struct dentry *dentry)
 > >  {
 > >         struct inode *wdir = workdir->d_inode;
 > >         struct dentry *whiteout;
 > >         int err;
 > >         int flags = 0;
 > >
 > > -       whiteout = ovl_whiteout(workdir);
 > > +       whiteout = ovl_whiteout(ofs, workdir);
 > >         err = PTR_ERR(whiteout);
 > >         if (IS_ERR(whiteout))
 > >                 return err;
 > > @@ -715,6 +746,7 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
 > >  static int ovl_remove_and_whiteout(struct dentry *dentry,
 > >                                    struct list_head *list)
 > >  {
 > > +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 > >         struct dentry *workdir = ovl_workdir(dentry);
 > >         struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 > >         struct dentry *upper;
 > > @@ -748,7 +780,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 > >                 goto out_dput_upper;
 > >         }
 > >
 > > -       err = ovl_cleanup_and_whiteout(workdir, d_inode(upperdir), upper);
 > > +       err = ovl_cleanup_and_whiteout(ofs, workdir, d_inode(upperdir), upper);
 > >         if (err)
 > >                 goto out_d_drop;
 > >
 > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
 > > index e6f3670146ed..cc7bcc3fb916 100644
 > > --- a/fs/overlayfs/overlayfs.h
 > > +++ b/fs/overlayfs/overlayfs.h
 > > @@ -225,6 +225,13 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
 > >         return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
 > >  }
 > >
 > > +static inline bool ovl_whiteout_linkable(struct ovl_fs *ofs,
 > > +                                        struct dentry *workdir,
 > 
 > workdir unused.

I'll fix in V3.

 > 
 > > +                                        struct dentry *dentry)
 > > +{
 > > +       return dentry->d_inode->i_nlink < ofs->whiteout_link_max;
 > > +}
 > > +
 > >  /* util.c */
 > >  int ovl_want_write(struct dentry *dentry);
 > >  void ovl_drop_write(struct dentry *dentry);
 > > @@ -455,8 +462,8 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
 > >
 > >  /* dir.c */
 > >  extern const struct inode_operations ovl_dir_inode_operations;
 > > -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 > > -                            struct dentry *dentry);
 > > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
 > > +                            struct inode *dir, struct dentry *dentry);
 > >  struct ovl_cattr {
 > >         dev_t rdev;
 > >         umode_t mode;
 > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
 > > index 5762d802fe01..de5f230b6e6b 100644
 > > --- a/fs/overlayfs/ovl_entry.h
 > > +++ b/fs/overlayfs/ovl_entry.h
 > > @@ -77,6 +77,10 @@ struct ovl_fs {
 > >         int xino_mode;
 > >         /* For allocation of non-persistent inode numbers */
 > >         atomic_long_t last_ino;
 > > +       /* Whiteout dentry cache */
 > > +       struct dentry *whiteout;
 > > +       /* Whiteout max link count */
 > > +       unsigned int whiteout_link_max;
 > >  };
 > >
 > >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
 > > index e452ff7d583d..1e921115e6aa 100644
 > > --- a/fs/overlayfs/readdir.c
 > > +++ b/fs/overlayfs/readdir.c
 > > @@ -1146,7 +1146,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 > >                          * Whiteout orphan index to block future open by
 > >                          * handle after overlay nlink dropped to zero.
 > >                          */
 > > -                       err = ovl_cleanup_and_whiteout(indexdir, dir, index);
 > > +                       err = ovl_cleanup_and_whiteout(ofs, indexdir, dir,
 > > +                                                      index);
 > >                 } else {
 > >                         /* Cleanup orphan index entries */
 > >                         err = ovl_cleanup(dir, index);
 > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
 > > index 732ad5495c92..340c4c05c410 100644
 > > --- a/fs/overlayfs/super.c
 > > +++ b/fs/overlayfs/super.c
 > > @@ -26,6 +26,10 @@ struct ovl_dir_cache;
 > >
 > >  #define OVL_MAX_STACK 500
 > >
 > > +static unsigned int ovl_whiteout_link_max_def = 60000;
 > > +module_param_named(whiteout_link_max, ovl_whiteout_link_max_def, uint, 0644);
 > > +MODULE_PARM_DESC(whiteout_link_max, "Maximum count of whiteout file link");
 > > +
 > >  static bool ovl_redirect_dir_def = IS_ENABLED(CONFIG_OVERLAY_FS_REDIRECT_DIR);
 > >  module_param_named(redirect_dir, ovl_redirect_dir_def, bool, 0644);
 > >  MODULE_PARM_DESC(redirect_dir,
 > > @@ -219,6 +223,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 > >         iput(ofs->upperdir_trap);
 > >         dput(ofs->indexdir);
 > >         dput(ofs->workdir);
 > > +       dput(ofs->whiteout);
 > >         if (ofs->workdir_locked)
 > >                 ovl_inuse_unlock(ofs->workbasedir);
 > >         dput(ofs->workbasedir);
 > > @@ -1762,6 +1767,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 > >
 > >                 if (!ofs->workdir)
 > >                         sb->s_flags |= SB_RDONLY;
 > > +               else
 > > +                       ofs->whiteout_link_max = min_not_zero(
 > > +                                       ofs->workdir->d_sb->s_max_links,
 > > +                                       ovl_whiteout_link_max_def ?
 > > +                                       ovl_whiteout_link_max_def : 1);
 > >
 > 
 > ovl_whiteout_link_max_def ?: 1);

I'll fix in V3.

Thanks,
cgxu

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

On Tue, Apr 7, 2020 at 12:08 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期五, 2020-04-03 17:18:06 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > On Fri, Apr 3, 2020 at 9:45 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >
>  > > Sharing inode with different whiteout files for saving
>  > > inode and speeding up delete operation.
>  > >
>  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  >
>  > A few more nits.
>  > Please wait with v3 until I fix my patches, so you can test in top of
>  > them.
>  > Please run the overlay xfstests to test your patch.
>  >
>  > I suspect this part of test overlay/031 will fail and will need to fix
>  > the test to expect at most single whiteout 'residue' in work dir:
>  >
>  > # try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
>  > # it will not clean up the dir and lead to residue.
>  > rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
>  > ls $workdir/work
>
> It seems no effect to current test case, I passed all test cases in overlay dir
> except nfs_export and mmap related cases.
>

mmap test fails in baseline.
did nfs_export tests fail with my recent branch (c1fe7dcb3db8)?
because I had a bug that caused nfs_export tests to fail.


...

>  > > +                       return whiteout;
>  > > +               }
>  > > +
>  > > +               dget(whiteout);
>  > > +               ofs->whiteout = whiteout;
>  >
>  > Shorter:
>  >                ofs->whiteout = dget(whiteout);
>  >
>
> Here, we don't need to grab the dentry again, I think we have already got
> the reference in lookup.
>

True.

Thanks,
Amir.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Chengguang Xu]

 ---- 在 星期二, 2020-04-07 17:19:03 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Tue, Apr 7, 2020 at 12:08 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > >  ---- 在 星期五, 2020-04-03 17:18:06 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > On Fri, Apr 3, 2020 at 9:45 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >  > >
 > >  > > Sharing inode with different whiteout files for saving
 > >  > > inode and speeding up delete operation.
 > >  > >
 > >  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > >  >
 > >  > A few more nits.
 > >  > Please wait with v3 until I fix my patches, so you can test in top of
 > >  > them.
 > >  > Please run the overlay xfstests to test your patch.
 > >  >
 > >  > I suspect this part of test overlay/031 will fail and will need to fix
 > >  > the test to expect at most single whiteout 'residue' in work dir:
 > >  >
 > >  > # try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
 > >  > # it will not clean up the dir and lead to residue.
 > >  > rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
 > >  > ls $workdir/work
 > >
 > > It seems no effect to current test case, I passed all test cases in overlay dir
 > > except nfs_export and mmap related cases.
 > >
 > 
 > mmap test fails in baseline.
 > did nfs_export tests fail with my recent branch (c1fe7dcb3db8)?
 > because I had a bug that caused nfs_export tests to fail.
 > 
 > 
 > ...
 > 

Actually, it is "not run" not fail. I'm not very familiar how to run with nfs_export,
is it needing some special options? 

test log below:

overlay/068     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/069     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/070     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/071     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch

overlay/050 4s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/051 3s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/052 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/053 2s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/054 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
overlay/055 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch


Thanks,
cgxu

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

>  > did nfs_export tests fail with my recent branch (c1fe7dcb3db8)?
>  > because I had a bug that caused nfs_export tests to fail.
>  >
>  >
>  > ...
>  >
>
> Actually, it is "not run" not fail. I'm not very familiar how to run with nfs_export,
> is it needing some special options?
>
> test log below:
>
> overlay/068     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/069     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/070     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/071     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>
> overlay/050 4s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/051 3s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/052 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/053 2s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/054 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
> overlay/055 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>
>

It depends which filesystem you have as lower/upper or
which mount options you used for the xfstests setup.
dmesg should give you a clue for why nfs_export could not be
enabled on overlayfs.

Thanks,
Amir.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Chengguang Xu]

 ---- 在 星期三, 2020-04-08 01:12:59 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > did nfs_export tests fail with my recent branch (c1fe7dcb3db8)?
 > >  > because I had a bug that caused nfs_export tests to fail.
 > >  >
 > >  >
 > >  > ...
 > >  >
 > >
 > > Actually, it is "not run" not fail. I'm not very familiar how to run with nfs_export,
 > > is it needing some special options?
 > >
 > > test log below:
 > >
 > > overlay/068     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/069     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/070     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/071     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > >
 > > overlay/050 4s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/051 3s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/052 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/053 2s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/054 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > > overlay/055 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
 > >
 > >
 > 
 > It depends which filesystem you have as lower/upper or
 > which mount options you used for the xfstests setup.
 > dmesg should give you a clue for why nfs_export could not be
 > enabled on overlayfs.
 > 

nfs_export and metacopy are incompatible. 
I tested "workdir" branch(latest commit is commit c1fe7dcb3db8ed8e84986eec07e0b302ee3b83de)
in your git tree and found three more fails.

----------------------------------

overlay/029 1s ... - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad)
    --- tests/overlay/029.out   2019-11-07 09:05:18.876796433 +0800
    +++ /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad        2020-04-08 09:55:07.462699895 +0800
    @@ -1,5 +1,9 @@
     QA output created by 029
     foo
    -bar
    -foo
    -bar
    +mount: /tmp/8751/mnt: mount(2) system call failed: Stale file handle.
    +cat: /tmp/8751/mnt/bar: No such file or directory
    ...
    (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/029.out /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad'  to see the entire diff)

overlay/070     [failed, exit status 1]- output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad)
    --- tests/overlay/070.out   2020-04-07 09:16:59.102568756 +0800
    +++ /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad        2020-04-08 09:55:38.580311600 +0800
    @@ -1,2 +1,26 @@
     QA output created by 070
    -Silence is golden
    +umount: /mnt/scratch: target is busy.
    +rm: cannot remove '/mnt/scratch/ovl-mnt': Device or resource busy
    +losetup: /mnt/scratch/ovl-lower/img: failed to set up loop device: No such file or directory
    +cp: target '/mnt/scratch/ovl-lower/lowertestdir/blkdev' is not a directory
    +cp: target '/mnt/scratch/ovl-upper/uppertestdir/blkdev' is not a directory
    ...
    (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/070.out /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad'  to see the entire diff)

overlay/071     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad)
    --- tests/overlay/071.out   2020-04-07 09:16:59.102568756 +0800
    +++ /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad        2020-04-08 09:55:39.899295141 +0800
    @@ -1,2 +1,6 @@
     QA output created by 071
    +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
    +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
    +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
    +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
     Silence is golden


Thanks,
cgxu.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

On Wed, Apr 8, 2020 at 5:11 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期三, 2020-04-08 01:12:59 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > >  > did nfs_export tests fail with my recent branch (c1fe7dcb3db8)?
>  > >  > because I had a bug that caused nfs_export tests to fail.
>  > >  >
>  > >  >
>  > >  > ...
>  > >  >
>  > >
>  > > Actually, it is "not run" not fail. I'm not very familiar how to run with nfs_export,
>  > > is it needing some special options?
>  > >
>  > > test log below:
>  > >
>  > > overlay/068     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/069     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/070     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/071     [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > >
>  > > overlay/050 4s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/051 3s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/052 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/053 2s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/054 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > > overlay/055 1s ... [not run] overlay feature 'nfs_export' cannot be enabled on /mnt/scratch
>  > >
>  > >
>  >
>  > It depends which filesystem you have as lower/upper or
>  > which mount options you used for the xfstests setup.
>  > dmesg should give you a clue for why nfs_export could not be
>  > enabled on overlayfs.
>  >
>
> nfs_export and metacopy are incompatible.
> I tested "workdir" branch(latest commit is commit c1fe7dcb3db8ed8e84986eec07e0b302ee3b83de)
> in your git tree and found three more fails.

I figured that might me it.
Please share more configuration details of your setup.
How is metacopy enabled on your system?
How are you running xfstests? Can you share the config file?
Details of the underlying filesystems.

>
> ----------------------------------
>
> overlay/029 1s ... - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad)
>     --- tests/overlay/029.out   2019-11-07 09:05:18.876796433 +0800
>     +++ /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad        2020-04-08 09:55:07.462699895 +0800
>     @@ -1,5 +1,9 @@
>      QA output created by 029
>      foo
>     -bar
>     -foo
>     -bar
>     +mount: /tmp/8751/mnt: mount(2) system call failed: Stale file handle.

The reason for this error should be in dmesg.

>     +cat: /tmp/8751/mnt/bar: No such file or directory
>     ...
>     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/029.out /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad'  to see the entire diff)
>
> overlay/070     [failed, exit status 1]- output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad)
>     --- tests/overlay/070.out   2020-04-07 09:16:59.102568756 +0800
>     +++ /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad        2020-04-08 09:55:38.580311600 +0800
>     @@ -1,2 +1,26 @@
>      QA output created by 070
>     -Silence is golden
>     +umount: /mnt/scratch: target is busy.

This failure has do to with somthing that happened before the test.
Its trails should be in dmesg.

>     +rm: cannot remove '/mnt/scratch/ovl-mnt': Device or resource busy
>     +losetup: /mnt/scratch/ovl-lower/img: failed to set up loop device: No such file or directory
>     +cp: target '/mnt/scratch/ovl-lower/lowertestdir/blkdev' is not a directory
>     +cp: target '/mnt/scratch/ovl-upper/uppertestdir/blkdev' is not a directory
>     ...
>     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/070.out /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad'  to see the entire diff)
>
> overlay/071     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad)
>     --- tests/overlay/071.out   2020-04-07 09:16:59.102568756 +0800
>     +++ /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad        2020-04-08 09:55:39.899295141 +0800
>     @@ -1,2 +1,6 @@
>      QA output created by 071
>     +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
>     +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
>     +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
>     +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
>      Silence is golden
>

What do the full log details say?
again, its probably a result of some previous failure.

FWIW, I ran:
kvm-xfstests -c overlay/large -g overlay/quick -m metacopy=on

And there were no errors. The nfs_exports tests did [not run]

Thanks,
Amir.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

>  > > nfs_export and metacopy are incompatible.
>  > > I tested "workdir" branch(latest commit is commit c1fe7dcb3db8ed8e84986eec07e0b302ee3b83de)
>  > > in your git tree and found three more fails.
>  >
>  > I figured that might me it.
>  > Please share more configuration details of your setup.
>  > How is metacopy enabled on your system?
>
> In order to test nfs_export, I disabled metacopy and enabled nfs_export in kernel config.

Ah right. There is this logic in ovl_parse_opt() where an explicit
metacopy or redirect_dir mount options win a conflict against the kernel config
default, but there is no such logic fir metacopy and index, because it
was harder to implement.

For the purpose of these tests, they just need to explicitly disable
metacopy in mount options. I will send a fix.

>
>  > How are you running xfstests? Can you share the config file?
>
> in xfstests-dev directory, run ./check -overlay overlay/*

FYI, -g overlay/auto will skip the "known issues" (i.e. mmap test)
and -g overlay/quick will skip the long stress test (overlay/019)

I also recommend running https://github.com/amir73il/unionmount-testsuite
if you want better test coverage.

...
>  >
>  > >
>  > > ----------------------------------
>  > >
>  > > overlay/029 1s ... - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad)
>  > >     --- tests/overlay/029.out   2019-11-07 09:05:18.876796433 +0800
>  > >     +++ /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad        2020-04-08 09:55:07.462699895 +0800
>  > >     @@ -1,5 +1,9 @@
>  > >      QA output created by 029
>  > >      foo
>  > >     -bar
>  > >     -foo
>  > >     -bar
>  > >     +mount: /tmp/8751/mnt: mount(2) system call failed: Stale file handle.
>  >
>  > The reason for this error should be in dmesg.
>
> I checked test log/case and the logic in the source code,
> I think the test failed at ovl_get_indexdir() -> ovl_verify_origin() during mount and this behaviour is just by design.
> so we should skip this test case when nfs_export is enabled.
>

OK. I was able to reproduce, but no need to skip the test
it is easy to fix it. I will post a fix.

>
>  >
>  > >     +cat: /tmp/8751/mnt/bar: No such file or directory
>  > >     ...
>  > >     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/029.out /home/cgxu/git/xfstests-dev/results//overlay/029.out.bad'  to see the entire diff)
>  > >
>  > > overlay/070     [failed, exit status 1]- output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad)
>  > >     --- tests/overlay/070.out   2020-04-07 09:16:59.102568756 +0800
>  > >     +++ /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad        2020-04-08 09:55:38.580311600 +0800
>  > >     @@ -1,2 +1,26 @@
>  > >      QA output created by 070
>  > >     -Silence is golden
>  > >     +umount: /mnt/scratch: target is busy.
>  >
>  > This failure has do to with somthing that happened before the test.
>  > Its trails should be in dmesg.
>  >
>  > >     +rm: cannot remove '/mnt/scratch/ovl-mnt': Device or resource busy
>  > >     +losetup: /mnt/scratch/ovl-lower/img: failed to set up loop device: No such file or directory
>  > >     +cp: target '/mnt/scratch/ovl-lower/lowertestdir/blkdev' is not a directory
>  > >     +cp: target '/mnt/scratch/ovl-upper/uppertestdir/blkdev' is not a directory
>  > >     ...
>  > >     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/070.out /home/cgxu/git/xfstests-dev/results//overlay/070.out.bad'  to see the entire diff)
>  > >
>  > > overlay/071     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad)
>  > >     --- tests/overlay/071.out   2020-04-07 09:16:59.102568756 +0800
>  > >     +++ /home/cgxu/git/xfstests-dev/results//overlay/071.out.bad        2020-04-08 09:55:39.899295141 +0800
>  > >     @@ -1,2 +1,6 @@
>  > >      QA output created by 071
>  > >     +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
>  > >     +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
>  > >     +_overlay_check_fs: overlayfs on /mnt/scratch/ovl-mnt,/ovl-upper.2,/ovl-work.2 is inconsistent
>  > >     +(see /home/cgxu/git/xfstests-dev/results//overlay/071.full for details)
>  > >      Silence is golden
>  > >
>  >
>  > What do the full log details say?
>  > again, its probably a result of some previous failure.
>
> It sill failed when I just ran one test case.
> I haven't got enough time to analyse test logic and full log, so I put all logs in attachment.
> Please let me know if other logs are needed.

Thanks for taking the time to report all those failures.
You must be one of few developers to actually use fsck.overlayfs...

You need this fix for fsck.overlayfs:
https://github.com/hisilicon/overlayfs-progs/pull/1

Sorry, I forgot I was carrying this patch on my setup.

Zhangyi,

Any chance of merging my fix?

Thanks,
Amir.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[zhangyi (F)]

Hi, Amir

On 2020/4/9 19:21, Amir Goldstein wrote:
[...]
> 
> Thanks for taking the time to report all those failures.
> You must be one of few developers to actually use fsck.overlayfs...
> 
> You need this fix for fsck.overlayfs:
> https://github.com/hisilicon/overlayfs-progs/pull/1
> 
> Sorry, I forgot I was carrying this patch on my setup.
> 
> Zhangyi,
> 
> Any chance of merging my fix?
> 

Thanks for the patch, I think we'd better to remove the FS_LAYER_XATTR flag
for a nested overlayfs layer, so we could skip checking OVL_XATTR_PREFIX
xattrs when scanning the layer. Something like this,

+	/* A nested overlayfs does not support OVL_XATTR_PREFIX xattr */
+	if (statfs.f_type == OVERLAYFS_SUPER_MAGIC)
+		return 0;

I will modify this and merge your patch.

Thanks,
Yi.

Re: [PATCH v2] ovl: sharing inode with different whiteout files

[Amir Goldstein]

On Thu, Apr 9, 2020 at 4:25 PM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> Hi, Amir
>
> On 2020/4/9 19:21, Amir Goldstein wrote:
> [...]
> >
> > Thanks for taking the time to report all those failures.
> > You must be one of few developers to actually use fsck.overlayfs...
> >
> > You need this fix for fsck.overlayfs:
> > https://github.com/hisilicon/overlayfs-progs/pull/1
> >
> > Sorry, I forgot I was carrying this patch on my setup.
> >
> > Zhangyi,
> >
> > Any chance of merging my fix?
> >
>
> Thanks for the patch, I think we'd better to remove the FS_LAYER_XATTR flag

I have no idea why I set this flag. It was a long time ago...
It really doesn't make sense and does not match what commit message says.

> for a nested overlayfs layer, so we could skip checking OVL_XATTR_PREFIX
> xattrs when scanning the layer. Something like this,
>
> +       /* A nested overlayfs does not support OVL_XATTR_PREFIX xattr */
> +       if (statfs.f_type == OVERLAYFS_SUPER_MAGIC)
> +               return 0;
>
> I will modify this and merge your patch.
>

Thanks,
Amir.