Re: [PATCH 2/2] ext4: get ext4_group_desc in ext4_mb_prefetch only when necessary

[Jan Kara]

On Thu 21-05-26 14:59:29, Bohdan Trach wrote:
> Getting ext4_group_desc structure can contribute to the cost of
> ext4_mb_prefetch() without any need, as most groups fail the
> !EXT4_MB_GRP_TEST_AND_SET_READ check.
> 
> Optimize ext4_mb_prefetch by getting the group description only when
> necessary.
> 
> The result is further increase in performance of fallocate() system call
> path that triggers ext4_mb_prefetch() via a linear group scan.
> 
> Signed-off-by: Bohdan Trach <bohdan.trach@xxxxxxxxxxxxxxx>

Looks good. Just one nit below:

> @@ -2872,14 +2870,17 @@ ext4_group_t ext4_mb_prefetch(struct super_block *sb, ext4_group_t group,
>  		 * prefetch once, so we avoid getblk() call, which can
>  		 * be expensive.
>  		 */
> -		if (gdp && grp && !EXT4_MB_GRP_TEST_AND_SET_READ(grp) &&
> -		    EXT4_MB_GRP_NEED_INIT(grp) &&
> -		    ext4_free_group_clusters(sb, gdp) > 0 ) {
> -			bh = ext4_read_block_bitmap_nowait(sb, group, true);
> -			if (!IS_ERR_OR_NULL(bh)) {
> -				if (!buffer_uptodate(bh) && cnt)
> -					(*cnt)++;
> -				brelse(bh);
> +		if (group < ngroups && grp && !EXT4_MB_GRP_TEST_AND_SET_READ(grp) &&

The group < ngroup should be always true and is also implicitly contained
in the 'grp != NULL' check. So I'd remove that bit. Otherwise feel free to
add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

									Honza

> +		    EXT4_MB_GRP_NEED_INIT(grp)) {
> +			struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);
> +
> +			if (gdp && ext4_free_group_clusters(sb, gdp) > 0) {
> +				bh = ext4_read_block_bitmap_nowait(sb, group, true);
> +				if (!IS_ERR_OR_NULL(bh)) {
> +					if (!buffer_uptodate(bh) && cnt)
> +						(*cnt)++;
> +					brelse(bh);
> +				}
>  			}
>  		}
>  		if (++group >= ngroups)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR