Messages in this thread | | | Date | Mon, 12 Jun 2023 11:47:07 +0800 | Subject | Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void | From | Baokun Li <> |
| |
On 2023/6/12 11:04, Theodore Ts'o wrote: > 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. I'm very sorry, I didn't turn on encrypt or bigalloc when I tested it. >> >> 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.
Yes, ext4_clu_mapped may return a positive value,
but when it does, reserved is false and we never need to release the space.
>> So I've fixed up your commit with the following changes. With this >> change, the test regressions go away. The previous reply was very confusing to me because the changes in the previous reply have nothing to do with ext4_clu_mapped and ret is always 0 when reserved is true, so we don't need ext4_da_release_space to perform a rollback. > 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; > }
Indeed, there is a behavioral change in ret here.
Before modification:
ext4_da_map_blocks --> return 0 ext4_insert_delayed_block --> return 0 ext4_clu_mapped --> return 1 ext4_es_insert_delayed_block --> return 0
After modification:
ext4_da_map_blocks --> return 1 ext4_insert_delayed_block --> return 1 ext4_clu_mapped --> return 1 ext4_es_insert_delayed_block --> void
> > 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 > Yes, it looks very good!A million thanks for the fix!
I am very sorry for taking your time to locate and fix this issue!
I will do more checks later.
-- With Best Regards, Baokun Li .
| |