Re: [PATCH] f2fs: enable atgc if atgc_age_threshold from user is less than elapsed_time
From: Chao Yu
Date: Wed Jun 05 2024 - 02:26:51 EST
On 2024/6/5 13:59, Zhiguo Niu wrote:
On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@xxxxxxxxxx> wrote:
On 2024/6/3 17:05, Zhiguo Niu wrote:
On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@xxxxxxxxxx> wrote:
On 2024/5/20 19:36, Zhiguo Niu wrote:
Now atgc can be enabled based on the following conditions:
-ATGC mount option is set
-elapsed_time is more than atgc_age_threshold already
but these conditions are check when umounted->mounted device again.
If the device has not be umounted->mounted for a long time, atgc can
not work even the above conditions met.
It is better to enable atgc dynamiclly when user change atgc_age_threshold
meanwhile this vale is less than elapsed_time and ATGC mount option is on.
Signed-off-by: Zhiguo Niu <zhiguo.niu@xxxxxxxxxx>
---
fs/f2fs/f2fs.h | 1 +
fs/f2fs/segment.c | 9 ++++-----
fs/f2fs/sysfs.c | 16 ++++++++++++++++
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 1974b6a..e441d2d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3694,6 +3694,7 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
+int f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi);
int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
unsigned int start, unsigned int end);
int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, bool force);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 71dc8042..44923d4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2931,14 +2931,11 @@ static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
return ret;
}
-static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
+int f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
{
struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
int ret = 0;
- if (!sbi->am.atgc_enabled)
- return 0;
-
f2fs_down_read(&SM_I(sbi)->curseg_lock);
mutex_lock(&curseg->curseg_mutex);
@@ -2955,7 +2952,9 @@ static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
}
int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
{
- return __f2fs_init_atgc_curseg(sbi);
+ if (!sbi->am.atgc_enabled)
+ return 0;
+ return f2fs_init_atgc_curseg(sbi);
}
static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 09d3ecf..72676c5 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -673,6 +673,22 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
return count;
}
+ if (!strcmp(a->attr.name, "atgc_age_threshold")) {
+ if (t < 0)
+ return -EINVAL;
+ sbi->am.age_threshold = t;
+ if (sbi->am.atgc_enabled)
+ return count;
+
+ if (test_opt(sbi, ATGC) &&
+ le64_to_cpu(sbi->ckpt->elapsed_time) >= t) {
Hi Chao,
Oh, I guess you want to fix this:
Yes, Sorry for not making it clear before.
static void init_atgc_management(struct f2fs_sb_info *sbi)
{
...
if (test_opt(sbi, ATGC) &&
SIT_I(sbi)->elapsed_time >= DEF_GC_THREAD_AGE_THRESHOLD)
am->atgc_enabled = true;
What about enabling atgc_enabled during checkpoint once elapsed time is
satisfed w/ the condition? I guess this can give another chance whenever
CP is been triggered, and it can avoid the racing condition as well.
1. I'm not sure if this will increase checkpoint time consumption?
Since it won't increase any IO time, I guess it's fine tolerate time consumed
by initialization of atgc curseg.
2. dynamically enabling atgc may have other problems. Is this confirmed?
I think so, checkpoint has avoided most race cases.
So, how do you think of below diff:
Hi Chao,
I think flow is ok, some details need to be discussed.
---
fs/f2fs/checkpoint.c | 2 ++
fs/f2fs/f2fs.h | 1 +
fs/f2fs/segment.c | 26 +++++++++++++++++++++++---
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 55d444bec5c0..4a73bd481a25 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1718,6 +1718,8 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
}
f2fs_restore_inmem_curseg(sbi);
+ f2fs_reinit_atgc_curseg(sbi);
+
stat_inc_cp_count(sbi);
stop:
unblock_operations(sbi);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9688df332147..8d385a1c75ad 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3693,6 +3693,7 @@ void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
+int f2fs_reinit_atgc_curseg(struct f2fs_sb_info *sbi);
void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index bdc2247387e8..6d4273faf061 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2949,12 +2949,12 @@ static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
return ret;
}
-static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
+static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi, bool force)
{
struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
int ret = 0;
- if (!sbi->am.atgc_enabled)
+ if (!sbi->am.atgc_enabled && !force)
Is this parameter "force" unnecessary?
in mount flow, even atgc enable conditions are all met, it is not
enabled because of force=false.
Or am I missing something?
return 0;
f2fs_down_read(&SM_I(sbi)->curseg_lock);
@@ -2971,9 +2971,29 @@ static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
f2fs_up_read(&SM_I(sbi)->curseg_lock);
return ret;
}
+
int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
{
- return __f2fs_init_atgc_curseg(sbi);
+ return __f2fs_init_atgc_curseg(sbi, false);
+}
+
+int f2fs_reinit_atgc_curseg(struct f2fs_sb_info *sbi)
+{
+ int ret;
+
+ if (!test_opt(sbi, ATGC))
+ return 0;
+ if (sbi->am.atgc_enabled)
+ return 0;
+ if (SIT_I(sbi)->elapsed_time < sbi->am.age_threshold)
SIT(sbi)->elapsed_time is just updated in mount flow, so
ckpt->elapsed_time is more suitable here?
Agreed, it needs to be fixed.
Can you please update those changes into v2?
Thanks,
thanks!
+ return 0;
+
+ ret = __f2fs_init_atgc_curseg(sbi, true);
+ if (!ret) {
+ sbi->am.atgc_enabled = true;
+ f2fs_info(sbi, "reenabled age threshold GC");
+ }
+ return ret;
}
static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
--
2.40.1
thanks！
Thanks,
+ if (f2fs_init_atgc_curseg(sbi))
+ return -EINVAL;
+ sbi->am.atgc_enabled = true;
+ }
+ return count;
+ }
+
if (!strcmp(a->attr.name, "gc_segment_mode")) {
if (t < MAX_GC_MODE)
sbi->gc_segment_mode = t;