[PATCH] ext4: Make running and commit transaction have their own freed_data_list

[PATCH] ext4: Make running and commit transaction have their own freed_data_list

[Jinke Han]

From: Jinke Han <hanjinke.666@bytedance.com>

When releasing space in jbd, we traverse s_freed_data_list to get the
free range belonging to the current commit transaction. In extreme cases,
the time spent may not be small, and we have observed cases exceeding
10ms. This patch makes running and commit transactions manage their own
free_data_list respectively, eliminating unnecessary traversal.

And in the callback phase of the commit transaction, no one will touch
it except the jbd thread itself, so s_md_lock is no longer needed.

Signed-off-by: Jinke Han <hanjinke.666@bytedance.com>
---
 fs/ext4/ext4.h    |  2 +-
 fs/ext4/mballoc.c | 19 +++++--------------
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 813b4da098a0..356905357dc9 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1557,7 +1557,7 @@ struct ext4_sb_info {
 	unsigned int *s_mb_maxs;
 	unsigned int s_group_info_size;
 	unsigned int s_mb_free_pending;
-	struct list_head s_freed_data_list;	/* List of blocks to be freed
+	struct list_head s_freed_data_list[2];	/* List of blocks to be freed
 						   after commit completed */
 	struct list_head s_discard_list;
 	struct work_struct s_discard_work;
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 4f2a1df98141..8fab5720a979 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3626,7 +3626,8 @@ int ext4_mb_init(struct super_block *sb)
 
 	spin_lock_init(&sbi->s_md_lock);
 	sbi->s_mb_free_pending = 0;
-	INIT_LIST_HEAD(&sbi->s_freed_data_list);
+	INIT_LIST_HEAD(&sbi->s_freed_data_list[0]);
+	INIT_LIST_HEAD(&sbi->s_freed_data_list[1]);
 	INIT_LIST_HEAD(&sbi->s_discard_list);
 	INIT_WORK(&sbi->s_discard_work, ext4_discard_work);
 	atomic_set(&sbi->s_retry_alloc_pending, 0);
@@ -3878,21 +3879,11 @@ void ext4_process_freed_data(struct super_block *sb, tid_t commit_tid)
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_free_data *entry, *tmp;
 	struct list_head freed_data_list;
-	struct list_head *cut_pos = NULL;
+	struct list_head *s_freed_head = &sbi->s_freed_data_list[commit_tid & 1];
 	bool wake;
 
 	INIT_LIST_HEAD(&freed_data_list);
-
-	spin_lock(&sbi->s_md_lock);
-	list_for_each_entry(entry, &sbi->s_freed_data_list, efd_list) {
-		if (entry->efd_tid != commit_tid)
-			break;
-		cut_pos = &entry->efd_list;
-	}
-	if (cut_pos)
-		list_cut_position(&freed_data_list, &sbi->s_freed_data_list,
-				  cut_pos);
-	spin_unlock(&sbi->s_md_lock);
+	list_replace_init(s_freed_head, &freed_data_list);
 
 	list_for_each_entry(entry, &freed_data_list, efd_list)
 		ext4_free_data_in_buddy(sb, entry);
@@ -6298,7 +6289,7 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
 	}
 
 	spin_lock(&sbi->s_md_lock);
-	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list);
+	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list[new_entry->efd_tid & 1]);
 	sbi->s_mb_free_pending += clusters;
 	spin_unlock(&sbi->s_md_lock);
 }
-- 
2.20.1

Re: [PATCH] ext4: Make running and commit transaction have their own freed_data_list

[Zhang Yi]

On 2023/6/12 20:40, Jinke Han wrote:
> From: Jinke Han <hanjinke.666@bytedance.com>
> 
> When releasing space in jbd, we traverse s_freed_data_list to get the
> free range belonging to the current commit transaction. In extreme cases,
> the time spent may not be small, and we have observed cases exceeding
> 10ms. This patch makes running and commit transactions manage their own
> free_data_list respectively, eliminating unnecessary traversal.
> 
> And in the callback phase of the commit transaction, no one will touch
> it except the jbd thread itself, so s_md_lock is no longer needed.
> 
> Signed-off-by: Jinke Han <hanjinke.666@bytedance.com>

Thanks for the patch, it looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/ext4.h    |  2 +-
>  fs/ext4/mballoc.c | 19 +++++--------------
>  2 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 813b4da098a0..356905357dc9 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1557,7 +1557,7 @@ struct ext4_sb_info {
>  	unsigned int *s_mb_maxs;
>  	unsigned int s_group_info_size;
>  	unsigned int s_mb_free_pending;
> -	struct list_head s_freed_data_list;	/* List of blocks to be freed
> +	struct list_head s_freed_data_list[2];	/* List of blocks to be freed
>  						   after commit completed */
>  	struct list_head s_discard_list;
>  	struct work_struct s_discard_work;
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4f2a1df98141..8fab5720a979 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3626,7 +3626,8 @@ int ext4_mb_init(struct super_block *sb)
>  
>  	spin_lock_init(&sbi->s_md_lock);
>  	sbi->s_mb_free_pending = 0;
> -	INIT_LIST_HEAD(&sbi->s_freed_data_list);
> +	INIT_LIST_HEAD(&sbi->s_freed_data_list[0]);
> +	INIT_LIST_HEAD(&sbi->s_freed_data_list[1]);
>  	INIT_LIST_HEAD(&sbi->s_discard_list);
>  	INIT_WORK(&sbi->s_discard_work, ext4_discard_work);
>  	atomic_set(&sbi->s_retry_alloc_pending, 0);
> @@ -3878,21 +3879,11 @@ void ext4_process_freed_data(struct super_block *sb, tid_t commit_tid)
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct ext4_free_data *entry, *tmp;
>  	struct list_head freed_data_list;
> -	struct list_head *cut_pos = NULL;
> +	struct list_head *s_freed_head = &sbi->s_freed_data_list[commit_tid & 1];
>  	bool wake;
>  
>  	INIT_LIST_HEAD(&freed_data_list);
> -
> -	spin_lock(&sbi->s_md_lock);
> -	list_for_each_entry(entry, &sbi->s_freed_data_list, efd_list) {
> -		if (entry->efd_tid != commit_tid)
> -			break;
> -		cut_pos = &entry->efd_list;
> -	}
> -	if (cut_pos)
> -		list_cut_position(&freed_data_list, &sbi->s_freed_data_list,
> -				  cut_pos);
> -	spin_unlock(&sbi->s_md_lock);
> +	list_replace_init(s_freed_head, &freed_data_list);
>  
>  	list_for_each_entry(entry, &freed_data_list, efd_list)
>  		ext4_free_data_in_buddy(sb, entry);
> @@ -6298,7 +6289,7 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
>  	}
>  
>  	spin_lock(&sbi->s_md_lock);
> -	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list);
> +	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list[new_entry->efd_tid & 1]);
>  	sbi->s_mb_free_pending += clusters;
>  	spin_unlock(&sbi->s_md_lock);
>  }
> 

Re: [PATCH] ext4: Make running and commit transaction have their own freed_data_list

[hanjinke]

Hi, Ted

Can you consider this patch？ As it was already Reviewed by Zhang Yi.

Thanks
Jinke

在 2023/6/12 下午8:40, Jinke Han 写道:
> From: Jinke Han <hanjinke.666@bytedance.com>
> 
> When releasing space in jbd, we traverse s_freed_data_list to get the
> free range belonging to the current commit transaction. In extreme cases,
> the time spent may not be small, and we have observed cases exceeding
> 10ms. This patch makes running and commit transactions manage their own
> free_data_list respectively, eliminating unnecessary traversal.
> 
> And in the callback phase of the commit transaction, no one will touch
> it except the jbd thread itself, so s_md_lock is no longer needed.
> 
> Signed-off-by: Jinke Han <hanjinke.666@bytedance.com>
> ---
>   fs/ext4/ext4.h    |  2 +-
>   fs/ext4/mballoc.c | 19 +++++--------------
>   2 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 813b4da098a0..356905357dc9 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1557,7 +1557,7 @@ struct ext4_sb_info {
>   	unsigned int *s_mb_maxs;
>   	unsigned int s_group_info_size;
>   	unsigned int s_mb_free_pending;
> -	struct list_head s_freed_data_list;	/* List of blocks to be freed
> +	struct list_head s_freed_data_list[2];	/* List of blocks to be freed
>   						   after commit completed */
>   	struct list_head s_discard_list;
>   	struct work_struct s_discard_work;
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4f2a1df98141..8fab5720a979 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3626,7 +3626,8 @@ int ext4_mb_init(struct super_block *sb)
>   
>   	spin_lock_init(&sbi->s_md_lock);
>   	sbi->s_mb_free_pending = 0;
> -	INIT_LIST_HEAD(&sbi->s_freed_data_list);
> +	INIT_LIST_HEAD(&sbi->s_freed_data_list[0]);
> +	INIT_LIST_HEAD(&sbi->s_freed_data_list[1]);
>   	INIT_LIST_HEAD(&sbi->s_discard_list);
>   	INIT_WORK(&sbi->s_discard_work, ext4_discard_work);
>   	atomic_set(&sbi->s_retry_alloc_pending, 0);
> @@ -3878,21 +3879,11 @@ void ext4_process_freed_data(struct super_block *sb, tid_t commit_tid)
>   	struct ext4_sb_info *sbi = EXT4_SB(sb);
>   	struct ext4_free_data *entry, *tmp;
>   	struct list_head freed_data_list;
> -	struct list_head *cut_pos = NULL;
> +	struct list_head *s_freed_head = &sbi->s_freed_data_list[commit_tid & 1];
>   	bool wake;
>   
>   	INIT_LIST_HEAD(&freed_data_list);
> -
> -	spin_lock(&sbi->s_md_lock);
> -	list_for_each_entry(entry, &sbi->s_freed_data_list, efd_list) {
> -		if (entry->efd_tid != commit_tid)
> -			break;
> -		cut_pos = &entry->efd_list;
> -	}
> -	if (cut_pos)
> -		list_cut_position(&freed_data_list, &sbi->s_freed_data_list,
> -				  cut_pos);
> -	spin_unlock(&sbi->s_md_lock);
> +	list_replace_init(s_freed_head, &freed_data_list);
>   
>   	list_for_each_entry(entry, &freed_data_list, efd_list)
>   		ext4_free_data_in_buddy(sb, entry);
> @@ -6298,7 +6289,7 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
>   	}
>   
>   	spin_lock(&sbi->s_md_lock);
> -	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list);
> +	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list[new_entry->efd_tid & 1]);
>   	sbi->s_mb_free_pending += clusters;
>   	spin_unlock(&sbi->s_md_lock);
>   }

Re: [PATCH] ext4: Make running and commit transaction have their own freed_data_list

[Theodore Ts'o]

On Mon, 12 Jun 2023 20:40:17 +0800, Jinke Han wrote:
> When releasing space in jbd, we traverse s_freed_data_list to get the
> free range belonging to the current commit transaction. In extreme cases,
> the time spent may not be small, and we have observed cases exceeding
> 10ms. This patch makes running and commit transactions manage their own
> free_data_list respectively, eliminating unnecessary traversal.
> 
> And in the callback phase of the commit transaction, no one will touch
> it except the jbd thread itself, so s_md_lock is no longer needed.
> 
> [...]

Applied, thanks!

[1/1] ext4: Make running and commit transaction have their own freed_data_list
      commit: ce774e5365e46be73ed055302c6de123a03394ea

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>