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

[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
.