Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Apr 2023 16:36:26 +0530
From:   Ojaswin Mujoo <ojaswin@linux.ibm.com>
To:     Kemeng Shi <shikemeng@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/8] ext4: treat stripe in block unit
Message-ID: <ZC/5MpJrKxzxMTl6@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <20230321161220.418652-1-shikemeng@huaweicloud.com>
 <20230321161220.418652-6-shikemeng@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321161220.418652-6-shikemeng@huaweicloud.com>
Precedence: bulk

On Wed, Mar 22, 2023 at 12:12:17AM +0800, Kemeng Shi wrote:
> Stripe is misused in block unit and in cluster unit in different code
> paths. User awared of stripe maybe not awared of bigalloc feature, so
> treat stripe only in block unit to fix this.
> Besides, it's hard to get stripe aligned blocks (start and length are both
> aligned with stripe) if stripe is not aligned with cluster, just disable
> stripe and alert user in this case to simpfy the code and avoid
> unecessary work to get stripe aligned blocks which likely to be failed.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Nice fixes, and I agree that we can disable stripes if it is not aligned
with cluster. There are anyways some gaps in our stripe support eg the
normalization logic doesnt even take stripesize into account when
determining the goal length.

Anyways, for this patch feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Regards,
ojaswin

> ---
>  fs/ext4/mballoc.c | 18 +++++++++++-------
>  fs/ext4/super.c   | 13 +++++++++++++
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 9a40e165e7d2..b963111eeec6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2178,7 +2178,8 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac,
>  			     ac->ac_g_ex.fe_len, &ex);
>  	ex.fe_logical = 0xDEADFA11; /* debug value */
>  
> -	if (max >= ac->ac_g_ex.fe_len && ac->ac_g_ex.fe_len == sbi->s_stripe) {
> +	if (max >= ac->ac_g_ex.fe_len &&
> +	    ac->ac_g_ex.fe_len == EXT4_B2C(sbi, sbi->s_stripe)) {
>  		ext4_fsblk_t start;
>  
>  		start = ext4_grp_offs_to_block(ac->ac_sb, &ex);
> @@ -2343,7 +2344,7 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac,
>  	struct ext4_free_extent ex;
>  	ext4_fsblk_t first_group_block;
>  	ext4_fsblk_t a;
> -	ext4_grpblk_t i;
> +	ext4_grpblk_t i, stripe;
>  	int max;
>  
>  	BUG_ON(sbi->s_stripe == 0);
> @@ -2355,10 +2356,12 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac,
>  	do_div(a, sbi->s_stripe);
>  	i = (a * sbi->s_stripe) - first_group_block;
>  
> +	stripe = EXT4_B2C(sbi, sbi->s_stripe);
> +	i = EXT4_B2C(sbi, i);
>  	while (i < EXT4_CLUSTERS_PER_GROUP(sb)) {
>  		if (!mb_test_bit(i, bitmap)) {
> -			max = mb_find_extent(e4b, i, sbi->s_stripe, &ex);
> -			if (max >= sbi->s_stripe) {
> +			max = mb_find_extent(e4b, i, stripe, &ex);
> +			if (max >= stripe) {
>  				ac->ac_found++;
>  				ex.fe_logical = 0xDEADF00D; /* debug value */
>  				ac->ac_b_ex = ex;
> @@ -2366,7 +2369,7 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac,
>  				break;
>  			}
>  		}
> -		i += sbi->s_stripe;
> +		i += stripe;
>  	}
>  }
>  
> @@ -2727,7 +2730,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  			if (cr == 0)
>  				ext4_mb_simple_scan_group(ac, &e4b);
>  			else if (cr == 1 && sbi->s_stripe &&
> -					!(ac->ac_g_ex.fe_len % sbi->s_stripe))
> +				 !(ac->ac_g_ex.fe_len %
> +				 EXT4_B2C(sbi, sbi->s_stripe)))
>  				ext4_mb_scan_aligned(ac, &e4b);
>  			else
>  				ext4_mb_complex_scan_group(ac, &e4b);
> @@ -3441,7 +3445,7 @@ int ext4_mb_init(struct super_block *sb)
>  	 */
>  	if (sbi->s_stripe > 1) {
>  		sbi->s_mb_group_prealloc = roundup(
> -			sbi->s_mb_group_prealloc, sbi->s_stripe);
> +			sbi->s_mb_group_prealloc, EXT4_B2C(sbi, sbi->s_stripe));
>  	}
>  
>  	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index f226f8ab469b..0a5bf375df5c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5231,6 +5231,19 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		goto failed_mount3;
>  
>  	sbi->s_stripe = ext4_get_stripe_size(sbi);
> +	/*
> +	 * It's hard to get stripe aligned blocks if stripe is not aligned with
> +	 * cluster, just disable stripe and alert user to simpfy code and avoid
> +	 * stripe aligned allocation which will rarely successes.
> +	 */
> +	if (sbi->s_stripe > 0 && sbi->s_cluster_ratio > 1 &&
> +	    sbi->s_stripe % sbi->s_cluster_ratio != 0) {
> +		ext4_msg(sb, KERN_WARNING,
> +			 "stripe (%lu) is not aligned with cluster size (%u), "
> +			 "stripe is disabled",
> +			 sbi->s_stripe, sbi->s_cluster_ratio);
> +		sbi->s_stripe = 0;
> +	}
>  	sbi->s_extent_max_zeroout_kb = 32;
>  
>  	/*
> -- 
> 2.30.0
>