[PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[Li Dongyang]

Currently the flag indicating block group has done fstrim is not
persistent, and trim status will be lost after remount, as
a result fstrim can not skip the already trimmed groups, which
could be slow on very large devices.

This patch introduces a new block group flag EXT4_BG_TRIMMED,
we need 1 extra block group descriptor write after trimming each
block group.
When clearing the flag, the block group descriptor is journalled
already so no extra overhead.

Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
we should honour EXT4_BG_TRIMMED when doing fstrim.
The new super block flag can be turned on/off via tune2fs.

Cc: Shuichi Ihara <sihara@ddn.com>
Cc: Andreas Dilger <adilger@dilger.ca>
Cc: Wang Shilong <wangshilong1991@gmail.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Li Dongyang <dongyangli@ddn.com>
---
 fs/ext4/ext4.h      | 10 ++------
 fs/ext4/ext4_jbd2.h |  3 ++-
 fs/ext4/mballoc.c   | 62 +++++++++++++++++++++++++++++++++++----------
 3 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0a2d55faa095..a990fb49b24f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -437,6 +437,7 @@ struct flex_groups {
 #define EXT4_BG_INODE_UNINIT	0x0001 /* Inode table/bitmap not in use */
 #define EXT4_BG_BLOCK_UNINIT	0x0002 /* Block bitmap not in use */
 #define EXT4_BG_INODE_ZEROED	0x0004 /* On-disk itable initialized to zero */
+#define EXT4_BG_TRIMMED		0x0008 /* block group was trimmed */
 
 /*
  * Macro-instructions used to manage group descriptors
@@ -1166,6 +1167,7 @@ struct ext4_inode_info {
 #define EXT2_FLAGS_SIGNED_HASH		0x0001  /* Signed dirhash in use */
 #define EXT2_FLAGS_UNSIGNED_HASH	0x0002  /* Unsigned dirhash in use */
 #define EXT2_FLAGS_TEST_FILESYS		0x0004	/* to test development code */
+#define EXT2_FLAGS_TRACK_TRIM		0x0008  /* Track trim status in each bg */
 
 /*
  * Mount flags set via mount options or defaults
@@ -3412,7 +3414,6 @@ struct ext4_group_info {
 };
 
 #define EXT4_GROUP_INFO_NEED_INIT_BIT		0
-#define EXT4_GROUP_INFO_WAS_TRIMMED_BIT		1
 #define EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT	2
 #define EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT	3
 #define EXT4_GROUP_INFO_BBITMAP_CORRUPT		\
@@ -3427,13 +3428,6 @@ struct ext4_group_info {
 	(test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state)))
 #define EXT4_MB_GRP_IBITMAP_CORRUPT(grp)	\
 	(test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state)))
-
-#define EXT4_MB_GRP_WAS_TRIMMED(grp)	\
-	(test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state)))
-#define EXT4_MB_GRP_SET_TRIMMED(grp)	\
-	(set_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state)))
-#define EXT4_MB_GRP_CLEAR_TRIMMED(grp)	\
-	(clear_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state)))
 #define EXT4_MB_GRP_TEST_AND_SET_READ(grp)	\
 	(test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_READ_BIT, &((grp)->bb_state)))
 
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 0c77697d5e90..ce529a454b2a 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -120,7 +120,8 @@
 #define EXT4_HT_MOVE_EXTENTS     9
 #define EXT4_HT_XATTR           10
 #define EXT4_HT_EXT_CONVERT     11
-#define EXT4_HT_MAX             12
+#define EXT4_HT_FS_TRIM		12
+#define EXT4_HT_MAX             13
 
 /**
  *   struct ext4_journal_cb_entry - Base structure for callback information.
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 21b903fe546e..80283be01363 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3849,15 +3849,6 @@ static void ext4_free_data_in_buddy(struct super_block *sb,
 	rb_erase(&entry->efd_node, &(db->bb_free_root));
 	mb_free_blocks(NULL, &e4b, entry->efd_start_cluster, entry->efd_count);
 
-	/*
-	 * Clear the trimmed flag for the group so that the next
-	 * ext4_trim_fs can trim it.
-	 * If the volume is mounted with -o discard, online discard
-	 * is supported and the free blocks will be trimmed online.
-	 */
-	if (!test_opt(sb, DISCARD))
-		EXT4_MB_GRP_CLEAR_TRIMMED(db);
-
 	if (!db->bb_free_root.rb_node) {
 		/* No more items in the per group rb tree
 		 * balance refcounts from ext4_mb_free_metadata()
@@ -6587,8 +6578,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 					 " group:%u block:%d count:%lu failed"
 					 " with %d", block_group, bit, count,
 					 err);
-		} else
-			EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info);
+		}
 
 		ext4_lock_group(sb, block_group);
 		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
@@ -6598,6 +6588,14 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 	ret = ext4_free_group_clusters(sb, gdp) + count_clusters;
 	ext4_free_group_clusters_set(sb, gdp, ret);
 	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
+	/*
+	 * Clear the trimmed flag for the group so that the next
+	 * ext4_trim_fs can trim it.
+	 * If the volume is mounted with -o discard, online discard
+	 * is supported and the free blocks will be trimmed online.
+	 */
+	if (!test_opt(sb, DISCARD))
+		gdp->bg_flags &= cpu_to_le16(~EXT4_BG_TRIMMED);
 	ext4_group_desc_csum_set(sb, block_group, gdp);
 	ext4_unlock_group(sb, block_group);
 
@@ -6995,10 +6993,19 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 		   ext4_grpblk_t minblocks, bool set_trimmed)
 {
 	struct ext4_buddy e4b;
+	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
+	struct ext4_group_desc *gdp;
+	struct buffer_head *gd_bh;
 	int ret;
 
 	trace_ext4_trim_all_free(sb, group, start, max);
 
+	gdp = ext4_get_group_desc(sb, group, &gd_bh);
+	if (!gdp) {
+		ret = -EIO;
+		return ret;
+	}
+
 	ret = ext4_mb_load_buddy(sb, group, &e4b);
 	if (ret) {
 		ext4_warning(sb, "Error %d loading buddy information for %u",
@@ -7008,11 +7015,10 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 
 	ext4_lock_group(sb, group);
 
-	if (!EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) ||
+	if (!(es->s_flags & cpu_to_le16(EXT2_FLAGS_TRACK_TRIM) &&
+	      gdp->bg_flags & cpu_to_le16(EXT4_BG_TRIMMED)) ||
 	    minblocks < EXT4_SB(sb)->s_last_trim_minblks) {
 		ret = ext4_try_to_trim_range(sb, &e4b, start, max, minblocks);
-		if (ret >= 0 && set_trimmed)
-			EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info);
 	} else {
 		ret = 0;
 	}
@@ -7020,6 +7026,34 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
 	ext4_unlock_group(sb, group);
 	ext4_mb_unload_buddy(&e4b);
 
+	if (ret > 0 && set_trimmed) {
+		int err;
+		handle_t *handle;
+
+		handle = ext4_journal_start_sb(sb, EXT4_HT_FS_TRIM, 1);
+		if (IS_ERR(handle)) {
+			ret = PTR_ERR(handle);
+			goto out_return;
+		}
+		err = ext4_journal_get_write_access(handle, sb, gd_bh,
+						    EXT4_JTR_NONE);
+		if (err) {
+			ret = err;
+			goto out_journal;
+		}
+		ext4_lock_group(sb, group);
+		gdp->bg_flags |= cpu_to_le16(EXT4_BG_TRIMMED);
+		ext4_group_desc_csum_set(sb, group, gdp);
+		ext4_unlock_group(sb, group);
+		err = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
+		if (err)
+			ret = err;
+out_journal:
+		err = ext4_journal_stop(handle);
+		if (err)
+			ret = err;
+	}
+out_return:
 	ext4_debug("trimmed %d blocks in the group %d\n",
 		ret, group);
 
-- 
2.41.0

[PATCH 2/2] e2fsprogs: support EXT2_FLAG_BG_TRIMMED and EXT2_FLAGS_TRACK_TRIM

[Li Dongyang]

This adds EXT2_FLAG_BG_TRIMMED, which is used on block group
descriptors during mke2fs after discard is done.
The EXT2_FLAG_BG_TRIMMED flag is cleared on the block group when
we free blocks.

Introduce EXT2_FLAGS_TRACK_TRIM, which is a new super block flag,
to indicate whether we should honour the EXT2_FLAG_BG_TRIMMED
set on each block group.
EXT2_FLAGS_TRACK_TRIM itself can be turned on/off via tune2fs.

Make dumpe2fs aware of the new flags.

Cc: Shuichi Ihara <sihara@ddn.com>
Cc: Andreas Dilger <adilger@dilger.ca>
Cc: Wang Shilong <wangshilong1991@gmail.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Li Dongyang <dongyangli@ddn.com>
---
 lib/e2p/ls.c             |  4 ++++
 lib/ext2fs/alloc_stats.c |  8 ++++++--
 lib/ext2fs/ext2_fs.h     |  2 ++
 misc/dumpe2fs.c          |  2 ++
 misc/mke2fs.c            |  9 +++++++++
 misc/tune2fs.8.in        |  8 ++++++++
 misc/tune2fs.c           | 10 ++++++++++
 7 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib/e2p/ls.c b/lib/e2p/ls.c
index 0b74aea2b..4b356eca6 100644
--- a/lib/e2p/ls.c
+++ b/lib/e2p/ls.c
@@ -162,6 +162,10 @@ static void print_super_flags(struct ext2_super_block * s, FILE *f)
 		fputs("test_filesystem ", f);
 		flags_found++;
 	}
+	if (s->s_flags & EXT2_FLAGS_TRACK_TRIM) {
+		fputs("track_trim ", f);
+		flags_found++;
+	}
 	if (flags_found)
 		fputs("\n", f);
 	else
diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
index 6f98bcc7c..4e03f92a4 100644
--- a/lib/ext2fs/alloc_stats.c
+++ b/lib/ext2fs/alloc_stats.c
@@ -70,10 +70,12 @@ void ext2fs_block_alloc_stats2(ext2_filsys fs, blk64_t blk, int inuse)
 #endif
 		return;
 	}
-	if (inuse > 0)
+	if (inuse > 0) {
 		ext2fs_mark_block_bitmap2(fs->block_map, blk);
-	else
+	} else {
 		ext2fs_unmark_block_bitmap2(fs->block_map, blk);
+		ext2fs_bg_flags_clear(fs, group, EXT2_BG_TRIMMED);
+	}
 	ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group) - inuse);
 	ext2fs_bg_flags_clear(fs, group, EXT2_BG_BLOCK_UNINIT);
 	ext2fs_group_desc_csum_set(fs, group);
@@ -139,6 +141,8 @@ void ext2fs_block_alloc_stats_range(ext2_filsys fs, blk64_t blk,
 			ext2fs_bg_free_blocks_count(fs, group) -
 			inuse*n/EXT2FS_CLUSTER_RATIO(fs));
 		ext2fs_bg_flags_clear(fs, group, EXT2_BG_BLOCK_UNINIT);
+		if (inuse < 0)
+			ext2fs_bg_flags_clear(fs, group, EXT2_BG_TRIMMED);
 		ext2fs_group_desc_csum_set(fs, group);
 		ext2fs_free_blocks_count_add(fs->super, -inuse * (blk64_t) n);
 		blk += n;
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index 0fc9c09a5..88e1114c9 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -223,6 +223,7 @@ struct ext4_group_desc
 #define EXT2_BG_INODE_UNINIT	0x0001 /* Inode table/bitmap not initialized */
 #define EXT2_BG_BLOCK_UNINIT	0x0002 /* Block bitmap not initialized */
 #define EXT2_BG_INODE_ZEROED	0x0004 /* On-disk itable initialized to zero */
+#define EXT2_BG_TRIMMED		0x0008 /* Block group was trimmed */
 
 /*
  * Data structures used by the directory indexing feature
@@ -563,6 +564,7 @@ struct ext2_inode *EXT2_INODE(struct ext2_inode_large *large_inode)
 #define EXT2_FLAGS_SIGNED_HASH		0x0001  /* Signed dirhash in use */
 #define EXT2_FLAGS_UNSIGNED_HASH	0x0002  /* Unsigned dirhash in use */
 #define EXT2_FLAGS_TEST_FILESYS		0x0004	/* OK for use on development code */
+#define EXT2_FLAGS_TRACK_TRIM		0x0008	/* Track trim status in each bg */
 #define EXT2_FLAGS_IS_SNAPSHOT		0x0010	/* This is a snapshot image */
 #define EXT2_FLAGS_FIX_SNAPSHOT		0x0020	/* Snapshot inodes corrupted */
 #define EXT2_FLAGS_FIX_EXCLUDE		0x0040	/* Exclude bitmaps corrupted */
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index 7c080ed9f..afe569dff 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -131,6 +131,8 @@ static void print_bg_opts(ext2_filsys fs, dgrp_t i)
  		     &first);
 	print_bg_opt(bg_flags, EXT2_BG_INODE_ZEROED, "ITABLE_ZEROED",
  		     &first);
+	print_bg_opt(bg_flags, EXT2_BG_TRIMMED, "TRIMMED",
+		     &first);
 	if (!first)
 		fputc(']', stdout);
 	fputc('\n', stdout);
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 4a9c1b092..bbfcde478 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -3154,6 +3154,15 @@ int main (int argc, char *argv[])
 	/* Can't undo discard ... */
 	if (!noaction && discard && dev_size && (io_ptr != undo_io_manager)) {
 		retval = mke2fs_discard_device(fs);
+		if (!retval) {
+			dgrp_t i;
+
+			fs->super->s_flags |= EXT2_FLAGS_TRACK_TRIM;
+			for (i = 0; i < fs->group_desc_count; i++) {
+				ext2fs_bg_flags_set(fs, i, EXT2_BG_TRIMMED);
+				ext2fs_group_desc_csum_set(fs, i);
+			}
+		}
 		if (!retval && io_channel_discard_zeroes_data(fs->io)) {
 			if (verbose)
 				printf("%s",
diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
index dcf108c1f..2eb7e88ed 100644
--- a/misc/tune2fs.8.in
+++ b/misc/tune2fs.8.in
@@ -273,6 +273,14 @@ mounted using experimental kernel code, such as the ext4dev file system.
 .B ^test_fs
 Clear the test_fs flag, indicating the file system should only be mounted
 using production-level file system code.
+.TP
+.B track_trim
+Set a flag in the file system superblock to make fstrim save the trim status
+in each block group and skip the block groups already been trimmed.
+.TP
+.B ^track_trim
+Clear the track_trim flag to make fstrim ignore the trim status saved in
+each block group, and trim every block group.
 .RE
 .TP
 .B \-f
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 458f7cf6a..dd9e8eab0 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -2312,6 +2312,14 @@ static int parse_extended_opts(ext2_filsys fs, const char *opts)
 			sb->s_flags &= ~EXT2_FLAGS_TEST_FILESYS;
 			printf("Clearing test filesystem flag\n");
 			ext2fs_mark_super_dirty(fs);
+		} else if (!strcmp(token, "track_trim")) {
+			sb->s_flags |= EXT2_FLAGS_TRACK_TRIM;
+			printf("Setting track_trim flag\n");
+			ext2fs_mark_super_dirty(fs);
+		} else if (!strcmp(token, "^track_trim")) {
+			sb->s_flags &= ~EXT2_FLAGS_TRACK_TRIM;
+			printf("Clearing track_trim flag\n");
+			ext2fs_mark_super_dirty(fs);
 		} else if (strcmp(token, "stride") == 0) {
 			if (!arg) {
 				r_usage++;
@@ -2458,6 +2466,8 @@ static int parse_extended_opts(ext2_filsys fs, const char *opts)
 			"\tforce_fsck\n"
 			"\ttest_fs\n"
 			"\t^test_fs\n"
+			"\ttrack_trim\n"
+			"\t^track_trim\n"
 			"\tencoding=<encoding>\n"
 			"\tencoding_flags=<flags>\n"));
 		free(buf);
-- 
2.41.0

Re: [PATCH 2/2] e2fsprogs: support EXT2_FLAG_BG_TRIMMED and EXT2_FLAGS_TRACK_TRIM

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 7082 bytes --]

On Aug 11, 2023, at 12:19 AM, Li Dongyang <dongyangli@ddn.com> wrote:
> 
> This adds EXT2_FLAG_BG_TRIMMED, which is used on block group
> descriptors during mke2fs after discard is done.
> The EXT2_FLAG_BG_TRIMMED flag is cleared on the block group when
> we free blocks.
> 
> Introduce EXT2_FLAGS_TRACK_TRIM, which is a new super block flag,
> to indicate whether we should honour the EXT2_FLAG_BG_TRIMMED
> set on each block group.
> EXT2_FLAGS_TRACK_TRIM itself can be turned on/off via tune2fs.
> 
> Make dumpe2fs aware of the new flags.

Ted, I think this patch is uncontroversial, and could be landed as-is.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> 
> Cc: Shuichi Ihara <sihara@ddn.com>
> Cc: Andreas Dilger <adilger@dilger.ca>
> Cc: Wang Shilong <wangshilong1991@gmail.com>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> Signed-off-by: Li Dongyang <dongyangli@ddn.com>
> ---
> lib/e2p/ls.c             |  4 ++++
> lib/ext2fs/alloc_stats.c |  8 ++++++--
> lib/ext2fs/ext2_fs.h     |  2 ++
> misc/dumpe2fs.c          |  2 ++
> misc/mke2fs.c            |  9 +++++++++
> misc/tune2fs.8.in        |  8 ++++++++
> misc/tune2fs.c           | 10 ++++++++++
> 7 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/e2p/ls.c b/lib/e2p/ls.c
> index 0b74aea2b..4b356eca6 100644
> --- a/lib/e2p/ls.c
> +++ b/lib/e2p/ls.c
> @@ -162,6 +162,10 @@ static void print_super_flags(struct ext2_super_block * s, FILE *f)
> 		fputs("test_filesystem ", f);
> 		flags_found++;
> 	}
> +	if (s->s_flags & EXT2_FLAGS_TRACK_TRIM) {
> +		fputs("track_trim ", f);
> +		flags_found++;
> +	}
> 	if (flags_found)
> 		fputs("\n", f);
> 	else
> diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
> index 6f98bcc7c..4e03f92a4 100644
> --- a/lib/ext2fs/alloc_stats.c
> +++ b/lib/ext2fs/alloc_stats.c
> @@ -70,10 +70,12 @@ void ext2fs_block_alloc_stats2(ext2_filsys fs, blk64_t blk, int inuse)
> #endif
> 		return;
> 	}
> -	if (inuse > 0)
> +	if (inuse > 0) {
> 		ext2fs_mark_block_bitmap2(fs->block_map, blk);
> -	else
> +	} else {
> 		ext2fs_unmark_block_bitmap2(fs->block_map, blk);
> +		ext2fs_bg_flags_clear(fs, group, EXT2_BG_TRIMMED);
> +	}
> 	ext2fs_bg_free_blocks_count_set(fs, group, ext2fs_bg_free_blocks_count(fs, group) - inuse);
> 	ext2fs_bg_flags_clear(fs, group, EXT2_BG_BLOCK_UNINIT);
> 	ext2fs_group_desc_csum_set(fs, group);
> @@ -139,6 +141,8 @@ void ext2fs_block_alloc_stats_range(ext2_filsys fs, blk64_t blk,
> 			ext2fs_bg_free_blocks_count(fs, group) -
> 			inuse*n/EXT2FS_CLUSTER_RATIO(fs));
> 		ext2fs_bg_flags_clear(fs, group, EXT2_BG_BLOCK_UNINIT);
> +		if (inuse < 0)
> +			ext2fs_bg_flags_clear(fs, group, EXT2_BG_TRIMMED);
> 		ext2fs_group_desc_csum_set(fs, group);
> 		ext2fs_free_blocks_count_add(fs->super, -inuse * (blk64_t) n);
> 		blk += n;
> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
> index 0fc9c09a5..88e1114c9 100644
> --- a/lib/ext2fs/ext2_fs.h
> +++ b/lib/ext2fs/ext2_fs.h
> @@ -223,6 +223,7 @@ struct ext4_group_desc
> #define EXT2_BG_INODE_UNINIT	0x0001 /* Inode table/bitmap not initialized */
> #define EXT2_BG_BLOCK_UNINIT	0x0002 /* Block bitmap not initialized */
> #define EXT2_BG_INODE_ZEROED	0x0004 /* On-disk itable initialized to zero */
> +#define EXT2_BG_TRIMMED		0x0008 /* Block group was trimmed */
> 
> /*
>  * Data structures used by the directory indexing feature
> @@ -563,6 +564,7 @@ struct ext2_inode *EXT2_INODE(struct ext2_inode_large *large_inode)
> #define EXT2_FLAGS_SIGNED_HASH		0x0001  /* Signed dirhash in use */
> #define EXT2_FLAGS_UNSIGNED_HASH	0x0002  /* Unsigned dirhash in use */
> #define EXT2_FLAGS_TEST_FILESYS		0x0004	/* OK for use on development code */
> +#define EXT2_FLAGS_TRACK_TRIM		0x0008	/* Track trim status in each bg */
> #define EXT2_FLAGS_IS_SNAPSHOT		0x0010	/* This is a snapshot image */
> #define EXT2_FLAGS_FIX_SNAPSHOT		0x0020	/* Snapshot inodes corrupted */
> #define EXT2_FLAGS_FIX_EXCLUDE		0x0040	/* Exclude bitmaps corrupted */
> diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
> index 7c080ed9f..afe569dff 100644
> --- a/misc/dumpe2fs.c
> +++ b/misc/dumpe2fs.c
> @@ -131,6 +131,8 @@ static void print_bg_opts(ext2_filsys fs, dgrp_t i)
>  		     &first);
> 	print_bg_opt(bg_flags, EXT2_BG_INODE_ZEROED, "ITABLE_ZEROED",
>  		     &first);
> +	print_bg_opt(bg_flags, EXT2_BG_TRIMMED, "TRIMMED",
> +		     &first);
> 	if (!first)
> 		fputc(']', stdout);
> 	fputc('\n', stdout);
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 4a9c1b092..bbfcde478 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -3154,6 +3154,15 @@ int main (int argc, char *argv[])
> 	/* Can't undo discard ... */
> 	if (!noaction && discard && dev_size && (io_ptr != undo_io_manager)) {
> 		retval = mke2fs_discard_device(fs);
> +		if (!retval) {
> +			dgrp_t i;
> +
> +			fs->super->s_flags |= EXT2_FLAGS_TRACK_TRIM;
> +			for (i = 0; i < fs->group_desc_count; i++) {
> +				ext2fs_bg_flags_set(fs, i, EXT2_BG_TRIMMED);
> +				ext2fs_group_desc_csum_set(fs, i);
> +			}
> +		}
> 		if (!retval && io_channel_discard_zeroes_data(fs->io)) {
> 			if (verbose)
> 				printf("%s",
> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> index dcf108c1f..2eb7e88ed 100644
> --- a/misc/tune2fs.8.in
> +++ b/misc/tune2fs.8.in
> @@ -273,6 +273,14 @@ mounted using experimental kernel code, such as the ext4dev file system.
> .B ^test_fs
> Clear the test_fs flag, indicating the file system should only be mounted
> using production-level file system code.
> +.TP
> +.B track_trim
> +Set a flag in the file system superblock to make fstrim save the trim status
> +in each block group and skip the block groups already been trimmed.
> +.TP
> +.B ^track_trim
> +Clear the track_trim flag to make fstrim ignore the trim status saved in
> +each block group, and trim every block group.
> .RE
> .TP
> .B \-f
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 458f7cf6a..dd9e8eab0 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -2312,6 +2312,14 @@ static int parse_extended_opts(ext2_filsys fs, const char *opts)
> 			sb->s_flags &= ~EXT2_FLAGS_TEST_FILESYS;
> 			printf("Clearing test filesystem flag\n");
> 			ext2fs_mark_super_dirty(fs);
> +		} else if (!strcmp(token, "track_trim")) {
> +			sb->s_flags |= EXT2_FLAGS_TRACK_TRIM;
> +			printf("Setting track_trim flag\n");
> +			ext2fs_mark_super_dirty(fs);
> +		} else if (!strcmp(token, "^track_trim")) {
> +			sb->s_flags &= ~EXT2_FLAGS_TRACK_TRIM;
> +			printf("Clearing track_trim flag\n");
> +			ext2fs_mark_super_dirty(fs);
> 		} else if (strcmp(token, "stride") == 0) {
> 			if (!arg) {
> 				r_usage++;
> @@ -2458,6 +2466,8 @@ static int parse_extended_opts(ext2_filsys fs, const char *opts)
> 			"\tforce_fsck\n"
> 			"\ttest_fs\n"
> 			"\t^test_fs\n"
> +			"\ttrack_trim\n"
> +			"\t^track_trim\n"
> 			"\tencoding=<encoding>\n"
> 			"\tencoding_flags=<flags>\n"));
> 		free(buf);
> --
> 2.41.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[Theodore Ts'o]

On Fri, Aug 11, 2023 at 04:19:04PM +1000, Li Dongyang wrote:
> Currently the flag indicating block group has done fstrim is not
> persistent, and trim status will be lost after remount, as
> a result fstrim can not skip the already trimmed groups, which
> could be slow on very large devices.
> 
> This patch introduces a new block group flag EXT4_BG_TRIMMED,
> we need 1 extra block group descriptor write after trimming each
> block group.
> When clearing the flag, the block group descriptor is journalled
> already so no extra overhead.

If we journalling is enabled (and it normally is enabled) then there
is also writes to the journalling.  Updating the block group
descriptor is also a random 4k write, which is not nothing.  So if we
are going to do this, then we should not try to set the flag if the
block group is unitialized, and we should actually send the discard in
that case, since presumably the blocks in question were discard when
the file system was mkfs'ed.

> Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
> we should honour EXT4_BG_TRIMMED when doing fstrim.
> The new super block flag can be turned on/off via tune2fs.

I don't see the point of having the superblock flag.  It seems to me
that either we should either do this via a proper feature flag, which
means that older kernels (and grub bootloaders that get release
updates at a super-lackadasical pace) won't touch file systems that
have the feature flag set --- or we don't have any kind of flag at
all, and kernels and userspace utilities which are EXT4_BG_TRIMMED
enlightened will honor and set/clear the flag.

This risk if we go down that path is that if we have a file system
which is normally used by a kernel that has support for this feature,
and that file system is mounted by an older kernel which doesn't have
this flag, there might be cases where the file system would be trimmed
without setting these flags, or blocks might get released on a block
group without clearing the flag.  Fortunately, trim is advisory, so if
we trim a block group that doesn't need it, or we don't trim a block
group where discard might be useful, it's not the end of the world.
And we could always have "e2fsck -E discard" ignore the
EXT4_BG_TRIMMED flag, and just trim all the blocks[1].

[1] https://photos.app.goo.gl/eVL9yHpjdXhjAnq88

						- Ted

Re: [PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

On Aug 11, 2023, at 12:35 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> On Fri, Aug 11, 2023 at 04:19:04PM +1000, Li Dongyang wrote:
>> Currently the flag indicating block group has done fstrim is not
>> persistent, and trim status will be lost after remount, as
>> a result fstrim can not skip the already trimmed groups, which
>> could be slow on very large devices.
>> 
>> This patch introduces a new block group flag EXT4_BG_TRIMMED,
>> we need 1 extra block group descriptor write after trimming each
>> block group.
>> When clearing the flag, the block group descriptor is journalled
>> already so no extra overhead.
> 
> If we journalling is enabled (and it normally is enabled) then there
> is also writes to the journalling.  Updating the block group
> descriptor is also a random 4k write, which is not nothing.  So if we
> are going to do this, then we should not try to set the flag if the
> block group is unitialized, and we should actually send the discard in
> that case, since presumably the blocks in question were discard when
> the file system was mkfs'ed.

Sorry Ted, I'm not sure I understand your comment here.  If the device
is trimmed at mke2fs time, then the BG_TRIMMED flags are set in the
group descriptors, so if the flag is still set then no need to TRIM
those groups later.

The comment about "no extra overhead" is in the case of clearing the
BG_TRIMMED flag when freeing blocks.  In that case, the group descriptors
are already being updated with the new blocks count, so there is no
overhead to clear the BG_TRIMMED flag at the same time.

Definitely there is an extra GDT write after TRIM to set the BG_TRIMMED
flag, but since fstrim is done sequentially for groups it is likely that
multiple groups in a single GDT block would be updated at the same time,
so the overhead is relatively small.

>> Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
>> we should honour EXT4_BG_TRIMMED when doing fstrim.
>> The new super block flag can be turned on/off via tune2fs.
> 
> I don't see the point of having the superblock flag.  It seems to me
> that either we should either do this via a proper feature flag, which
> means that older kernels (and grub bootloaders that get release
> updates at a super-lackadasical pace) won't touch file systems that
> have the feature flag set --- or we don't have any kind of flag at
> all, and kernels and userspace utilities which are EXT4_BG_TRIMMED
> enlightened will honor and set/clear the flag.

In the previous email thread about the persistent BG_TRIMMED flag,
you were requesting a superblock flag and not a full feature, to avoid
the incompatibility issues with a new feature for this:

https://patchwork.ozlabs.org/project/linux-ext4/patch/1592831677-13945-1-git-send-email-wangshilong1991@gmail.com/#2502168

   "So what I was thinking was we could define a new flag which
    would be set in es->s_flags in the on-disk superblock:

    #define EXT2_FLAGS_PERSISTENT_TRIM_TRACKING 0x0008

    If this flag is set, then the EXT4_BG_WAS_TRIMMED flags will
    be honored; otherwise, they will be ignored when FITRIM is
    executed and the block group will be unconditionally trimmed.

    The advantage of doing it this way is that we don't need to
    allocate a new feature bit, and older versions of e2fsck won't
    have heartburn over seeing a feature bit it doesn't understand.
    I also suspect this is something that the system administrator
    will either always want enabled or disabled, so it's better to
    make it be a tunable to be set via tune2fs."

> This risk if we go down that path is that if we have a file system
> which is normally used by a kernel that has support for this feature,
> and that file system is mounted by an older kernel which doesn't have
> this flag, there might be cases where the file system would be trimmed
> without setting these flags, or blocks might get released on a block
> group without clearing the flag.  Fortunately, trim is advisory, so if
> we trim a block group that doesn't need it, or we don't trim a block
> group where discard might be useful, it's not the end of the world.
> And we could always have "e2fsck -E discard" ignore the
> EXT4_BG_TRIMMED flag, and just trim all the blocks[1].

I'm OK with the superblock flag.  Since TRIM is advisory you aren't
going to lose data or corrupt the filesystem if the flags are wrong.
At worst, some TRIM will be skipped until upgrading to a new kernel
or the flag is disabled in the superblock, but this is a corner case.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

On Aug 11, 2023, at 12:19 AM, Li Dongyang <dongyangli@ddn.com> wrote:
> 
> Currently the flag indicating block group has done fstrim is not
> persistent, and trim status will be lost after remount, as
> a result fstrim can not skip the already trimmed groups, which
> could be slow on very large devices.
> 
> This patch introduces a new block group flag EXT4_BG_TRIMMED,
> we need 1 extra block group descriptor write after trimming each
> block group.
> When clearing the flag, the block group descriptor is journalled
> already so no extra overhead.
> 
> Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
> we should honour EXT4_BG_TRIMMED when doing fstrim.
> The new super block flag can be turned on/off via tune2fs.

Dongyang,
I think this is not *quite* correct in the case where the TRACK_TRIM flag
is not set.  I agree we want the BG_TRIMMED flag to always be cleared in
that case when blocks are freed in a group (this has no added cost, and
will maintain correctness even if the feature is disabled).

However, it doesn't look like the patch will skip *writing* the flag if
the TRACK_TRIM flag is unset, which would also add needless overhead in
that case.  I think it is OK to set the flag in memory to maintain the
same behavior as today, and writing it to disk is fine (it will be ignored
anyway), but it shouldn't trigger an extra transaction.

> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 21b903fe546e..80283be01363 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6995,10 +6993,19 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> 		   ext4_grpblk_t minblocks, bool set_trimmed)
> {
> 	struct ext4_buddy e4b;
> +	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
> +	struct ext4_group_desc *gdp;
> +	struct buffer_head *gd_bh;
> 	int ret;
> 
> 	trace_ext4_trim_all_free(sb, group, start, max);
> 
> +	gdp = ext4_get_group_desc(sb, group, &gd_bh);
> +	if (!gdp) {
> +		ret = -EIO;
> +		return ret;
> +	}
> +
> 	ret = ext4_mb_load_buddy(sb, group, &e4b);
> 	if (ret) {
> 		ext4_warning(sb, "Error %d loading buddy information for %u",
> @@ -7008,11 +7015,10 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> 
> 	ext4_lock_group(sb, group);
> 
> -	if (!EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) ||
> +	if (!(es->s_flags & cpu_to_le16(EXT2_FLAGS_TRACK_TRIM) &&
> +	      gdp->bg_flags & cpu_to_le16(EXT4_BG_TRIMMED)) ||
> 	    minblocks < EXT4_SB(sb)->s_last_trim_minblks) {

I think this should still *send* the TRIM request if BG_TRIMMED is not
set, regardless of whether TRACK_TRIM is set or not, it should just
not save the flag to disk below.

> 		ret = ext4_try_to_trim_range(sb, &e4b, start, max, minblocks);
> -		if (ret >= 0 && set_trimmed)
> -			EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info);

This should clear the "set_trimmed" flag if there was an error, so the
flag is not set below.

> 	} else {
> 		ret = 0;
> 	}
> @@ -7020,6 +7026,34 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> 	ext4_unlock_group(sb, group);
> 	ext4_mb_unload_buddy(&e4b);
> 
> +	if (ret > 0 && set_trimmed) {

Here, this should check the TRACK_TRIM flag and not force the GDT write
if the feature is disabled.  *Not* writing the flag to disk is fine, at
worst it means that another TRIM would be sent in case of a crash, which
is what happened before this patch.  Only the BG_TRIMMED flag should be
set in the group descriptor in that case, based on the flag saved above.

Cheers, Andreas

> +		int err;
> +		handle_t *handle;
> +
> +		handle = ext4_journal_start_sb(sb, EXT4_HT_FS_TRIM, 1);
> +		if (IS_ERR(handle)) {
> +			ret = PTR_ERR(handle);
> +			goto out_return;
> +		}
> +		err = ext4_journal_get_write_access(handle, sb, gd_bh,
> +						    EXT4_JTR_NONE);
> +		if (err) {
> +			ret = err;
> +			goto out_journal;
> +		}
> +		ext4_lock_group(sb, group);
> +		gdp->bg_flags |= cpu_to_le16(EXT4_BG_TRIMMED);
> +		ext4_group_desc_csum_set(sb, group, gdp);
> +		ext4_unlock_group(sb, group);
> +		err = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
> +		if (err)
> +			ret = err;
> +out_journal:
> +		err = ext4_journal_stop(handle);
> +		if (err)
> +			ret = err;
> +	}
> +out_return:
> 	ext4_debug("trimmed %d blocks in the group %d\n",
> 		ret, group);
> 
> --
> 2.41.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

[Dongyang Li]

On Mon, 2023-08-14 at 23:04 -0600, Andreas Dilger wrote:
> On Aug 11, 2023, at 12:19 AM, Li Dongyang <dongyangli@ddn.com> wrote:
> > 
> > Currently the flag indicating block group has done fstrim is not
> > persistent, and trim status will be lost after remount, as
> > a result fstrim can not skip the already trimmed groups, which
> > could be slow on very large devices.
> > 
> > This patch introduces a new block group flag EXT4_BG_TRIMMED,
> > we need 1 extra block group descriptor write after trimming each
> > block group.
> > When clearing the flag, the block group descriptor is journalled
> > already so no extra overhead.
> > 
> > Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
> > we should honour EXT4_BG_TRIMMED when doing fstrim.
> > The new super block flag can be turned on/off via tune2fs.
> 
> Dongyang,
> I think this is not *quite* correct in the case where the TRACK_TRIM
> flag
> is not set.  I agree we want the BG_TRIMMED flag to always be cleared
> in
> that case when blocks are freed in a group (this has no added cost,
> and
> will maintain correctness even if the feature is disabled).
> 
> However, it doesn't look like the patch will skip *writing* the flag
> if
> the TRACK_TRIM flag is unset, which would also add needless overhead
> in
> that case.  I think it is OK to set the flag in memory to maintain
> the
> same behavior as today, and writing it to disk is fine (it will be
> ignored
> anyway), but it shouldn't trigger an extra transaction.
I agree with the skip writing flag when TRACK_TRIM is not set.
IMHO I don't think we should maintain essentially the same flags in
memory if we are making the BG_TRIMMED flag persistent.
> 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 21b903fe546e..80283be01363 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -6995,10 +6993,19 @@ ext4_trim_all_free(struct super_block *sb,
> > ext4_group_t group,
> >                    ext4_grpblk_t minblocks, bool set_trimmed)
> > {
> >         struct ext4_buddy e4b;
> > +       struct ext4_super_block *es = EXT4_SB(sb)->s_es;
> > +       struct ext4_group_desc *gdp;
> > +       struct buffer_head *gd_bh;
> >         int ret;
> > 
> >         trace_ext4_trim_all_free(sb, group, start, max);
> > 
> > +       gdp = ext4_get_group_desc(sb, group, &gd_bh);
> > +       if (!gdp) {
> > +               ret = -EIO;
> > +               return ret;
> > +       }
> > +
> >         ret = ext4_mb_load_buddy(sb, group, &e4b);
> >         if (ret) {
> >                 ext4_warning(sb, "Error %d loading buddy
> > information for %u",
> > @@ -7008,11 +7015,10 @@ ext4_trim_all_free(struct super_block *sb,
> > ext4_group_t group,
> > 
> >         ext4_lock_group(sb, group);
> > 
> > -       if (!EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) ||
> > +       if (!(es->s_flags & cpu_to_le16(EXT2_FLAGS_TRACK_TRIM) &&
> > +             gdp->bg_flags & cpu_to_le16(EXT4_BG_TRIMMED)) ||
> >             minblocks < EXT4_SB(sb)->s_last_trim_minblks) {
> 
> I think this should still *send* the TRIM request if BG_TRIMMED is
> not
> set, regardless of whether TRACK_TRIM is set or not, it should just
> not save the flag to disk below.
If BG_TRIMMED is not set, then TRIM request will be sent regardless
already.
Checking TRACK_TRIM here also gives us the option to use it as a
switch: force fstrim everything regardless if the group has BG_TRIMMED
or not.
> 
> >                 ret = ext4_try_to_trim_range(sb, &e4b, start, max,
> > minblocks);
> > -               if (ret >= 0 && set_trimmed)
> > -                       EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info);
> 
> This should clear the "set_trimmed" flag if there was an error, so
> the
> flag is not set below.
We check if ret > 0 below, should be fine here.
> 
> >         } else {
> >                 ret = 0;
> >         }
> > @@ -7020,6 +7026,34 @@ ext4_trim_all_free(struct super_block *sb,
> > ext4_group_t group,
> >         ext4_unlock_group(sb, group);
> >         ext4_mb_unload_buddy(&e4b);
> > 
> > +       if (ret > 0 && set_trimmed) {
> 
> Here, this should check the TRACK_TRIM flag and not force the GDT
> write
> if the feature is disabled.  *Not* writing the flag to disk is fine,
> at
> worst it means that another TRIM would be sent in case of a crash,
> which
> is what happened before this patch.  Only the BG_TRIMMED flag should
> be
> set in the group descriptor in that case, based on the flag saved
> above.
Got it, will update the patch.

Thanks
Dongyang
> 
> Cheers, Andreas
> 
> > +               int err;
> > +               handle_t *handle;
> > +
> > +               handle = ext4_journal_start_sb(sb, EXT4_HT_FS_TRIM,
> > 1);
> > +               if (IS_ERR(handle)) {
> > +                       ret = PTR_ERR(handle);
> > +                       goto out_return;
> > +               }
> > +               err = ext4_journal_get_write_access(handle, sb,
> > gd_bh,
> > +                                                   EXT4_JTR_NONE);
> > +               if (err) {
> > +                       ret = err;
> > +                       goto out_journal;
> > +               }
> > +               ext4_lock_group(sb, group);
> > +               gdp->bg_flags |= cpu_to_le16(EXT4_BG_TRIMMED);
> > +               ext4_group_desc_csum_set(sb, group, gdp);
> > +               ext4_unlock_group(sb, group);
> > +               err = ext4_handle_dirty_metadata(handle, NULL,
> > gd_bh);
> > +               if (err)
> > +                       ret = err;
> > +out_journal:
> > +               err = ext4_journal_stop(handle);
> > +               if (err)
> > +                       ret = err;
> > +       }
> > +out_return:
> >         ext4_debug("trimmed %d blocks in the group %d\n",
> >                 ret, group);
> > 
> > --
> > 2.41.0
> > 
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
>