[PATCH 0/2] Performance improvements for ovl_indexdir_cleanup()

[PATCH 0/2] Performance improvements for ovl_indexdir_cleanup()

[Amir Goldstein]

Miklos,

I ran into an incident of very large index dir which took considerable
amount of time to mount the indexed overlay (~30 minutes).
The index dir had millions of entries and I do not think that the use
case that caused this is typical.

The following two patches are based on perf top analysis of this
incident.  I do not have access to the data set that caused the
very long mount time, but I tested the desired CPU usage improvements
on a smaller scale data set.

It is hard to say if this extreme case of very large index dir is
common enough to be worth any attention, so I did not tag the fixes
for stable and I don't think it is urgent to apply them.

Unfortunattely, the investigation of the incident was not timed
optimally w.r.t. to the current merge window.
Nevertheless, the changes are quite trivial, so you may want to consider
them either for -rc or for next release.

Thanks,
Amir.

Amir Goldstein (2):
  ovl: do not reconnect upper index records in ovl_indexdir_cleanup()
  ovl: use plain list filler in indexdir and workdir cleanup

 fs/overlayfs/export.c    |  4 ++--
 fs/overlayfs/namei.c     |  7 ++++---
 fs/overlayfs/overlayfs.h |  3 ++-
 fs/overlayfs/readdir.c   | 12 ++----------
 4 files changed, 10 insertions(+), 16 deletions(-)

-- 
2.25.1

[PATCH 1/2] ovl: do not reconnect upper index records in ovl_indexdir_cleanup()

[Amir Goldstein]

ovl_indexdir_cleanup() is called on mount of overayfs with nfs_export
feature to cleanup stale index records for lower and upper files that
have been deleted while overlayfs was offline.

This has the side effect (good or bad) of pre populating inode cache
with all the copied up upper inodes, while verifying the index entries.

For copied up directories, the upper file handles are decoded to
conncted upper dentries.  This has the even bigger side effect of
reading the content of all the parent upper directories which may take
significantly more time and IO than just reading the upper inodes.

Do not request connceted upper dentries for verifying upper directory
index entries, because we have no use for the connected dentry.

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

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index be866a3a92aa..a25bb3453dde 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -463,7 +463,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
 
 	/* Get connected upper overlay dir from index */
 	if (index) {
-		struct dentry *upper = ovl_index_upper(ofs, index);
+		struct dentry *upper = ovl_index_upper(ofs, index, true);
 
 		dput(index);
 		if (IS_ERR_OR_NULL(upper))
@@ -739,7 +739,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 
 	/* Then try to get a connected upper dir by index */
 	if (index && d_is_dir(index)) {
-		struct dentry *upper = ovl_index_upper(ofs, index);
+		struct dentry *upper = ovl_index_upper(ofs, index, true);
 
 		err = PTR_ERR(upper);
 		if (IS_ERR_OR_NULL(upper))
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 69dc577974f8..3edd4887cd62 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -487,7 +487,8 @@ int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
 }
 
 /* Get upper dentry from index */
-struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
+struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
+			       bool connected)
 {
 	struct ovl_fh *fh;
 	struct dentry *upper;
@@ -499,7 +500,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
 	if (IS_ERR_OR_NULL(fh))
 		return ERR_CAST(fh);
 
-	upper = ovl_decode_real_fh(ofs, fh, ovl_upper_mnt(ofs), true);
+	upper = ovl_decode_real_fh(ofs, fh, ovl_upper_mnt(ofs), connected);
 	kfree(fh);
 
 	if (IS_ERR_OR_NULL(upper))
@@ -572,7 +573,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	 * directly from the index dentry, but for dir index we first need to
 	 * decode the upper directory.
 	 */
-	upper = ovl_index_upper(ofs, index);
+	upper = ovl_index_upper(ofs, index, false);
 	if (IS_ERR_OR_NULL(upper)) {
 		err = PTR_ERR(upper);
 		/*
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index a0e450313ea4..222883632df0 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -523,7 +523,8 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 int ovl_verify_set_fh(struct ovl_fs *ofs, struct dentry *dentry,
 		      enum ovl_xattr ox, struct dentry *real, bool is_upper,
 		      bool set);
-struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index);
+struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index,
+			       bool connected);
 int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
 int ovl_get_index_name(struct ovl_fs *ofs, struct dentry *origin,
 		       struct qstr *name);
-- 
2.25.1

[PATCH 2/2] ovl: use plain list filler in indexdir and workdir cleanup

[Amir Goldstein]

Those two cleanup routines are using the helper ovl_dir_read() with
the merge dir filler, which populates an rb tree, that is never used.

The index dir entry names all have a long (42 bytes) constant prefix,
so it is not surprising that perf top has demostrated high CPU usage
by rb tree population during cleanup of a large index dir:

      - 9.53% ovl_fill_merge
         - 78.41% ovl_cache_entry_find_link.constprop.27
            + 72.11% strncmp

Use the plain list filler that does not populate the unneeded rb tree.

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

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 78f62cc1797b..f11324b46d23 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1071,14 +1071,10 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, struct path *path,
 	int err;
 	struct inode *dir = path->dentry->d_inode;
 	LIST_HEAD(list);
-	struct rb_root root = RB_ROOT;
 	struct ovl_cache_entry *p;
 	struct ovl_readdir_data rdd = {
-		.ctx.actor = ovl_fill_merge,
-		.dentry = NULL,
+		.ctx.actor = ovl_fill_plain,
 		.list = &list,
-		.root = &root,
-		.is_lowest = false,
 	};
 	bool incompat = false;
 
@@ -1159,14 +1155,10 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 	struct inode *dir = indexdir->d_inode;
 	struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = indexdir };
 	LIST_HEAD(list);
-	struct rb_root root = RB_ROOT;
 	struct ovl_cache_entry *p;
 	struct ovl_readdir_data rdd = {
-		.ctx.actor = ovl_fill_merge,
-		.dentry = NULL,
+		.ctx.actor = ovl_fill_plain,
 		.list = &list,
-		.root = &root,
-		.is_lowest = false,
 	};
 
 	err = ovl_dir_read(&path, &rdd);
-- 
2.25.1

Re: [PATCH 0/2] Performance improvements for ovl_indexdir_cleanup()

[Miklos Szeredi]

On Tue, 4 Oct 2022 at 12:34, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> I ran into an incident of very large index dir which took considerable
> amount of time to mount the indexed overlay (~30 minutes).
> The index dir had millions of entries and I do not think that the use
> case that caused this is typical.
>
> The following two patches are based on perf top analysis of this
> incident.  I do not have access to the data set that caused the
> very long mount time, but I tested the desired CPU usage improvements
> on a smaller scale data set.
>
> It is hard to say if this extreme case of very large index dir is
> common enough to be worth any attention, so I did not tag the fixes
> for stable and I don't think it is urgent to apply them.
>
> Unfortunattely, the investigation of the incident was not timed
> optimally w.r.t. to the current merge window.
> Nevertheless, the changes are quite trivial, so you may want to consider
> them either for -rc or for next release.

Looks good.  Pushed to #overlayfs-next.

Thanks,
Miklos