Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Wed, 13 Dec 2023 10:27:40 +0100
From:   Jan Kara <jack@suse.cz>
To:     Suraj Jitindar Singh <surajjs@amazon.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        sjitindarsingh@smail.com, stable@vger.kernel.org
Subject: Re: [PATCH] fs/ext4: Allow for the last group to be marked as trimmed
Message-ID: <20231213092740.2q3lwn45hpmtqoxu@quack3>
References: <20231213051635.37731-1-surajjs@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231213051635.37731-1-surajjs@amazon.com>
Precedence: bulk

On Wed 13-12-23 16:16:35, Suraj Jitindar Singh wrote:
> The ext4 filesystem tracks the trim status of blocks at the group level.
> When an entire group has been trimmed then it is marked as such and subsequent
> trim invocations with the same minimum trim size will not be attempted on that
> group unless it is marked as able to be trimmed again such as when a block is
> freed.
> 
> Currently the last group can't be marked as trimmed due to incorrect logic
> in ext4_last_grp_cluster(). ext4_last_grp_cluster() is supposed to return the
> zero based index of the last cluster in a group. This is then used by
> ext4_try_to_trim_range() to determine if the trim operation spans the entire
> group and as such if the trim status of the group should be recorded.
> 
> ext4_last_grp_cluster() takes a 0 based group index, thus the valid values
> for grp are 0..(ext4_get_groups_count - 1). Any group index less than
> (ext4_get_groups_count - 1) is not the last group and must have
> EXT4_CLUSTERS_PER_GROUP(sb) clusters. For the last group we need to calculate
> the number of clusters based on the number of blocks in the group. Finally
> subtract 1 from the number of clusters as zero based indexing is expected.
> Rearrange the function slightly to make it clear what we are calculating
> and returning.
> 
> Reproducer:
> // Create file system where the last group has fewer blocks than blocks per group
> $ mkfs.ext4 -b 4096 -g 8192 /dev/nvme0n1 8191
> $ mount /dev/nvme0n1 /mnt
> 
> Before Patch:
> $ fstrim -v /mnt
> /mnt: 25.9 MiB (27156480 bytes) trimmed
> // Group not marked as trimmed so second invocation still discards blocks
> $ fstrim -v /mnt
> /mnt: 25.9 MiB (27156480 bytes) trimmed
> 
> After Patch:
> fstrim -v /mnt
> /mnt: 25.9 MiB (27156480 bytes) trimmed
> // Group marked as trimmed so second invocation DOESN'T discard any blocks
> fstrim -v /mnt
> /mnt: 0 B (0 bytes) trimmed
> 
> Fixes: 45e4ab320c9b ("ext4: move setting of trimmed bit into ext4_try_to_trim_range()")
> Cc: stable@vger.kernel.org # 4.19+
> Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>

Indeed. The fix looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 454d5612641ee..c15d8b6f887dd 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6731,11 +6731,16 @@ __acquires(bitlock)
>  static ext4_grpblk_t ext4_last_grp_cluster(struct super_block *sb,
>  					   ext4_group_t grp)
>  {
> -	if (grp < ext4_get_groups_count(sb))
> -		return EXT4_CLUSTERS_PER_GROUP(sb) - 1;
> -	return (ext4_blocks_count(EXT4_SB(sb)->s_es) -
> -		ext4_group_first_block_no(sb, grp) - 1) >>
> -					EXT4_CLUSTER_BITS(sb);
> +	unsigned long nr_clusters_in_group;
> +
> +	if (grp < (ext4_get_groups_count(sb) - 1))
> +		nr_clusters_in_group = EXT4_CLUSTERS_PER_GROUP(sb);
> +	else
> +		nr_clusters_in_group = (ext4_blocks_count(EXT4_SB(sb)->s_es) -
> +					ext4_group_first_block_no(sb, grp))
> +				       >> EXT4_CLUSTER_BITS(sb);
> +
> +	return nr_clusters_in_group - 1;
>  }
>  
>  static bool ext4_trim_interrupted(void)
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR