[Cluster-devel] [GFS2 PATCH 0/5] Refactor gfs2_evict_inode

[Cluster-devel] [GFS2 PATCH 0/5] Refactor gfs2_evict_inode

[Bob Peterson]

Function gfs2_evict_inode is very large and messy. This patch set is an
attempt to simplify the function and make it more understandable. This
will make it easier to maintain. I did this mainly because we need to fix
it so it can call truncate_inode_pages on the inode glock's address space.

Bob Peterson (5):
  gfs2: switch variable error to ret in gfs2_evict_inode
  gfs2: factor delete_evicted_inode out of gfs2_evict_inode
  gfs2: further simplify gfs2_evict_inode with new func
    may_delete_evicted
  gfs2: factor out evict code related to dinodes we are not deleting
  gfs2: simplify the logic in gfs2_evict_inode

 fs/gfs2/super.c | 214 ++++++++++++++++++++++++++++++------------------
 1 file changed, 136 insertions(+), 78 deletions(-)

-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 1/5] gfs2: switch variable error to ret in gfs2_evict_inode

[Bob Peterson]

Function gfs2_evict_inode is too big and unreadable. This patch is
just a baby step toward improving that. This first step just renames
variable error to ret. This will help make future patches more readable.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 19add2da1013..ab08b9a1102c 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1338,7 +1338,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
 	struct address_space *metamapping;
-	int error;
+	int ret;
 
 	if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
 		clear_inode(inode);
@@ -1362,8 +1362,8 @@ static void gfs2_evict_inode(struct inode *inode)
 		goto out;
 
 	/* Must not read inode block until block type has been verified */
-	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
-	if (unlikely(error)) {
+	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
+	if (unlikely(ret)) {
 		glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
@@ -1372,13 +1372,13 @@ static void gfs2_evict_inode(struct inode *inode)
 
 	if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
 		goto out_truncate;
-	error = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
-	if (error)
+	ret = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
+	if (ret)
 		goto out_truncate;
 
 	if (test_bit(GIF_INVALID, &ip->i_flags)) {
-		error = gfs2_inode_refresh(ip);
-		if (error)
+		ret = gfs2_inode_refresh(ip);
+		if (ret)
 			goto out_truncate;
 	}
 
@@ -1399,20 +1399,20 @@ static void gfs2_evict_inode(struct inode *inode)
 
 	if (S_ISDIR(inode->i_mode) &&
 	    (ip->i_diskflags & GFS2_DIF_EXHASH)) {
-		error = gfs2_dir_exhash_dealloc(ip);
-		if (error)
+		ret = gfs2_dir_exhash_dealloc(ip);
+		if (ret)
 			goto out_unlock;
 	}
 
 	if (ip->i_eattr) {
-		error = gfs2_ea_dealloc(ip);
-		if (error)
+		ret = gfs2_ea_dealloc(ip);
+		if (ret)
 			goto out_unlock;
 	}
 
 	if (!gfs2_is_stuffed(ip)) {
-		error = gfs2_file_dealloc(ip);
-		if (error)
+		ret = gfs2_file_dealloc(ip);
+		if (ret)
 			goto out_unlock;
 	}
 
@@ -1421,7 +1421,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	   location and try to set gl_object again. We clear gl_object here so
 	   that subsequent inode creates don't see an old gl_object. */
 	glock_clear_object(ip->i_gl, ip);
-	error = gfs2_dinode_dealloc(ip);
+	ret = gfs2_dinode_dealloc(ip);
 	gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
 	goto out_unlock;
 
@@ -1436,8 +1436,8 @@ static void gfs2_evict_inode(struct inode *inode)
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 
-	error = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
-	if (error)
+	ret = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
+	if (ret)
 		goto out_unlock;
 	/* Needs to be done before glock release & also in a transaction */
 	truncate_inode_pages(&inode->i_data, 0);
@@ -1452,8 +1452,8 @@ static void gfs2_evict_inode(struct inode *inode)
 		glock_clear_object(ip->i_gl, ip);
 		gfs2_glock_dq_uninit(&gh);
 	}
-	if (error && error != GLR_TRYFAILED && error != -EROFS)
-		fs_warn(sdp, "gfs2_evict_inode: %d\n", error);
+	if (ret && ret != GLR_TRYFAILED && ret != -EROFS)
+		fs_warn(sdp, "gfs2_evict_inode: %d\n", ret);
 out:
 	truncate_inode_pages_final(&inode->i_data);
 	if (ip->i_qadata)
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 2/5] gfs2: factor delete_evicted_inode out of gfs2_evict_inode

[Bob Peterson]

Function gfs2_evict_inode is way too big, complex and unreadable. This is a
first baby step toward breaking it apart to be more readable. It factors out
the portion that deletes the online bits for a dinode that is unlinked and
needs to be deleted. A future patch will factor out more. (If I factor
out too much, the patch itself becomes unreadable).

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 67 +++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index ab08b9a1102c..6f9e394d2a2e 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1310,6 +1310,45 @@ static bool gfs2_upgrade_iopen_glock(struct inode *inode)
 	return true;
 }
 
+/**
+ * delete_evicted_inode - delete the pieces of an unlinked evicted inode
+ * @inode: The inode to evict
+ */
+static int delete_evicted_inode(struct inode *inode)
+{
+	struct gfs2_inode *ip = GFS2_I(inode);
+	int ret;
+
+	if (S_ISDIR(inode->i_mode) &&
+	    (ip->i_diskflags & GFS2_DIF_EXHASH)) {
+		ret = gfs2_dir_exhash_dealloc(ip);
+		if (ret)
+			goto out;
+	}
+
+	if (ip->i_eattr) {
+		ret = gfs2_ea_dealloc(ip);
+		if (ret)
+			goto out;
+	}
+
+	if (!gfs2_is_stuffed(ip)) {
+		ret = gfs2_file_dealloc(ip);
+		if (ret)
+			goto out;
+	}
+
+	/* We're about to clear the bitmap for the dinode, but as soon as we
+	   do, gfs2_create_inode can create another inode at the same block
+	   location and try to set gl_object again. We clear gl_object here so
+	   that subsequent inode creates don't see an old gl_object. */
+	glock_clear_object(ip->i_gl, ip);
+	ret = gfs2_dinode_dealloc(ip);
+	gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
+out:
+	return ret;
+}
+
 /**
  * gfs2_evict_inode - Remove an inode from cache
  * @inode: The inode to evict
@@ -1396,33 +1435,7 @@ static void gfs2_evict_inode(struct inode *inode)
 			goto out_truncate;
 		}
 	}
-
-	if (S_ISDIR(inode->i_mode) &&
-	    (ip->i_diskflags & GFS2_DIF_EXHASH)) {
-		ret = gfs2_dir_exhash_dealloc(ip);
-		if (ret)
-			goto out_unlock;
-	}
-
-	if (ip->i_eattr) {
-		ret = gfs2_ea_dealloc(ip);
-		if (ret)
-			goto out_unlock;
-	}
-
-	if (!gfs2_is_stuffed(ip)) {
-		ret = gfs2_file_dealloc(ip);
-		if (ret)
-			goto out_unlock;
-	}
-
-	/* We're about to clear the bitmap for the dinode, but as soon as we
-	   do, gfs2_create_inode can create another inode at the same block
-	   location and try to set gl_object again. We clear gl_object here so
-	   that subsequent inode creates don't see an old gl_object. */
-	glock_clear_object(ip->i_gl, ip);
-	ret = gfs2_dinode_dealloc(ip);
-	gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
+	ret = delete_evicted_inode(inode);
 	goto out_unlock;
 
 out_truncate:
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 3/5] gfs2: further simplify gfs2_evict_inode with new func may_delete_evicted

[Bob Peterson]

This patch further simplifies function gfs2_evict_inode() by adding a new
function may_delete_evicted(). The function may also lock the inode glock.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 115 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 40 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 6f9e394d2a2e..82c79143681a 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1350,91 +1350,126 @@ static int delete_evicted_inode(struct inode *inode)
 }
 
 /**
- * gfs2_evict_inode - Remove an inode from cache
+ * may_delete_evicted - determine whether the inode is eligible for deletion
  * @inode: The inode to evict
  *
- * There are three cases to consider:
- * 1. i_nlink == 0, we are final opener (and must deallocate)
- * 2. i_nlink == 0, we are not the final opener (and cannot deallocate)
- * 3. i_nlink > 0
+ * This function determines whether the evicted inode is eligible to be deleted
+ * and locks the inode glock.
  *
- * If the fs is read only, then we have to treat all cases as per #3
- * since we are unable to do any deallocation. The inode will be
- * deallocated by the next read/write node to attempt an allocation
- * in the same resource group
- *
- * We have to (at the moment) hold the inodes main lock to cover
- * the gap between unlocking the shared lock on the iopen lock and
- * taking the exclusive lock. I'd rather do a shared -> exclusive
- * conversion on the iopen lock, but we can change that later. This
- * is safe, just less efficient.
+ * Returns: 1 if the dinode associated with this inode should be deleted
+ *          0 if the dinode should not be deleted, or
+ *          -EEXIST if the decision must be deferred at this time.
  */
-
-static void gfs2_evict_inode(struct inode *inode)
+static int may_delete_evicted(struct inode *inode, struct gfs2_holder *gh)
 {
+	struct gfs2_inode *ip = GFS2_I(inode);
 	struct super_block *sb = inode->i_sb;
 	struct gfs2_sbd *sdp = sb->s_fs_info;
-	struct gfs2_inode *ip = GFS2_I(inode);
-	struct gfs2_holder gh;
-	struct address_space *metamapping;
 	int ret;
 
-	if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
-		clear_inode(inode);
-		return;
-	}
-
-	if (inode->i_nlink || sb_rdonly(sb))
-		goto out;
-
 	if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
 		BUG_ON(!gfs2_glock_is_locked_by_me(ip->i_gl));
-		gfs2_holder_mark_uninitialized(&gh);
-		goto out_delete;
+		gfs2_holder_mark_uninitialized(gh);
+		goto may_delete;
 	}
 
 	if (test_bit(GIF_DEFERRED_DELETE, &ip->i_flags))
-		goto out;
+		goto defer;
 
 	/* Deletes should never happen under memory pressure anymore.  */
 	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
-		goto out;
+		goto defer;
 
 	/* Must not read inode block until block type has been verified */
-	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
+	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, gh);
 	if (unlikely(ret)) {
 		glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
 		ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
 		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
-		goto out;
+		goto defer;
 	}
 
 	if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
-		goto out_truncate;
+		goto may_not_delete;
 	ret = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
 	if (ret)
-		goto out_truncate;
+		goto may_not_delete;
 
 	if (test_bit(GIF_INVALID, &ip->i_flags)) {
 		ret = gfs2_inode_refresh(ip);
 		if (ret)
-			goto out_truncate;
+			goto may_not_delete;
 	}
 
 	/*
 	 * The inode may have been recreated in the meantime.
 	 */
 	if (inode->i_nlink)
-		goto out_truncate;
+		goto may_not_delete;
 
-out_delete:
+may_delete:
 	if (gfs2_holder_initialized(&ip->i_iopen_gh) &&
 	    test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
 		if (!gfs2_upgrade_iopen_glock(inode)) {
 			gfs2_holder_uninit(&ip->i_iopen_gh);
-			goto out_truncate;
+			goto may_not_delete;
 		}
 	}
+	return 1;
+
+may_not_delete:
+	return 0;
+
+defer:
+	return -EEXIST;
+}
+
+/**
+ * gfs2_evict_inode - Remove an inode from cache
+ * @inode: The inode to evict
+ *
+ * There are three cases to consider:
+ * 1. i_nlink == 0, we are final opener (and must deallocate)
+ * 2. i_nlink == 0, we are not the final opener (and cannot deallocate)
+ * 3. i_nlink > 0
+ *
+ * If the fs is read only, then we have to treat all cases as per #3
+ * since we are unable to do any deallocation. The inode will be
+ * deallocated by the next read/write node to attempt an allocation
+ * in the same resource group
+ *
+ * We have to (at the moment) hold the inodes main lock to cover
+ * the gap between unlocking the shared lock on the iopen lock and
+ * taking the exclusive lock. I'd rather do a shared -> exclusive
+ * conversion on the iopen lock, but we can change that later. This
+ * is safe, just less efficient.
+ */
+
+static void gfs2_evict_inode(struct inode *inode)
+{
+	struct super_block *sb = inode->i_sb;
+	struct gfs2_sbd *sdp = sb->s_fs_info;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_holder gh;
+	struct address_space *metamapping;
+	int ret;
+
+	if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
+		clear_inode(inode);
+		return;
+	}
+
+	if (inode->i_nlink || sb_rdonly(sb))
+		goto out;
+
+	gfs2_holder_mark_uninitialized(&gh);
+	/* may_delete_evicted also locks the inode glock */
+	ret = may_delete_evicted(inode, &gh);
+	if (ret == -EEXIST)
+		goto out;
+	if (!ret)
+		goto out_truncate;
+
 	ret = delete_evicted_inode(inode);
 	goto out_unlock;
 
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 4/5] gfs2: factor out evict code related to dinodes we are not deleting

[Bob Peterson]

Now that we've factored out the delete-dinode case to simplify gfs2_evict_inode
we take it a step further and factor out the other case: where we don't
delete the inode.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 51 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 82c79143681a..46260f370090 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1424,6 +1424,38 @@ static int may_delete_evicted(struct inode *inode, struct gfs2_holder *gh)
 	return -EEXIST;
 }
 
+/**
+ * evict_saved_inode - evict an inode whose dinode has not been deleted
+ * @inode: The inode to evict
+ */
+static void evict_saved_inode(struct inode *inode)
+{
+	struct super_block *sb = inode->i_sb;
+	struct gfs2_sbd *sdp = sb->s_fs_info;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct address_space *metamapping;
+	int ret;
+
+	gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
+		       GFS2_LFC_EVICT_INODE);
+	metamapping = gfs2_glock2aspace(ip->i_gl);
+	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
+		filemap_fdatawrite(metamapping);
+		filemap_fdatawait(metamapping);
+	}
+	write_inode_now(inode, 1);
+	gfs2_ail_flush(ip->i_gl, 0);
+
+	ret = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
+	if (ret)
+		return;
+
+	/* Needs to be done before glock release & also in a transaction */
+	truncate_inode_pages(&inode->i_data, 0);
+	truncate_inode_pages(metamapping, 0);
+	gfs2_trans_end(sdp);
+}
+
 /**
  * gfs2_evict_inode - Remove an inode from cache
  * @inode: The inode to evict
@@ -1451,7 +1483,6 @@ static void gfs2_evict_inode(struct inode *inode)
 	struct gfs2_sbd *sdp = sb->s_fs_info;
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder gh;
-	struct address_space *metamapping;
 	int ret;
 
 	if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
@@ -1474,23 +1505,7 @@ static void gfs2_evict_inode(struct inode *inode)
 	goto out_unlock;
 
 out_truncate:
-	gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
-		       GFS2_LFC_EVICT_INODE);
-	metamapping = gfs2_glock2aspace(ip->i_gl);
-	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
-		filemap_fdatawrite(metamapping);
-		filemap_fdatawait(metamapping);
-	}
-	write_inode_now(inode, 1);
-	gfs2_ail_flush(ip->i_gl, 0);
-
-	ret = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
-	if (ret)
-		goto out_unlock;
-	/* Needs to be done before glock release & also in a transaction */
-	truncate_inode_pages(&inode->i_data, 0);
-	truncate_inode_pages(metamapping, 0);
-	gfs2_trans_end(sdp);
+	evict_saved_inode(inode);
 
 out_unlock:
 	if (gfs2_rs_active(&ip->i_res))
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 5/5] gfs2: simplify the logic in gfs2_evict_inode

[Bob Peterson]

Now that we've factored out the deleted and undeleted dinode cases
in gfs2_evict_inode, we can greatly simplify the logic. Now the function
is easy to read and understand.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/super.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 46260f370090..a1b72c371622 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1498,16 +1498,11 @@ static void gfs2_evict_inode(struct inode *inode)
 	ret = may_delete_evicted(inode, &gh);
 	if (ret == -EEXIST)
 		goto out;
-	if (!ret)
-		goto out_truncate;
-
-	ret = delete_evicted_inode(inode);
-	goto out_unlock;
-
-out_truncate:
-	evict_saved_inode(inode);
+	if (ret)
+		ret = delete_evicted_inode(inode);
+	else
+		evict_saved_inode(inode);
 
-out_unlock:
 	if (gfs2_rs_active(&ip->i_res))
 		gfs2_rs_deltree(&ip->i_res);
 
-- 
2.26.2

[Cluster-devel] [GFS2 PATCH 0/5] Refactor gfs2_evict_inode

[Andreas Gruenbacher]

Bob,

On Tue, Sep 15, 2020 at 4:38 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Function gfs2_evict_inode is very large and messy. This patch set is an
> attempt to simplify the function and make it more understandable. This
> will make it easier to maintain. I did this mainly because we need to fix
> it so it can call truncate_inode_pages on the inode glock's address space.

that refactoring looks good in principle; I'll follow up on the
individual patches with some more comments.

The glock's address space lifetime doesn't end with the life of the
associated inode though, so we should probably only call
truncate_inode_pages_final on the glock's address space when the glock
dies. (At that point, nrpages should be zero already, but
nrexceptional might still be nonzero.)

> Bob Peterson (5):
>   gfs2: switch variable error to ret in gfs2_evict_inode
>   gfs2: factor delete_evicted_inode out of gfs2_evict_inode
>   gfs2: further simplify gfs2_evict_inode with new func
>     may_delete_evicted
>   gfs2: factor out evict code related to dinodes we are not deleting
>   gfs2: simplify the logic in gfs2_evict_inode
>
>  fs/gfs2/super.c | 214 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 136 insertions(+), 78 deletions(-)
>
> --
> 2.26.2
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 2/5] gfs2: factor delete_evicted_inode out of gfs2_evict_inode

[Andreas Gruenbacher]

On Tue, Sep 15, 2020 at 4:38 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Function gfs2_evict_inode is way too big, complex and unreadable. This is a
> first baby step toward breaking it apart to be more readable. It factors out
> the portion that deletes the online bits for a dinode that is unlinked and
> needs to be deleted. A future patch will factor out more. (If I factor
> out too much, the patch itself becomes unreadable).
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/super.c | 67 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index ab08b9a1102c..6f9e394d2a2e 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1310,6 +1310,45 @@ static bool gfs2_upgrade_iopen_glock(struct inode *inode)
>         return true;
>  }
>
> +/**
> + * delete_evicted_inode - delete the pieces of an unlinked evicted inode

The inode is in the process of being evicted, so maybe better call
this function evict_delete_inode?

> + * @inode: The inode to evict
> + */
> +static int delete_evicted_inode(struct inode *inode)
> +{
> +       struct gfs2_inode *ip = GFS2_I(inode);
> +       int ret;
> +
> +       if (S_ISDIR(inode->i_mode) &&
> +           (ip->i_diskflags & GFS2_DIF_EXHASH)) {
> +               ret = gfs2_dir_exhash_dealloc(ip);
> +               if (ret)
> +                       goto out;
> +       }
> +
> +       if (ip->i_eattr) {
> +               ret = gfs2_ea_dealloc(ip);
> +               if (ret)
> +                       goto out;
> +       }
> +
> +       if (!gfs2_is_stuffed(ip)) {
> +               ret = gfs2_file_dealloc(ip);
> +               if (ret)
> +                       goto out;
> +       }
> +
> +       /* We're about to clear the bitmap for the dinode, but as soon as we
> +          do, gfs2_create_inode can create another inode at the same block
> +          location and try to set gl_object again. We clear gl_object here so
> +          that subsequent inode creates don't see an old gl_object. */
> +       glock_clear_object(ip->i_gl, ip);
> +       ret = gfs2_dinode_dealloc(ip);
> +       gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
> +out:
> +       return ret;
> +}
> +
>  /**
>   * gfs2_evict_inode - Remove an inode from cache
>   * @inode: The inode to evict
> @@ -1396,33 +1435,7 @@ static void gfs2_evict_inode(struct inode *inode)
>                         goto out_truncate;
>                 }
>         }
> -
> -       if (S_ISDIR(inode->i_mode) &&
> -           (ip->i_diskflags & GFS2_DIF_EXHASH)) {
> -               ret = gfs2_dir_exhash_dealloc(ip);
> -               if (ret)
> -                       goto out_unlock;
> -       }
> -
> -       if (ip->i_eattr) {
> -               ret = gfs2_ea_dealloc(ip);
> -               if (ret)
> -                       goto out_unlock;
> -       }
> -
> -       if (!gfs2_is_stuffed(ip)) {
> -               ret = gfs2_file_dealloc(ip);
> -               if (ret)
> -                       goto out_unlock;
> -       }
> -
> -       /* We're about to clear the bitmap for the dinode, but as soon as we
> -          do, gfs2_create_inode can create another inode at the same block
> -          location and try to set gl_object again. We clear gl_object here so
> -          that subsequent inode creates don't see an old gl_object. */
> -       glock_clear_object(ip->i_gl, ip);
> -       ret = gfs2_dinode_dealloc(ip);
> -       gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
> +       ret = delete_evicted_inode(inode);
>         goto out_unlock;
>
>  out_truncate:
> --
> 2.26.2
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 3/5] gfs2: further simplify gfs2_evict_inode with new func may_delete_evicted

[Andreas Gruenbacher]

On Tue, Sep 15, 2020 at 4:38 PM Bob Peterson <rpeterso@redhat.com> wrote:
> This patch further simplifies function gfs2_evict_inode() by adding a new
> function may_delete_evicted(). The function may also lock the inode glock.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/super.c | 115 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 75 insertions(+), 40 deletions(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 6f9e394d2a2e..82c79143681a 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1350,91 +1350,126 @@ static int delete_evicted_inode(struct inode *inode)
>  }
>
>  /**
> - * gfs2_evict_inode - Remove an inode from cache
> + * may_delete_evicted - determine whether the inode is eligible for deletion

"may_delete_evicted" -> "evict_should_delete"

>   * @inode: The inode to evict
>   *
> - * There are three cases to consider:
> - * 1. i_nlink == 0, we are final opener (and must deallocate)
> - * 2. i_nlink == 0, we are not the final opener (and cannot deallocate)
> - * 3. i_nlink > 0
> + * This function determines whether the evicted inode is eligible to be deleted
> + * and locks the inode glock.
>   *
> - * If the fs is read only, then we have to treat all cases as per #3
> - * since we are unable to do any deallocation. The inode will be
> - * deallocated by the next read/write node to attempt an allocation
> - * in the same resource group
> - *
> - * We have to (at the moment) hold the inodes main lock to cover
> - * the gap between unlocking the shared lock on the iopen lock and
> - * taking the exclusive lock. I'd rather do a shared -> exclusive
> - * conversion on the iopen lock, but we can change that later. This
> - * is safe, just less efficient.
> + * Returns: 1 if the dinode associated with this inode should be deleted
> + *          0 if the dinode should not be deleted, or
> + *          -EEXIST if the decision must be deferred at this time.
>   */
> -
> -static void gfs2_evict_inode(struct inode *inode)
> +static int may_delete_evicted(struct inode *inode, struct gfs2_holder *gh)
>  {
> +       struct gfs2_inode *ip = GFS2_I(inode);
>         struct super_block *sb = inode->i_sb;
>         struct gfs2_sbd *sdp = sb->s_fs_info;
> -       struct gfs2_inode *ip = GFS2_I(inode);
> -       struct gfs2_holder gh;
> -       struct address_space *metamapping;
>         int ret;
>
> -       if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
> -               clear_inode(inode);
> -               return;
> -       }
> -
> -       if (inode->i_nlink || sb_rdonly(sb))
> -               goto out;
> -
>         if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
>                 BUG_ON(!gfs2_glock_is_locked_by_me(ip->i_gl));
> -               gfs2_holder_mark_uninitialized(&gh);
> -               goto out_delete;
> +               gfs2_holder_mark_uninitialized(gh);

The holder is initialized in gfs2_evict_inode now, so this is redundant.

> +               goto may_delete;
>         }
>
>         if (test_bit(GIF_DEFERRED_DELETE, &ip->i_flags))
> -               goto out;
> +               goto defer;
>
>         /* Deletes should never happen under memory pressure anymore.  */
>         if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
> -               goto out;
> +               goto defer;
>
>         /* Must not read inode block until block type has been verified */
> -       ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &gh);
> +       ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, gh);
>         if (unlikely(ret)) {
>                 glock_clear_object(ip->i_iopen_gh.gh_gl, ip);
>                 ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
>                 gfs2_glock_dq_uninit(&ip->i_iopen_gh);
> -               goto out;
> +               goto defer;
>         }
>
>         if (gfs2_inode_already_deleted(ip->i_gl, ip->i_no_formal_ino))
> -               goto out_truncate;
> +               goto may_not_delete;
>         ret = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
>         if (ret)
> -               goto out_truncate;
> +               goto may_not_delete;
>
>         if (test_bit(GIF_INVALID, &ip->i_flags)) {
>                 ret = gfs2_inode_refresh(ip);
>                 if (ret)
> -                       goto out_truncate;
> +                       goto may_not_delete;
>         }
>
>         /*
>          * The inode may have been recreated in the meantime.
>          */
>         if (inode->i_nlink)
> -               goto out_truncate;
> +               goto may_not_delete;
>
> -out_delete:
> +may_delete:
>         if (gfs2_holder_initialized(&ip->i_iopen_gh) &&
>             test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
>                 if (!gfs2_upgrade_iopen_glock(inode)) {
>                         gfs2_holder_uninit(&ip->i_iopen_gh);
> -                       goto out_truncate;
> +                       goto may_not_delete;
>                 }
>         }
> +       return 1;
> +
> +may_not_delete:
> +       return 0;

Just return 0 instead of "goto may_not_delete". Same for "goto defer".
Or make this function return an enum with descriptive names; -EEXIST
doesn't really fit, anyway.

> +
> +defer:
> +       return -EEXIST;
> +}
> +
> +/**
> + * gfs2_evict_inode - Remove an inode from cache
> + * @inode: The inode to evict
> + *
> + * There are three cases to consider:
> + * 1. i_nlink == 0, we are final opener (and must deallocate)
> + * 2. i_nlink == 0, we are not the final opener (and cannot deallocate)
> + * 3. i_nlink > 0
> + *
> + * If the fs is read only, then we have to treat all cases as per #3
> + * since we are unable to do any deallocation. The inode will be
> + * deallocated by the next read/write node to attempt an allocation
> + * in the same resource group
> + *
> + * We have to (at the moment) hold the inodes main lock to cover
> + * the gap between unlocking the shared lock on the iopen lock and
> + * taking the exclusive lock. I'd rather do a shared -> exclusive
> + * conversion on the iopen lock, but we can change that later. This
> + * is safe, just less efficient.
> + */
> +
> +static void gfs2_evict_inode(struct inode *inode)
> +{
> +       struct super_block *sb = inode->i_sb;
> +       struct gfs2_sbd *sdp = sb->s_fs_info;
> +       struct gfs2_inode *ip = GFS2_I(inode);
> +       struct gfs2_holder gh;
> +       struct address_space *metamapping;
> +       int ret;
> +
> +       if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
> +               clear_inode(inode);
> +               return;
> +       }
> +
> +       if (inode->i_nlink || sb_rdonly(sb))
> +               goto out;
> +
> +       gfs2_holder_mark_uninitialized(&gh);
> +       /* may_delete_evicted also locks the inode glock */

... and maybe upgrades the iopen glock ...

I think the above comment can just go though.


> +       ret = may_delete_evicted(inode, &gh);
> +       if (ret == -EEXIST)
> +               goto out;
> +       if (!ret)
> +               goto out_truncate;
> +
>         ret = delete_evicted_inode(inode);
>         goto out_unlock;
>
> --
> 2.26.2
>

Thanks,
Andreas

[Cluster-devel] [GFS2 PATCH 4/5] gfs2: factor out evict code related to dinodes we are not deleting

[Andreas Gruenbacher]

On Tue, Sep 15, 2020 at 4:38 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Now that we've factored out the delete-dinode case to simplify gfs2_evict_inode
> we take it a step further and factor out the other case: where we don't
> delete the inode.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/super.c | 51 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 82c79143681a..46260f370090 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1424,6 +1424,38 @@ static int may_delete_evicted(struct inode *inode, struct gfs2_holder *gh)
>         return -EEXIST;
>  }
>
> +/**
> + * evict_saved_inode - evict an inode whose dinode has not been deleted

Maybe evict_truncate_inode or just evict_inode?

> + * @inode: The inode to evict
> + */
> +static void evict_saved_inode(struct inode *inode)
> +{
> +       struct super_block *sb = inode->i_sb;
> +       struct gfs2_sbd *sdp = sb->s_fs_info;
> +       struct gfs2_inode *ip = GFS2_I(inode);
> +       struct address_space *metamapping;
> +       int ret;
> +
> +       gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
> +                      GFS2_LFC_EVICT_INODE);
> +       metamapping = gfs2_glock2aspace(ip->i_gl);
> +       if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
> +               filemap_fdatawrite(metamapping);
> +               filemap_fdatawait(metamapping);
> +       }
> +       write_inode_now(inode, 1);
> +       gfs2_ail_flush(ip->i_gl, 0);
> +
> +       ret = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
> +       if (ret)
> +               return;

Should return the error here.


> +
> +       /* Needs to be done before glock release & also in a transaction */
> +       truncate_inode_pages(&inode->i_data, 0);
> +       truncate_inode_pages(metamapping, 0);
> +       gfs2_trans_end(sdp);
> +}
> +
>  /**
>   * gfs2_evict_inode - Remove an inode from cache
>   * @inode: The inode to evict
> @@ -1451,7 +1483,6 @@ static void gfs2_evict_inode(struct inode *inode)
>         struct gfs2_sbd *sdp = sb->s_fs_info;
>         struct gfs2_inode *ip = GFS2_I(inode);
>         struct gfs2_holder gh;
> -       struct address_space *metamapping;
>         int ret;
>
>         if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
> @@ -1474,23 +1505,7 @@ static void gfs2_evict_inode(struct inode *inode)
>         goto out_unlock;
>
>  out_truncate:
> -       gfs2_log_flush(sdp, ip->i_gl, GFS2_LOG_HEAD_FLUSH_NORMAL |
> -                      GFS2_LFC_EVICT_INODE);
> -       metamapping = gfs2_glock2aspace(ip->i_gl);
> -       if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
> -               filemap_fdatawrite(metamapping);
> -               filemap_fdatawait(metamapping);
> -       }
> -       write_inode_now(inode, 1);
> -       gfs2_ail_flush(ip->i_gl, 0);
> -
> -       ret = gfs2_trans_begin(sdp, 0, sdp->sd_jdesc->jd_blocks);
> -       if (ret)
> -               goto out_unlock;
> -       /* Needs to be done before glock release & also in a transaction */
> -       truncate_inode_pages(&inode->i_data, 0);
> -       truncate_inode_pages(metamapping, 0);
> -       gfs2_trans_end(sdp);
> +       evict_saved_inode(inode);
>
>  out_unlock:
>         if (gfs2_rs_active(&ip->i_res))
> --
> 2.26.2
>

Thanks,
Andreas