Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Theodore Ts'o]

On Sat, Jun 10, 2023 at 03:03:19PM -0400, Theodore Ts'o wrote:
> Unforuntately, the changes to ext4_insert_delayed_block() in this
> patch were buggy, and were causing tests to hang when running
> ext4/encrypt, ext4/bigalloc_4k, and ext4/bigalloc_1k test scenarios.
> A bisect using "gce-xfstests -c ext4/bigalloc_4k -C 5 generic/579"
> pinpointed the problem.
> 
> The problem is that ext4_clu_mapped can return a positive value, and
> so there are times when we do need to release the space even though
> there are no errors.
> 
> So I've fixed up your commit with the following changes.  With this
> change, the test regressions go away.

It turns out my fix was not correct, because I misread the fundamental
problem with the patch.  The issue was in the last patch hunk:
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
 errout:
 	return ret;
 }

The problem is that entering this code hunk, ret could be non-zero.
But when we made ext4_es_insert_delayed_block() to return void.  So
the changes to fs/ext4/inode.c needed to be replaced by this:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 129b9af53d62..7700db1782dd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1630,7 +1630,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	int ret;
 	bool allocated = false;
-	bool reserved = false;
 
 	/*
 	 * If the cluster containing lblk is shared with a delayed,
@@ -1646,8 +1645,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	if (sbi->s_cluster_ratio == 1) {
 		ret = ext4_da_reserve_space(inode);
 		if (ret != 0)   /* ENOSPC */
-			goto errout;
-		reserved = true;
+			return ret;
 	} else {   /* bigalloc */
 		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
 			if (!ext4_es_scan_clu(inode,
@@ -1655,12 +1653,11 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 				ret = ext4_clu_mapped(inode,
 						      EXT4_B2C(sbi, lblk));
 				if (ret < 0)
-					goto errout;
+					return ret;
 				if (ret == 0) {
 					ret = ext4_da_reserve_space(inode);
 					if (ret != 0)   /* ENOSPC */
-						goto errout;
-					reserved = true;
+						return ret;
 				} else {
 					allocated = true;
 				}
@@ -1670,12 +1667,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		}
 	}
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
-errout:
-	return ret;
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
+	return 0;
 }
 
 /*

						- Ted