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
