| Message ID | 1716204978-29455-1-git-send-email-zhiguo.niu@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] f2fs: enable atgc if atgc_age_threshold from user is less than elapsed_time | expand |
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. Zhiguo, I didn't get it, can you please explain more about this issue? Thanks, > > 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@unisoc.com> > --- > 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) { > + 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;
On Mon, May 27, 2024 at 12:07 PM Zhiguo Niu <niuzhiguo84@gmail.com> wrote: > > On Mon, May 27, 2024 at 11:49 AM Chao Yu <chao@kernel.org> 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. > > > > Zhiguo, I didn't get it, can you please explain more about this issue? > > > > Thanks, > Hi Chao, > > At present, the point of atgc enale is checked during the mount > process. What I mean is that if a device has not been rebooted > (re-mounted) for a long time, even if the above two conditions are > met(ATGC mount option is set, and the device has been powered on long > enough， more than atgc default threshold ), atgc cannot be dynamically > enabled. > > If the user is used to not restarting the phone after turning it on, > it will be difficult to use atgc. > thanks! Hi Chao, Do you have any suggestions or comments on this? thanks！ > > > > > > > > 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@unisoc.com> > > > --- > > > 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) { > > > + 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;
On 2024/5/30 17:49, Zhiguo Niu wrote: > On Mon, May 27, 2024 at 12:07 PM Zhiguo Niu <niuzhiguo84@gmail.com> wrote: >> >> On Mon, May 27, 2024 at 11:49 AM Chao Yu <chao@kernel.org> 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. >>> >>> Zhiguo, I didn't get it, can you please explain more about this issue? >>> >>> Thanks, >> Hi Chao, >> >> At present, the point of atgc enale is checked during the mount >> process. What I mean is that if a device has not been rebooted >> (re-mounted) for a long time, even if the above two conditions are >> met(ATGC mount option is set, and the device has been powered on long >> enough， more than atgc default threshold ), atgc cannot be dynamically >> enabled. >> >> If the user is used to not restarting the phone after turning it on, >> it will be difficult to use atgc. >> thanks! > Hi Chao, > Do you have any suggestions or comments on this? Zhiguo, I remember that atgc can not be enabled at runtime due to some reasons, but I need some time to recall and check the details... Thanks, > thanks！ > >>> >>>> >>>> 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@unisoc.com> >>>> --- >>>> 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) { >>>> + 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;
On Fri, May 31, 2024 at 10:04 AM Chao Yu <chao@kernel.org> wrote: > > On 2024/5/30 17:49, Zhiguo Niu wrote: > > On Mon, May 27, 2024 at 12:07 PM Zhiguo Niu <niuzhiguo84@gmail.com> wrote: > >> > >> On Mon, May 27, 2024 at 11:49 AM Chao Yu <chao@kernel.org> 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. > >>> > >>> Zhiguo, I didn't get it, can you please explain more about this issue? > >>> > >>> Thanks, > >> Hi Chao, > >> > >> At present, the point of atgc enale is checked during the mount > >> process. What I mean is that if a device has not been rebooted > >> (re-mounted) for a long time, even if the above two conditions are > >> met(ATGC mount option is set, and the device has been powered on long > >> enough， more than atgc default threshold ), atgc cannot be dynamically > >> enabled. > >> > >> If the user is used to not restarting the phone after turning it on, > >> it will be difficult to use atgc. > >> thanks! > > Hi Chao, > > Do you have any suggestions or comments on this? > > Zhiguo, > > I remember that atgc can not be enabled at runtime due to some reasons, but > I need some time to recall and check the details... > > Thanks, Hi Chao, OK, Thanks for your help. > > > thanks！ > > > >>> > >>>> > >>>> 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@unisoc.com> > >>>> --- > >>>> 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) { > >>>> + 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;
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@unisoc.com> > --- > 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) { Oh, I guess you want to fix this: 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. 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;
On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> > > --- > > 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? 2. dynamically enabling atgc may have other problems. Is this confirmed? 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;
On 2024/6/3 17:05, Zhiguo Niu wrote: > On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> >>> --- >>> 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: --- 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) 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) + 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)
On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@kernel.org> wrote: > > On 2024/6/3 17:05, Zhiguo Niu wrote: > > On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> > >>> --- > >>> 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? 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;
On Wed, Jun 5, 2024 at 1:59 PM Zhiguo Niu <niuzhiguo84@gmail.com> wrote: > > On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@kernel.org> wrote: > > > > On 2024/6/3 17:05, Zhiguo Niu wrote: > > > On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> > > >>> --- > > >>> 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? hi Chao, sorry, I read && as || , Please ignore the above comment, but Is it still OK without the "force" parameter? thanks！ > > 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? > 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;
On 2024/6/5 13:59, Zhiguo Niu wrote: > On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@kernel.org> wrote: >> >> On 2024/6/3 17:05, Zhiguo Niu wrote: >>> On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> >>>>> --- >>>>> 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;
On Wed, Jun 5, 2024 at 2:26 PM Chao Yu <chao@kernel.org> wrote: > > On 2024/6/5 13:59, Zhiguo Niu wrote: > > On Wed, Jun 5, 2024 at 11:48 AM Chao Yu <chao@kernel.org> wrote: > >> > >> On 2024/6/3 17:05, Zhiguo Niu wrote: > >>> On Mon, Jun 3, 2024 at 3:41 PM Chao Yu <chao@kernel.org> 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@unisoc.com> > >>>>> --- > >>>>> 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? Hi Chao, OK, and I will also verify the basic function. thanks！ > > 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;
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) { + 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;
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@unisoc.com> --- fs/f2fs/f2fs.h | 1 + fs/f2fs/segment.c | 9 ++++----- fs/f2fs/sysfs.c | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-)