| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Apr 2022 09:26:15 +0800
From: Chao Yu <chao@...nel.org>
To: Jaegeuk Kim <jaegeuk@...nel.org>
Cc: linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, Chao Yu <chao.yu@...o.com>
Subject: Re: [PATCH v3] f2fs: give priority to select unpinned section for
foreground GC
On 2022/4/13 0:21, Jaegeuk Kim wrote:
> On 04/12, Chao Yu wrote:
>> On 2022/4/12 4:20, Jaegeuk Kim wrote:
>>> On 04/06, Chao Yu wrote:
>>>> Previously, during foreground GC, if victims contain data of pinned file,
>>>> it will fail migration of the data, and meanwhile i_gc_failures of that
>>>> pinned file may increase, and when it exceeds threshold, GC will unpin
>>>> the file, result in breaking pinfile's semantics.
>>>>
>>>> In order to mitigate such condition, let's record and skip section which
>>>> has pinned file's data and give priority to select unpinned one.
>>>>
>>>> Signed-off-by: Chao Yu <chao.yu@...o.com>
>>>> ---
>>>> v3:
>>>> - check pin status before pinning section in pin_section().
>>>> fs/f2fs/gc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> fs/f2fs/segment.c | 7 ++++++
>>>> fs/f2fs/segment.h | 2 ++
>>>> 3 files changed, 62 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index 6a7e4148ff9d..df23824ae3c2 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>>> f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>>> }
>>>> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>>>
>>> Need f2fs_...?
>>
>> Sure, I can add prefix...
>>
>> It's a local function, it won't pollute global namespace w/o f2fs_ prefix
>> though.
>>
>>>
>>>> +{
>>>> + struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> + unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>>> +
>>>> + if (test_bit(secno, dirty_i->pinned_secmap))
>>>> + return;
>>>> + set_bit(secno, dirty_i->pinned_secmap);
>>>> + dirty_i->pinned_secmap_cnt++;
>>>> +}
>>>> +
>>>> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
>>>> +{
>>>> + return dirty_i->pinned_secmap_cnt;
>>>> +}
>>>> +
>>>> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
>>>> + unsigned int secno)
>>>> +{
>>>> + return pinned_section_exists(dirty_i) &&
>>>> + test_bit(secno, dirty_i->pinned_secmap);
>>>> +}
>>>> +
>>>> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
>>>> +{
>>>> + unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>>>> +
>>>> + memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>>>> + DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>>>> +}
>>>> +
>>>> /*
>>>> * This function is called from two paths.
>>>> * One is garbage collection and the other is SSR segment selection.
>>>> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>>> if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>>> goto next;
>>>> + if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
>>>> + goto next;
>>>> +
>>>> if (is_atgc) {
>>>> add_victim_entry(sbi, &p, segno);
>>>> goto next;
>>>> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>>> }
>>>> if (f2fs_is_pinned_file(inode)) {
>>>> - if (gc_type == FG_GC)
>>>> + if (gc_type == FG_GC) {
>>>> f2fs_pin_file_control(inode, true);
>>>> + pin_section(F2FS_I_SB(inode), segno);
>>>
>>> Do we need to check unpinning the inode?
>>> if (!f2fs_pin_file_control())
>>> f2fs_set_pin_section();
>>
>> I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
>> so could you please check below logic:
>>
>> From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
>> From: Chao Yu <chao@...nel.org>
>> Date: Wed, 6 Apr 2022 23:26:51 +0800
>> Subject: [PATCH v4] f2fs: give priority to select unpinned section for
>> foreground GC
>>
>> Previously, during foreground GC, if victims contain data of pinned file,
>> it will fail migration of the data, and meanwhile i_gc_failures of that
>> pinned file may increase, and when it exceeds threshold, GC will unpin
>> the file, result in breaking pinfile's semantics.
>>
>> In order to mitigate such condition, let's record and skip section which
>> has pinned file's data and give priority to select unpinned one.
>>
>> Signed-off-by: Chao Yu <chao.yu@...o.com>
>> ---
>> v4:
>> - add f2fs_ prefix for newly introduced functions
>> - add bool type variable for functionality switch
>> - increase GC_FAILURE_PIN only if it disable pinning section
>> fs/f2fs/gc.c | 66 ++++++++++++++++++++++++++++++++++++++++++-----
>> fs/f2fs/segment.c | 8 ++++++
>> fs/f2fs/segment.h | 3 +++
>> 3 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6a7e4148ff9d..296b31e28d3d 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>> f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>> }
>>
>> +static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>> +{
>> + struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>> + unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>> +
>> + if (!dirty_i->enable_pin_section)
>> + return false;
>> + if (test_bit(secno, dirty_i->pinned_secmap))
>> + return true;
>> + set_bit(secno, dirty_i->pinned_secmap);
>> + dirty_i->pinned_secmap_cnt++;
>
> if (!test_and_set_bit())
> dirty_i->pinned_secmap_cnt++;
More clean.
>
>> + return true;
>> +}
>> +
>> +static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
>> +{
>> + return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
>> +}
>> +
>> +static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
>> + unsigned int secno)
>> +{
>> + return f2fs_pinned_section_exists(dirty_i) &&
>> + test_bit(secno, dirty_i->pinned_secmap);
>> +}
>> +
>> +static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
>> +{
>> + unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>> +
>> + memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>> + DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>> + DIRTY_I(sbi)->enable_pin_section = enable;
>> +}
>> +
>> /*
>> * This function is called from two paths.
>> * One is garbage collection and the other is SSR segment selection.
>> @@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>> if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>> goto next;
>>
>> + if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
>> + goto next;
>> +
>> if (is_atgc) {
>> add_victim_entry(sbi, &p, segno);
>> goto next;
>> @@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>> }
>>
>
> Can we make a generic function?
>
> f2fs_gc_pinned_control()
> {
> if (!f2fs_is_pinned_file(inode))
> return 0;
> if (gc_type != FG_GC)
> return 0;
> if (!f2fs_pin_section())
> f2fs_pin_file_control();
> return -EAGAIN;
> }
Better. :)
Thanks,
>
>> if (f2fs_is_pinned_file(inode)) {
>> - if (gc_type == FG_GC)
>> - f2fs_pin_file_control(inode, true);
>> + if (gc_type == FG_GC) {
>> + if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
>
>> + f2fs_pin_file_control(inode, true);
>> + }
>> err = -EAGAIN;
>> goto out;
>> }
>> @@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>> goto out;
>> }
>> if (f2fs_is_pinned_file(inode)) {
>> - if (gc_type == FG_GC)
>> - f2fs_pin_file_control(inode, true);
>> + if (gc_type == FG_GC) {
>> + if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
>> + f2fs_pin_file_control(inode, true);
>> + }
>> err = -EAGAIN;
>> goto out;
>> }
>> @@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>
>> if (is_inode_flag_set(inode, FI_PIN_FILE) &&
>> gc_type == FG_GC) {
>> - f2fs_pin_file_control(inode, true);
>> + if (!f2fs_pin_section(sbi, segno))
>> + f2fs_pin_file_control(inode, true);
>> iput(inode);
>> return submitted;
>> }
>> @@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>> ret = -EINVAL;
>> goto stop;
>> }
>> +retry:
>> ret = __get_victim(sbi, &segno, gc_type);
>> - if (ret)
>> + if (ret) {
>> + /* allow to search victim from sections has pinned data */
>> + if (ret == -ENODATA && gc_type == FG_GC &&
>> + f2fs_pinned_section_exists(DIRTY_I(sbi))) {
>> + f2fs_unpin_all_sections(sbi, false);
>> + goto retry;
>> + }
>> goto stop;
>> + }
>>
>> seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>> if (gc_type == FG_GC &&
>> @@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>> SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>> SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>
>> + if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
>> + f2fs_unpin_all_sections(sbi, true);
>> +
>> trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>> get_pages(sbi, F2FS_DIRTY_NODES),
>> get_pages(sbi, F2FS_DIRTY_DENTS),
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 22dfeb991529..93c7bae57a25 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>> dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> if (!dirty_i->victim_secmap)
>> return -ENOMEM;
>> +
>> + dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> + if (!dirty_i->pinned_secmap)
>> + return -ENOMEM;
>> +
>> + dirty_i->pinned_secmap_cnt = 0;
>> + dirty_i->enable_pin_section = true;
>> return 0;
>> }
>>
>> @@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>> {
>> struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>
>> + kvfree(dirty_i->pinned_secmap);
>> kvfree(dirty_i->victim_secmap);
>> }
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 5c94caf0c0a1..8a591455d796 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -294,6 +294,9 @@ struct dirty_seglist_info {
>> struct mutex seglist_lock; /* lock for segment bitmaps */
>> int nr_dirty[NR_DIRTY_TYPE]; /* # of dirty segments */
>> unsigned long *victim_secmap; /* background GC victims */
>> + unsigned long *pinned_secmap; /* pinned victims from foreground GC */
>> + unsigned int pinned_secmap_cnt; /* count of victims which has pinned data */
>> + bool enable_pin_section; /* enable pinning section */
>> };
>>
>> /* victim selection function for cleaning and SSR */
>> --
>> 2.25.1
>>
>> Thanks,
>>
>>>
>>>> + }
>>>> err = -EAGAIN;
>>>> goto out;
>>>> }
>>>> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>> goto out;
>>>> }
>>>> if (f2fs_is_pinned_file(inode)) {
>>>> - if (gc_type == FG_GC)
>>>> + if (gc_type == FG_GC) {
>>>> f2fs_pin_file_control(inode, true);
>>>> + pin_section(F2FS_I_SB(inode), segno);
>>>> + }
>>>> err = -EAGAIN;
>>>> goto out;
>>>> }
>>>> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>> gc_type == FG_GC) {
>>>> f2fs_pin_file_control(inode, true);
>>>> iput(inode);
>>>> + pin_section(sbi, segno);
>>>
>>> We don't have this code.
>>>
>>>> return submitted;
>>>> }
>>>> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>> ret = -EINVAL;
>>>> goto stop;
>>>> }
>>>> +retry:
>>>> ret = __get_victim(sbi, &segno, gc_type);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + /* allow to search victim from sections has pinned data */
>>>> + if (ret == -ENODATA && gc_type == FG_GC &&
>>>> + pinned_section_exists(DIRTY_I(sbi))) {
>>>> + unpin_all_sections(sbi);
>>>> + goto retry;
>>>> + }
>>>> goto stop;
>>>> + }
>>>> seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>>> if (gc_type == FG_GC &&
>>>> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>> SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>>> SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>>> + if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
>>>> + unpin_all_sections(sbi);
>>>> +
>>>> trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>>> get_pages(sbi, F2FS_DIRTY_NODES),
>>>> get_pages(sbi, F2FS_DIRTY_DENTS),
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 012524db7437..1c20d7c9eca3 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>>> dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>> if (!dirty_i->victim_secmap)
>>>> return -ENOMEM;
>>>> +
>>>> + dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>> + if (!dirty_i->pinned_secmap)
>>>> + return -ENOMEM;
>>>> +
>>>> + dirty_i->pinned_secmap_cnt = 0;
>>>> return 0;
>>>> }
>>>> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>>> {
>>>> struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> + kvfree(dirty_i->pinned_secmap);
>>>> kvfree(dirty_i->victim_secmap);
>>>> }
>>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>>> index 5c94caf0c0a1..fd6f246e649c 100644
>>>> --- a/fs/f2fs/segment.h
>>>> +++ b/fs/f2fs/segment.h
>>>> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>>>> struct mutex seglist_lock; /* lock for segment bitmaps */
>>>> int nr_dirty[NR_DIRTY_TYPE]; /* # of dirty segments */
>>>> unsigned long *victim_secmap; /* background GC victims */
>>>> + unsigned long *pinned_secmap; /* pinned victims from foreground GC */
>>>> + unsigned int pinned_secmap_cnt; /* count of victims which has pinned data */
>>>> };
>>>> /* victim selection function for cleaning and SSR */
>>>> --
>>>> 2.32.0
Powered by blists - more mailing lists