[PATCH v2 0/3] Overlayfs file handle decode optimization

[PATCH v2 0/3] Overlayfs file handle decode optimization

[Amir Goldstein]

Miklos,

This is the 2nd revision of patch set to optimize file handle decode.
It is available on branch ovl-nfs-export on my github [1].

Patch 2 introduces a semantic change to exportfs_decode_fh() to allow
callers to opt-out of reconnecting the dentry. I like this minimal
semantic chance, but if you think I should factor-out a "_noreconnect"
variant of exportfs_decode_fh(), I can do that. Also, in
ovl_lower_fh_to_d(), if we get a disconnected directory dentry and we
end up needing a connected dentry, we drop the reference to the
disconnected dentry and call exportfs_decode_fh() again without
opting-out of reconnect. I could also export exportfs_reconnect_path()
for that case, but seems unnecessary to do that as the disconnected
dentry will likely still be in cache anyway.

To verify optimization, I ran all the overlay/exportfs tests with trace
prints in index lookup and reconnect functions to verify that reconnect
is not called when decoding a lower dir file handle of a deleted dir.
Detailed analysis on this test results are in commit message of the last
patch.

Thanks,
Amir.

Changes since v1:
- Discard hash inodes by file handle
- Lookup in inode cache before lookup in index dir
- Lookup underlying fs inode cache without reconnect lower dir

[1] https://github.com/amir73il/linux/commits/ovl-nfs-export

Amir Goldstein (3):
  ovl: disambiguate ovl_encode_fh()
  ovl: do not try to reconnect a disconnected origin dentry
  ovl: lookup in inode cache first when decoding lower file handle

 fs/exportfs/expfs.c      |  9 +++++++
 fs/overlayfs/copy_up.c   |  6 ++---
 fs/overlayfs/export.c    | 70 +++++++++++++++++++++++++++---------------------
 fs/overlayfs/namei.c     | 20 +++++++-------
 fs/overlayfs/overlayfs.h |  7 ++---
 5 files changed, 66 insertions(+), 46 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ovl: disambiguate ovl_encode_fh()

[Amir Goldstein]

Rename ovl_encode_fh() to ovl_encode_real_fh() to differentiate from the
exportfs function ovl_encode_inode_fh() and change the latter to
ovl_encode_fh() to match the exportfs method name.

Rename ovl_decode_fh() to ovl_decode_real_fh() for consistency.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d855f508fa20..8bede0742619 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -232,7 +232,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
-struct ovl_fh *ovl_encode_fh(struct dentry *real, bool is_upper)
+struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper)
 {
 	struct ovl_fh *fh;
 	int fh_type, fh_len, dwords;
@@ -300,7 +300,7 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 	 * up and a pure upper inode.
 	 */
 	if (ovl_can_decode_fh(lower->d_sb)) {
-		fh = ovl_encode_fh(lower, false);
+		fh = ovl_encode_real_fh(lower, false);
 		if (IS_ERR(fh))
 			return PTR_ERR(fh);
 	}
@@ -321,7 +321,7 @@ static int ovl_set_upper_fh(struct dentry *upper, struct dentry *index)
 	const struct ovl_fh *fh;
 	int err;
 
-	fh = ovl_encode_fh(upper, true);
+	fh = ovl_encode_real_fh(upper, true);
 	if (IS_ERR(fh))
 		return PTR_ERR(fh);
 
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 87bd4148f4fb..e688cf019460 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -228,8 +228,8 @@ static int ovl_d_to_fh(struct dentry *dentry, char *buf, int buflen)
 		goto fail;
 
 	/* Encode an upper or lower file handle */
-	fh = ovl_encode_fh(enc_lower ? ovl_dentry_lower(dentry) :
-				       ovl_dentry_upper(dentry), !enc_lower);
+	fh = ovl_encode_real_fh(enc_lower ? ovl_dentry_lower(dentry) :
+				ovl_dentry_upper(dentry), !enc_lower);
 	err = PTR_ERR(fh);
 	if (IS_ERR(fh))
 		goto fail;
@@ -267,8 +267,8 @@ static int ovl_dentry_to_fh(struct dentry *dentry, u32 *fid, int *max_len)
 	return OVL_FILEID;
 }
 
-static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len,
-			       struct inode *parent)
+static int ovl_encode_fh(struct inode *inode, u32 *fid, int *max_len,
+			 struct inode *parent)
 {
 	struct dentry *dentry;
 	int type;
@@ -685,7 +685,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
 	if (!ofs->upper_mnt)
 		return ERR_PTR(-EACCES);
 
-	upper = ovl_decode_fh(fh, ofs->upper_mnt);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt);
 	if (IS_ERR_OR_NULL(upper))
 		return upper;
 
@@ -829,7 +829,7 @@ static struct dentry *ovl_get_parent(struct dentry *dentry)
 }
 
 const struct export_operations ovl_export_operations = {
-	.encode_fh	= ovl_encode_inode_fh,
+	.encode_fh	= ovl_encode_fh,
 	.fh_to_dentry	= ovl_fh_to_dentry,
 	.fh_to_parent	= ovl_fh_to_parent,
 	.get_name	= ovl_get_name,
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 87d8bf2c09c6..6e31a4f9e47f 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -179,7 +179,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 	goto out;
 }
 
-struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
+struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 {
 	struct dentry *real;
 	int bytes;
@@ -327,7 +327,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh,
 	int i;
 
 	for (i = 0; i < ofs->numlower; i++) {
-		origin = ovl_decode_fh(fh, ofs->lower_layers[i].mnt);
+		origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt);
 		if (origin)
 			break;
 	}
@@ -425,7 +425,7 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name,
 	struct ovl_fh *fh;
 	int err;
 
-	fh = ovl_encode_fh(real, is_upper);
+	fh = ovl_encode_real_fh(real, is_upper);
 	err = PTR_ERR(fh);
 	if (IS_ERR(fh))
 		goto fail;
@@ -461,7 +461,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_fh(fh, ofs->upper_mnt);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt);
 	kfree(fh);
 
 	if (IS_ERR_OR_NULL(upper))
@@ -629,7 +629,7 @@ int ovl_get_index_name(struct dentry *origin, struct qstr *name)
 	struct ovl_fh *fh;
 	int err;
 
-	fh = ovl_encode_fh(origin, false);
+	fh = ovl_encode_real_fh(origin, false);
 	if (IS_ERR(fh))
 		return PTR_ERR(fh);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 225ff1171147..dd6c10e5a7db 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -266,7 +266,7 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 
 /* namei.c */
 int ovl_check_fh_len(struct ovl_fh *fh, int fh_len);
-struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt);
+struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt);
 int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh,
 			struct dentry *upperdentry, struct ovl_path **stackp);
 int ovl_verify_set_fh(struct dentry *dentry, const char *name,
@@ -361,7 +361,7 @@ int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_copy_xattr(struct dentry *old, struct dentry *new);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
-struct ovl_fh *ovl_encode_fh(struct dentry *real, bool is_upper);
+struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
 int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 		   struct dentry *upper);
 
-- 
2.7.4

[PATCH v2 2/3] ovl: do not try to reconnect a disconnected origin dentry

[Amir Goldstein]

On lookup of non directory, we try to decode the origin file handle
stored in upper inode. The origin file handle is supposed to be decoded
to a disconnected non-dir dentry, which is fine, because we only need
the lower inode of a copy up origin.

However, if the origin file handle somehow turns out to be a directory
we pay the expensive cost of reconnecting the directory dentry, only to
get a mismatch file type and drop the dentry.

Optimize this case by explicitly opting out of reconnecting the dentry.
Opting-out of reconnect is done by passing a NULL acceptable callback
to exportfs_decode_fh().

While the case described above is a strange corner case that does not
really need to be optimized, the API added for this optimization will
be used by a following patch to optimize a more common case of decoding
an overlayfs file handle.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/exportfs/expfs.c      |  9 +++++++++
 fs/overlayfs/export.c    |  4 ++--
 fs/overlayfs/namei.c     | 16 +++++++++-------
 fs/overlayfs/overlayfs.h |  5 +++--
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 329a5d103846..645158dc33f1 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -435,6 +435,15 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 	if (IS_ERR_OR_NULL(result))
 		return ERR_PTR(-ESTALE);
 
+	/*
+	 * If no acceptance criteria was specified by caller, a disconnected
+	 * dentry is also accepatable. Callers may use this mode to query if
+	 * file handle is stale or to get a reference to an inode without
+	 * risking the high overhead caused by directory reconnect.
+	 */
+	if (!acceptable)
+		return result;
+
 	if (d_is_dir(result)) {
 		/*
 		 * This request is for a directory.
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index e688cf019460..15411350a7a1 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -685,7 +685,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
 	if (!ofs->upper_mnt)
 		return ERR_PTR(-EACCES);
 
-	upper = ovl_decode_real_fh(fh, ofs->upper_mnt);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true);
 	if (IS_ERR_OR_NULL(upper))
 		return upper;
 
@@ -735,7 +735,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 	}
 
 	/* Then lookup origin by fh */
-	err = ovl_check_origin_fh(ofs, fh, NULL, &stack);
+	err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);
 	if (err) {
 		goto out_err;
 	} else if (index) {
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6e31a4f9e47f..e62e06c735ef 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -179,7 +179,8 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 	goto out;
 }
 
-struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt)
+struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
+				  bool connected)
 {
 	struct dentry *real;
 	int bytes;
@@ -194,7 +195,7 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 	bytes = (fh->len - offsetof(struct ovl_fh, fid));
 	real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
 				  bytes >> 2, (int)fh->type,
-				  ovl_acceptable, mnt);
+				  connected ? ovl_acceptable : NULL, mnt);
 	if (IS_ERR(real)) {
 		/*
 		 * Treat stale file handle to lower file as "origin unknown".
@@ -320,14 +321,15 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 }
 
 
-int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh,
+int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 			struct dentry *upperdentry, struct ovl_path **stackp)
 {
 	struct dentry *origin = NULL;
 	int i;
 
 	for (i = 0; i < ofs->numlower; i++) {
-		origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt);
+		origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
+					    connected);
 		if (origin)
 			break;
 	}
@@ -371,7 +373,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
 	if (IS_ERR_OR_NULL(fh))
 		return PTR_ERR(fh);
 
-	err = ovl_check_origin_fh(ofs, fh, upperdentry, stackp);
+	err = ovl_check_origin_fh(ofs, fh, false, upperdentry, stackp);
 	kfree(fh);
 
 	if (err) {
@@ -461,7 +463,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(fh, ofs->upper_mnt);
+	upper = ovl_decode_real_fh(fh, ofs->upper_mnt, true);
 	kfree(fh);
 
 	if (IS_ERR_OR_NULL(upper))
@@ -568,7 +570,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 
 	/* Check if non-dir index is orphan and don't warn before cleaning it */
 	if (!d_is_dir(index) && d_inode(index)->i_nlink == 1) {
-		err = ovl_check_origin_fh(ofs, fh, index, &stack);
+		err = ovl_check_origin_fh(ofs, fh, false, index, &stack);
 		if (err)
 			goto fail;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index dd6c10e5a7db..b51613b355c5 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -266,8 +266,9 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 
 /* namei.c */
 int ovl_check_fh_len(struct ovl_fh *fh, int fh_len);
-struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt);
-int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh,
+struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
+				  bool connected);
+int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 			struct dentry *upperdentry, struct ovl_path **stackp);
 int ovl_verify_set_fh(struct dentry *dentry, const char *name,
 		      struct dentry *real, bool is_upper, bool set);
-- 
2.7.4

[PATCH v2 3/3] ovl: lookup in inode cache first when decoding lower file handle

[Amir Goldstein]

When decoding a lower file handle, we need to check if lower file was
copied up and indexed and if it has a whiteout index, we need to check
if this is an unlinked but open non-dir before returning -ESTALE.

To find out if this is an unlinked but open non-dir we need to lookup
an overlay inode in inode cache by lower inode and that requires decoding
the lower file handle before looking in inode cache.

Before this change, if the lower inode turned out to be a directory, we
may have paid an expensive cost to reconnect that lower directory for
nothing.

After this change, we start by decoding a disconnected lower dentry and
using the lower inode for looking up an overlay inode in inode cache.
If we find overlay inode and dentry in cache, we avoid the index lookup
overhead. If we don't find an overlay inode and dentry in cache, then we
only need to decode a connected lower dentry in case the lower dentry is
a non-indexed directory.

The xfstests group overlay/exportfs tests decoding overlayfs file
handles after drop_caches with different states of the file at encode
and decode time. Overall the tests in the group call ovl_lower_fh_to_d()
89 times to decode a lower file handle.

Before this change, the tests called ovl_get_index_fh() 75 times and
reconnect_one() 61 times.
After this change, the tests call ovl_get_index_fh() 70 times and
reconnect_one() 59 times. The 2 cases where reconnect_one() was avoided
are cases where a non-upper directory file handle was encoded, then the
directory removed and then file handle was decoded.

To demonstrate the affect on decoding file handles with hot inode/dentry
cache, the drop_caches call in the tests was disabled. Without
drop_caches, there are no reconnect_one() calls at all before or after
the change. Before the change, there are 75 calls to ovl_get_index_fh(),
exactly as the case with drop_caches. After the change, there are only
10 calls to ovl_get_index_fh().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/export.c | 58 +++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 15411350a7a1..a908da8b63c1 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -703,25 +703,39 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 	struct ovl_path *stack = &origin;
 	struct dentry *dentry = NULL;
 	struct dentry *index = NULL;
-	struct inode *inode = NULL;
-	bool is_deleted = false;
+	struct inode *inode;
 	int err;
 
-	/* First lookup indexed upper by fh */
+	/* First lookup overlay inode in inode cache by origin fh */
+	err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
+	if (err)
+		return ERR_PTR(err);
+
+	if (d_is_dir(origin.dentry) ||
+	    !(origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
+		inode = ovl_lookup_inode(sb, origin.dentry, false);
+		err = PTR_ERR(inode);
+		if (IS_ERR(inode))
+			goto out_err;
+		if (inode) {
+			dentry = d_find_any_alias(inode);
+			iput(inode);
+			if (dentry)
+				goto out;
+		}
+	}
+
+	/* Then lookup indexed upper/whiteout by origin fh */
 	if (ofs->indexdir) {
 		index = ovl_get_index_fh(ofs, fh);
 		err = PTR_ERR(index);
 		if (IS_ERR(index)) {
-			if (err != -ESTALE)
-				return ERR_PTR(err);
-
-			/* Found a whiteout index - treat as deleted inode */
-			is_deleted = true;
 			index = NULL;
+			goto out_err;
 		}
 	}
 
-	/* Then try to get upper dir by index */
+	/* Then try to get a connected upper dir by index */
 	if (index && d_is_dir(index)) {
 		struct dentry *upper = ovl_index_upper(ofs, index);
 
@@ -734,24 +748,19 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 		goto out;
 	}
 
-	/* Then lookup origin by fh */
-	err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);
-	if (err) {
-		goto out_err;
-	} else if (index) {
-		err = ovl_verify_origin(index, origin.dentry, false);
+	/* Otherwise, get a connected non-upper dir or disconnected non-dir */
+	if (d_is_dir(origin.dentry) &&
+	    (origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
+		dput(origin.dentry);
+		origin.dentry = NULL;
+		err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);
 		if (err)
 			goto out_err;
-	} else if (is_deleted) {
-		/* Lookup deleted non-dir by origin inode */
-		if (!d_is_dir(origin.dentry))
-			inode = ovl_lookup_inode(sb, origin.dentry, false);
-		err = -ESTALE;
-		if (!inode || atomic_read(&inode->i_count) == 1)
+	}
+	if (index) {
+		err = ovl_verify_origin(index, origin.dentry, false);
+		if (err)
 			goto out_err;
-
-		/* Deleted but still open? */
-		index = dget(ovl_i_dentry_upper(inode));
 	}
 
 	dentry = ovl_get_dentry(sb, NULL, &origin, index);
@@ -759,7 +768,6 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
 out:
 	dput(origin.dentry);
 	dput(index);
-	iput(inode);
 	return dentry;
 
 out_err:
-- 
2.7.4

Re: [PATCH v2 3/3] ovl: lookup in inode cache first when decoding lower file handle

[Miklos Szeredi]

On Sun, Mar 11, 2018 at 12:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> When decoding a lower file handle, we need to check if lower file was
> copied up and indexed and if it has a whiteout index, we need to check
> if this is an unlinked but open non-dir before returning -ESTALE.
>
> To find out if this is an unlinked but open non-dir we need to lookup
> an overlay inode in inode cache by lower inode and that requires decoding
> the lower file handle before looking in inode cache.
>
> Before this change, if the lower inode turned out to be a directory, we
> may have paid an expensive cost to reconnect that lower directory for
> nothing.
>
> After this change, we start by decoding a disconnected lower dentry and
> using the lower inode for looking up an overlay inode in inode cache.
> If we find overlay inode and dentry in cache, we avoid the index lookup
> overhead. If we don't find an overlay inode and dentry in cache, then we
> only need to decode a connected lower dentry in case the lower dentry is
> a non-indexed directory.
>
> The xfstests group overlay/exportfs tests decoding overlayfs file
> handles after drop_caches with different states of the file at encode
> and decode time. Overall the tests in the group call ovl_lower_fh_to_d()
> 89 times to decode a lower file handle.
>
> Before this change, the tests called ovl_get_index_fh() 75 times and
> reconnect_one() 61 times.
> After this change, the tests call ovl_get_index_fh() 70 times and
> reconnect_one() 59 times. The 2 cases where reconnect_one() was avoided
> are cases where a non-upper directory file handle was encoded, then the
> directory removed and then file handle was decoded.
>
> To demonstrate the affect on decoding file handles with hot inode/dentry
> cache, the drop_caches call in the tests was disabled. Without
> drop_caches, there are no reconnect_one() calls at all before or after
> the change. Before the change, there are 75 calls to ovl_get_index_fh(),
> exactly as the case with drop_caches. After the change, there are only
> 10 calls to ovl_get_index_fh().
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/export.c | 58 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 15411350a7a1..a908da8b63c1 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -703,25 +703,39 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>         struct ovl_path *stack = &origin;
>         struct dentry *dentry = NULL;
>         struct dentry *index = NULL;
> -       struct inode *inode = NULL;
> -       bool is_deleted = false;
> +       struct inode *inode;
>         int err;
>
> -       /* First lookup indexed upper by fh */
> +       /* First lookup overlay inode in inode cache by origin fh */
> +       err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       if (d_is_dir(origin.dentry) ||
> +           !(origin.dentry->d_flags & DCACHE_DISCONNECTED)) {

I don't understand the above test.

If origin is a connected directory, then we have a chance of being
able to lookup the overlay inode in the icache.

If origin is a disconnected directory, then we don't have a chance,
because if overlay dentry was cached, it would hold a ref to the
connected origin.

If origin is a non-dir, then we just don't care about being disconnected or not.

So test should be

        if (!d_is_dir(origin.dentry) || !(origin.dentry->d_flags &
DCACHE_DISCONNECTED))

What am I missing?

> +               inode = ovl_lookup_inode(sb, origin.dentry, false);
> +               err = PTR_ERR(inode);
> +               if (IS_ERR(inode))
> +                       goto out_err;
> +               if (inode) {
> +                       dentry = d_find_any_alias(inode);
> +                       iput(inode);
> +                       if (dentry)
> +                               goto out;
> +               }
> +       }
> +
> +       /* Then lookup indexed upper/whiteout by origin fh */
>         if (ofs->indexdir) {
>                 index = ovl_get_index_fh(ofs, fh);
>                 err = PTR_ERR(index);
>                 if (IS_ERR(index)) {
> -                       if (err != -ESTALE)
> -                               return ERR_PTR(err);
> -
> -                       /* Found a whiteout index - treat as deleted inode */
> -                       is_deleted = true;
>                         index = NULL;
> +                       goto out_err;
>                 }
>         }
>
> -       /* Then try to get upper dir by index */
> +       /* Then try to get a connected upper dir by index */
>         if (index && d_is_dir(index)) {
>                 struct dentry *upper = ovl_index_upper(ofs, index);
>
> @@ -734,24 +748,19 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>                 goto out;
>         }
>
> -       /* Then lookup origin by fh */
> -       err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);
> -       if (err) {
> -               goto out_err;
> -       } else if (index) {
> -               err = ovl_verify_origin(index, origin.dentry, false);
> +       /* Otherwise, get a connected non-upper dir or disconnected non-dir */
> +       if (d_is_dir(origin.dentry) &&
> +           (origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
> +               dput(origin.dentry);
> +               origin.dentry = NULL;
> +               err = ovl_check_origin_fh(ofs, fh, true, NULL, &stack);
>                 if (err)
>                         goto out_err;
> -       } else if (is_deleted) {
> -               /* Lookup deleted non-dir by origin inode */
> -               if (!d_is_dir(origin.dentry))
> -                       inode = ovl_lookup_inode(sb, origin.dentry, false);
> -               err = -ESTALE;
> -               if (!inode || atomic_read(&inode->i_count) == 1)
> +       }
> +       if (index) {
> +               err = ovl_verify_origin(index, origin.dentry, false);
> +               if (err)
>                         goto out_err;
> -
> -               /* Deleted but still open? */
> -               index = dget(ovl_i_dentry_upper(inode));
>         }
>
>         dentry = ovl_get_dentry(sb, NULL, &origin, index);
> @@ -759,7 +768,6 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>  out:
>         dput(origin.dentry);
>         dput(index);
> -       iput(inode);
>         return dentry;
>
>  out_err:
> --
> 2.7.4
>

Re: [PATCH v2 3/3] ovl: lookup in inode cache first when decoding lower file handle

[Amir Goldstein]

On Thu, Mar 22, 2018 at 12:41 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Sun, Mar 11, 2018 at 12:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> When decoding a lower file handle, we need to check if lower file was
>> copied up and indexed and if it has a whiteout index, we need to check
>> if this is an unlinked but open non-dir before returning -ESTALE.
>>
>> To find out if this is an unlinked but open non-dir we need to lookup
>> an overlay inode in inode cache by lower inode and that requires decoding
>> the lower file handle before looking in inode cache.
>>
>> Before this change, if the lower inode turned out to be a directory, we
>> may have paid an expensive cost to reconnect that lower directory for
>> nothing.
>>
>> After this change, we start by decoding a disconnected lower dentry and
>> using the lower inode for looking up an overlay inode in inode cache.
>> If we find overlay inode and dentry in cache, we avoid the index lookup
>> overhead. If we don't find an overlay inode and dentry in cache, then we
>> only need to decode a connected lower dentry in case the lower dentry is
>> a non-indexed directory.
>>
>> The xfstests group overlay/exportfs tests decoding overlayfs file
>> handles after drop_caches with different states of the file at encode
>> and decode time. Overall the tests in the group call ovl_lower_fh_to_d()
>> 89 times to decode a lower file handle.
>>
>> Before this change, the tests called ovl_get_index_fh() 75 times and
>> reconnect_one() 61 times.
>> After this change, the tests call ovl_get_index_fh() 70 times and
>> reconnect_one() 59 times. The 2 cases where reconnect_one() was avoided
>> are cases where a non-upper directory file handle was encoded, then the
>> directory removed and then file handle was decoded.
>>
>> To demonstrate the affect on decoding file handles with hot inode/dentry
>> cache, the drop_caches call in the tests was disabled. Without
>> drop_caches, there are no reconnect_one() calls at all before or after
>> the change. Before the change, there are 75 calls to ovl_get_index_fh(),
>> exactly as the case with drop_caches. After the change, there are only
>> 10 calls to ovl_get_index_fh().
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/export.c | 58 +++++++++++++++++++++++++++++----------------------
>>  1 file changed, 33 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>> index 15411350a7a1..a908da8b63c1 100644
>> --- a/fs/overlayfs/export.c
>> +++ b/fs/overlayfs/export.c
>> @@ -703,25 +703,39 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>>         struct ovl_path *stack = &origin;
>>         struct dentry *dentry = NULL;
>>         struct dentry *index = NULL;
>> -       struct inode *inode = NULL;
>> -       bool is_deleted = false;
>> +       struct inode *inode;
>>         int err;
>>
>> -       /* First lookup indexed upper by fh */
>> +       /* First lookup overlay inode in inode cache by origin fh */
>> +       err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +
>> +       if (d_is_dir(origin.dentry) ||
>> +           !(origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
>
> I don't understand the above test.
>
> If origin is a connected directory, then we have a chance of being
> able to lookup the overlay inode in the icache.
>
> If origin is a disconnected directory, then we don't have a chance,
> because if overlay dentry was cached, it would hold a ref to the
> connected origin.
>
> If origin is a non-dir, then we just don't care about being disconnected or not.
>
> So test should be
>
>         if (!d_is_dir(origin.dentry) || !(origin.dentry->d_flags &
> DCACHE_DISCONNECTED))
>
> What am I missing?

Probably my mistake.
I will look into it.

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: lookup in inode cache first when decoding lower file handle

[Amir Goldstein]

On Thu, Mar 22, 2018 at 1:55 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Mar 22, 2018 at 12:41 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Sun, Mar 11, 2018 at 12:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> When decoding a lower file handle, we need to check if lower file was
>>> copied up and indexed and if it has a whiteout index, we need to check
>>> if this is an unlinked but open non-dir before returning -ESTALE.
>>>
>>> To find out if this is an unlinked but open non-dir we need to lookup
>>> an overlay inode in inode cache by lower inode and that requires decoding
>>> the lower file handle before looking in inode cache.
>>>
>>> Before this change, if the lower inode turned out to be a directory, we
>>> may have paid an expensive cost to reconnect that lower directory for
>>> nothing.
>>>
>>> After this change, we start by decoding a disconnected lower dentry and
>>> using the lower inode for looking up an overlay inode in inode cache.
>>> If we find overlay inode and dentry in cache, we avoid the index lookup
>>> overhead. If we don't find an overlay inode and dentry in cache, then we
>>> only need to decode a connected lower dentry in case the lower dentry is
>>> a non-indexed directory.
>>>
>>> The xfstests group overlay/exportfs tests decoding overlayfs file
>>> handles after drop_caches with different states of the file at encode
>>> and decode time. Overall the tests in the group call ovl_lower_fh_to_d()
>>> 89 times to decode a lower file handle.
>>>
>>> Before this change, the tests called ovl_get_index_fh() 75 times and
>>> reconnect_one() 61 times.
>>> After this change, the tests call ovl_get_index_fh() 70 times and
>>> reconnect_one() 59 times. The 2 cases where reconnect_one() was avoided
>>> are cases where a non-upper directory file handle was encoded, then the
>>> directory removed and then file handle was decoded.
>>>
>>> To demonstrate the affect on decoding file handles with hot inode/dentry
>>> cache, the drop_caches call in the tests was disabled. Without
>>> drop_caches, there are no reconnect_one() calls at all before or after
>>> the change. Before the change, there are 75 calls to ovl_get_index_fh(),
>>> exactly as the case with drop_caches. After the change, there are only
>>> 10 calls to ovl_get_index_fh().
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/export.c | 58 +++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 33 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>>> index 15411350a7a1..a908da8b63c1 100644
>>> --- a/fs/overlayfs/export.c
>>> +++ b/fs/overlayfs/export.c
>>> @@ -703,25 +703,39 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>>>         struct ovl_path *stack = &origin;
>>>         struct dentry *dentry = NULL;
>>>         struct dentry *index = NULL;
>>> -       struct inode *inode = NULL;
>>> -       bool is_deleted = false;
>>> +       struct inode *inode;
>>>         int err;
>>>
>>> -       /* First lookup indexed upper by fh */
>>> +       /* First lookup overlay inode in inode cache by origin fh */
>>> +       err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
>>> +       if (err)
>>> +               return ERR_PTR(err);
>>> +
>>> +       if (d_is_dir(origin.dentry) ||
>>> +           !(origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
>>
>> I don't understand the above test.
>>
>> If origin is a connected directory, then we have a chance of being
>> able to lookup the overlay inode in the icache.
>>
>> If origin is a disconnected directory, then we don't have a chance,
>> because if overlay dentry was cached, it would hold a ref to the
>> connected origin.
>>
>> If origin is a non-dir, then we just don't care about being disconnected or not.
>>
>> So test should be
>>
>>         if (!d_is_dir(origin.dentry) || !(origin.dentry->d_flags &
>> DCACHE_DISCONNECTED))
>>
>> What am I missing?
>
> Probably my mistake.
> I will look into it.
>

Yes, it is a mistake as you observed.
I was trying to understand why it didn't affect the tests.

Apparently, the case where underlying dentry is disconnected
and overlay inode is in cache is harder to reproduce and is not covered by
the current tests. I think it requires:
- encoding non-upper non-dir overlay file handle
- drop caches
- open by non-upper file handle (disconnected underlying and not in
overlay icache)
- open by non-upper file handle again (disconnected underlying and in
overlay icache)

The current tests only cover:
- encoding non-upper non-dir overlay file handle
- drop caches
- open by non-upper file handle (disconnected underlying and not in
overlay icache)
AND:
- encoding non-upper non-dir overlay file handle AND keep the file open
- drop caches
- open by non-upper file handle (connected underlying and in overlay icache)

I will add the missing tests to my TODO.
I suppose you will be fixing this bug on commit?

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: lookup in inode cache first when decoding lower file handle

[Miklos Szeredi]

On Thu, Mar 22, 2018 at 4:20 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Mar 22, 2018 at 1:55 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> Yes, it is a mistake as you observed.
> I was trying to understand why it didn't affect the tests.
>
> Apparently, the case where underlying dentry is disconnected
> and overlay inode is in cache is harder to reproduce and is not covered by
> the current tests. I think it requires:
> - encoding non-upper non-dir overlay file handle
> - drop caches
> - open by non-upper file handle (disconnected underlying and not in
> overlay icache)
> - open by non-upper file handle again (disconnected underlying and in
> overlay icache)
>
> The current tests only cover:
> - encoding non-upper non-dir overlay file handle
> - drop caches
> - open by non-upper file handle (disconnected underlying and not in
> overlay icache)
> AND:
> - encoding non-upper non-dir overlay file handle AND keep the file open
> - drop caches
> - open by non-upper file handle (connected underlying and in overlay icache)
>
> I will add the missing tests to my TODO.
> I suppose you will be fixing this bug on commit?

Yes.

Thanks,
Miklos