| 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: Sat, 9 Dec 2023 17:46:13 +0800
From: Chao Yu <chao@...nel.org>
To: Wu Bo <wubo.oduw@...il.com>, Wu Bo <bo.wu@...o.com>,
Jaegeuk Kim <jaegeuk@...nel.org>
Cc: linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block
situation
On 2023/11/28 20:51, Wu Bo wrote:
>
> On 2023/11/28 14:22, Chao Yu wrote:
>> On 2023/11/17 7:34, Wu Bo wrote:
>>> On 2023/11/11 12:49, Chao Yu wrote:
>>>> On 2023/11/8 21:48, Wu Bo wrote:
>>>>> On 2023/11/7 22:39, Chao Yu wrote:
>>>>>> On 2023/10/30 17:40, Wu Bo wrote:
>>>>>>> If GC victim has pinned block, it can't be recycled.
>>>>>>> And if GC is foreground running, after many failure try, the pinned file
>>>>>>> is expected to be clear pin flag. To enable the section be recycled.
>>>>>>>
>>>>>>> But when fallocate trigger FG_GC, GC can never recycle the pinned
>>>>>>> section. Because GC will go to stop before the failure try meet the
>>>>>>> threshold:
>>>>>>> if (has_enough_free_secs(sbi, sec_freed, 0)) {
>>>>>>> if (!gc_control->no_bg_gc &&
>>>>>>> total_sec_freed < gc_control->nr_free_secs)
>>>>>>> goto go_gc_more;
>>>>>>> goto stop;
>>>>>>> }
>>>>>>>
>>>>>>> So when fallocate trigger FG_GC, at least recycle one.
>>>>>>
>>>>>> Hmm... it may break pinfile's semantics at least on one pinned file?
>>>>>> In this case, I prefer to fail fallocate() rather than unpinning file,
>>>>>> in order to avoid leaving invalid LBA references of unpinned file held
>>>>>> by userspace.
>>>>>
>>>>> As f2fs designed now, FG_GC is able to unpin the pinned file.
>>>>>
>>>>> fallocate() triggered FG_GC, but can't recycle space. It breaks the
>>>>> design logic of FG_GC.
>>>>
>>>> Yes, contradictoriness exists.
>>>>
>>>> IMO, unpin file by GC looks more dangerous, it may cause potential data
>>>> corruption w/ below case:
>>>> 1. app pins file & holds LBAs of data blocks.
>>>> 2. GC unpins file and migrates its data to new LBAs.
>>>> 3. other file reuses previous LBAs.
>>>> 4. app read/write data via previous LBAs.
>>>>
>>>> So I suggest to normalize use of pinfile and do not add more unpin cases
>>>> in filesystem inner processes.
>>>>
>>>>>
>>>>> This issue is happened in Android OTA scenario. fallocate() always
>>>>> return failure cause OTA fail.
>>>>
>>>> Can you please check why other pinned files were so fragmented that f2fs_gc()
>>>> can not recycle one free section?
>>>>
>>> Not because pinned files were fragmented, but if the GC victim section has one block is pinned will cause this issue.
>>>
>>> If the section don't unpin the block, it can't be recycled. But there is high chance that the pinned section will be chosen next time under f2fs current victim selection strategy.
>>>
>>> So if we want to avoid unpin files, I think change victim selection to considering pinned blocks can fix this issue.
>>
>> Oh, I get it.
>>
>> How about this?
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 325dab01a29d..3fb52dec5df8 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1730,7 +1730,10 @@ next_alloc:
>> f2fs_down_write(&sbi->gc_lock);
>> stat_inc_gc_call_count(sbi, FOREGROUND);
>> err = f2fs_gc(sbi, &gc_control);
>> - if (err && err != -ENODATA)
>> +
>> + if (err == -EAGAIN)
>> + f2fs_balance_fs(sbi, true);
>> + else if (err && err != -ENODATA)
>> goto out_err;
>> }
> Do you mean to call f2fs_balance_fs() to recycle one section?
> But in this situation, f2fs_balance_fs() will return at enough-free-section check:
> if (has_enough_free_secs(sbi, 0, 0))
> return;
As you said, there are lots of free segments, so I guess it's fine for
latter 2m-aligned allocation, and for the case number of free section is
lower than fggc threshold, we can call f2fs_balance_fs() to reclaim enough
free sections.
Thanks,
>>
>> However, the code won't fix contradictoriness issue, because the root cause
>> is we left fragmented pinned data in filesystem, which should be avoided in
>> GC-reliance LFS filesyetem as much as possible.
>>
>> Thanks,
>>
>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> And this commit changed previous behavior of fallocate():
>>>>>
>>>>> Commit 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN
>>>>> happens")
>>>>>
>>>>> Before this commit, if fallocate() meet this situation, it will trigger
>>>>> FG_GC to recycle pinned space finally.
>>>>>
>>>>> FG_GC is expected to recycle pinned space when there is no more free
>>>>> space. And this is the right time to do it when fallocate() need free
>>>>> space.
>>>>>
>>>>> It is weird when f2fs shows enough spare space but can't fallocate(). So
>>>>> I think it should be fixed.
>>>>>
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>> This issue can be reproduced by filling f2fs space as following layout.
>>>>>>> Every segment has one block is pinned:
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>> | | |p| | | | ... | | seg_n
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>> | | |p| | | | ... | | seg_n+1
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>> ...
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>> | | |p| | | | ... | | seg_n+k
>>>>>>> +-+-+-+-+-+-+-----+-+
>>>>>>>
>>>>>>> And following are steps to reproduce this issue:
>>>>>>> dd if=/dev/zero of=./f2fs_pin.img bs=2M count=1024
>>>>>>> mkfs.f2fs f2fs_pin.img
>>>>>>> mkdir f2fs
>>>>>>> mount f2fs_pin.img ./f2fs
>>>>>>> cd f2fs
>>>>>>> dd if=/dev/zero of=./large_padding bs=1M count=1760
>>>>>>> ./pin_filling.sh
>>>>>>> rm padding*
>>>>>>> sync
>>>>>>> touch fallocate_40m
>>>>>>> f2fs_io pinfile set fallocate_40m
>>>>>>> fallocate -l 41943040 fallocate_40m
>>>>>>>
>>>>>>> fallocate always fail with EAGAIN even there has enough free space.
>>>>>>>
>>>>>>> 'pin_filling.sh' is:
>>>>>>> count=1
>>>>>>> while :
>>>>>>> do
>>>>>>> # filling the seg space
>>>>>>> for i in {1..511}:
>>>>>>> do
>>>>>>> name=padding_$count-$i
>>>>>>> echo write $name
>>>>>>> dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
>>>>>>> if [ $? -ne 0 ]; then
>>>>>>> exit 0
>>>>>>> fi
>>>>>>> done
>>>>>>> sync
>>>>>>>
>>>>>>> # pin one block in a segment
>>>>>>> name=pin_file$count
>>>>>>> dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
>>>>>>> sync
>>>>>>> f2fs_io pinfile set $name
>>>>>>> count=$(($count + 1))
>>>>>>> done
>>>>>>>
>>>>>>> Signed-off-by: Wu Bo <bo.wu@...o.com>
>>>>>>> ---
>>>>>>> fs/f2fs/file.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>> index ca5904129b16..e8a13616543f 100644
>>>>>>> --- a/fs/f2fs/file.c
>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>> @@ -1690,7 +1690,7 @@ static int f2fs_expand_inode_data(struct inode
>>>>>>> *inode, loff_t offset,
>>>>>>> .init_gc_type = FG_GC,
>>>>>>> .should_migrate_blocks = false,
>>>>>>> .err_gc_skipped = true,
>>>>>>> - .nr_free_secs = 0 };
>>>>>>> + .nr_free_secs = 1 };
>>>>>>> pgoff_t pg_start, pg_end;
>>>>>>> loff_t new_size;
>>>>>>> loff_t off_end;
Powered by blists - more mailing lists