Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 15:46:45 +0200
From:   Jan Kara <jack@suse.cz>
To:     "Kiselev, Oleg" <okiselev@amazon.com>
Cc:     "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        Theodore Ts'o <tytso@mit.edu>
Subject: Re: [PATCH 1/2] ext4: reduce computation of overhead during resize
Message-ID: <20220714134645.r4gqax4au5el2pox@quack3>
References: <D03FEE2D-DCAE-44A7-B0D3-0047808426BB@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <D03FEE2D-DCAE-44A7-B0D3-0047808426BB@amazon.com>
Precedence: bulk

On Thu 30-06-22 02:17:21, Kiselev, Oleg wrote:
> This patch avoids doing an O(n**2)-complexity walk through every flex group.
> Instead, it uses the already computed overhead information for the newly
> allocated space, and simply adds it to the previously calculated
> overhead stored in the superblock.  This drastically reduces the time
> taken to resize very large bigalloc filesystems (from 3+ hours for a
> 64TB fs down to milliseconds).
> 
> Signed-off-by: Oleg Kiselev <okiselev@amazon.com>
> ---
>  fs/ext4/resize.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)

Overall this looks fine, a few smaller comments below.

> 
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 8b70a4701293..2acc9fca99ea 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -1380,6 +1380,16 @@ static int ext4_setup_new_descs(handle_t *handle, struct super_block *sb,
>  	return err;
>  }
> 
> +static void ext4_set_overhead(struct super_block *sb,
> +                             const ext4_grpblk_t overhead)
> +{

ext4_add_overhead() would be a better name I suppose. Also the 'overhead'
should rather be ext4_fsblk_t to be on the safe side...

> +       struct ext4_sb_info *sbi = EXT4_SB(sb);
> +       struct ext4_super_block *es = sbi->s_es;

Empty line between variable declarations and the code please.

> +       sbi->s_overhead += overhead;
> +       es->s_overhead_clusters = cpu_to_le32((unsigned long) sbi->s_overhead);
						^^^ the typecast looks
bogus here...

> +       smp_wmb();
> +}

The barrier without any comment makes me really wonder why it is here...
But I get ext4_calculate_overhead() has is as well so you're just keeping
it.

> +
>  /*
>   * ext4_update_super() updates the super block so that the newly added
>   * groups can be seen by the filesystem.
> @@ -1482,8 +1492,16 @@ static void ext4_update_super(struct super_block *sb,
> 
>  	/*
>  	 * Update the fs overhead information
> +	 *
> +	 * For bigalloc, if the superblock already has a properly calculated
> +	 * overhead, update it wth a value based on numbers already computed
				^^ with

> +	 * above for the newly allocated capacity.
>  	 */
> -	ext4_calculate_overhead(sb);
> +	if (ext4_has_feature_bigalloc(sb) && (sbi->s_overhead != 0))
> +		ext4_set_overhead(sb,
> +			EXT4_NUM_B2C(sbi, blocks_count - free_blocks));
> +	else
> +		ext4_calculate_overhead(sb);
> 
>  	if (test_opt(sb, DEBUG))
>  		printk(KERN_DEBUG "EXT4-fs: added group %u:"

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR