linux-kernel - Re: [PATCH 2/5] f2fs: clear the remaining prefree

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9e5ce0bb-22ed-6b30-4592-8d7e371f31a4@huawei.com>
Date:   Fri, 13 Jul 2018 16:59:47 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Yunlong Song <yunlong.song@...wei.com>, <jaegeuk@...nel.org>,
        <chao@...nel.org>, <yunlong.song@...oud.com>
CC:     <miaoxie@...wei.com>, <bintian.wang@...wei.com>,
        <shengyong1@...wei.com>, <heyunlei@...wei.com>,
        <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] f2fs: clear the remaining prefree_map of the section

On 2018/7/13 11:51, Yunlong Song wrote:
> Because in f2fs_clear_prefree_segments, the codes:
> ...
>      while (1) {
>          int i;
>          start = find_next_bit(prefree_map, MAIN_SEGS(sbi), end + 1);
>          if (start >= MAIN_SEGS(sbi))
>              break;
>          end = find_next_zero_bit(prefree_map, MAIN_SEGS(sbi),
>                                  start + 1);
> 
>          for (i = start; i < end; i++)
>              clear_bit(i, prefree_map);
> ...
> next:
>          secno = GET_SEC_FROM_SEG(sbi, start);
>          start_segno = GET_SEG_FROM_SEC(sbi, secno);
>          if (!IS_CURSEC(sbi, secno) &&
>              !get_valid_blocks(sbi, start, true))
>              f2fs_issue_discard(sbi, START_BLOCK(sbi, start_segno),
>                  sbi->segs_per_sec << sbi->log_blocks_per_seg);
> 
>          start = start_segno + sbi->segs_per_sec;
>          if (start < end)
>              goto next;
>          else
>              end = start - 1;
> ...
> In round 2, for prefree_map: 1 1 0 1 1, start = 0, end = 2, then
> 
> start = start_segno + sbi->segs_per_sec makes start = 5
> 
> if (start < end)  --> start = 5, end = 2
> 
> so end = start -1  --> end = 4, then return to while again, this time skips
> prefree bit 3 and 4.

I got it, thanks for the explanation.

If we are in LFS mode & with big section, what about aligning start/end to
section boundary for fstrim or real-time discard operation, and decide to issue
discard only when whole section is invalid and the last segment in section is
freed in current checkpoint? so that we can issue discard aligned to section
size as much as possible and avoid redundant discard.

Thanks,

> 
> On 2018/7/13 11:42, Chao Yu wrote:
>> On 2018/7/13 11:28, Yunlong Song wrote:
>>> round 1: section bitmap : 1 1 1 1 1, all valid, prefree_map: 0 0 0 0 0
>>> then rm data block NO.2, block NO.2 becomes invalid, prefree_map: 0 0 1 0 0
>>> write_checkpoint: section bitmap: 1 1 0 1 1, prefree_map: 0 0 0 0 0,
>>> prefree of NO.2 is cleared, and no discard issued
>>>
>>> round2: rm data block NO.0, NO.1, NO.3, NO.4
>>> all invalid, but prefree bit of NO.2 is set and cleared in round1, then
>>> prefree_map: 1 1 0 1 1
>>> write_checkpoint: section bitmap: 0 0 0 0 0, prefree_map: 0 0 0 1 1, no
>> Why prefree_map is not 0 0 0 0 0?
>>
>> Thanks,
>>
>>> valid blocks of this section, so discard issued
>>> but this time prefree bit of NO.3 and NO.4 is skipped...
>>>
>>> round3:
>>> write_checkpoint: section bitmap: 0 0 0 0 0, prefree_map: 0 0 0 1 1 - >
>>> 0 0 0 0 0, no valid blocks of this section, so discard issued
>>> this time prefree bit of NO.3 and NO.4 is cleared, but the discard of
>>> this section is sent again...
>>>
>>> On 2018/7/13 11:13, Chao Yu wrote:
>>>> On 2018/7/12 23:09, Yunlong Song wrote:
>>>>> For the case when sbi->segs_per_sec > 1, take section:segment = 5 for
>>>>> example, if the section prefree_map is ...previous section | current
>>>>> section (1 1 0 1 1) | next section..., then the start = x, end = x + 1,
>>>>> after start = start_segno + sbi->segs_per_sec, start = x + 5, then it
>>>>> will skip x + 3 and x + 4, but their bitmap is still set, which will
>>>>> cause duplicated f2fs_issue_discard of this same section in the next
>>>>> write_checkpoint, so fix it.
>>>> I didn't get it, if # 2 segment is not prefree state, so it still has valid
>>>> blocks there, so we won't issue discard due to below condition, right?
>>>>
>>>> 		if (!IS_CURSEC(sbi, secno) &&
>>>> 			!get_valid_blocks(sbi, start, true))
>>>>
>>>> Thanks,
>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.song@...wei.com>
>>>>> ---
>>>>>    fs/f2fs/segment.c | 19 +++++++++++++++++--
>>>>>    1 file changed, 17 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>> index 47b6595..fd38b61 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -1684,8 +1684,23 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
>>>>>    		start = start_segno + sbi->segs_per_sec;
>>>>>    		if (start < end)
>>>>>    			goto next;
>>>>> -		else
>>>>> -			end = start - 1;
>>>>> +		else {
>>>>> +			start_segno = start;
>>>>> +
>>>>> +			while (1) {
>>>>> +				start = find_next_bit(prefree_map, start_segno,
>>>>> +									end + 1);
>>>>> +				if (start >= start_segno)
>>>>> +					break;
>>>>> +				end = find_next_zero_bit(prefree_map, start_segno,
>>>>> +										start + 1);
>>>>> +				for (i = start; i < end; i++)
>>>>> +					clear_bit(i, prefree_map);
>>>>> +				dirty_i->nr_dirty[PRE] -= end - start;
>>>>> +			}
>>>>> +
>>>>> +			end = start_segno - 1;
>>>>> +		}
>>>>>    	}
>>>>>    	mutex_unlock(&dirty_i->seglist_lock);
>>>>>    
>>>>>
>>>> .
>>>>
>>
>> .
>>
>