| 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
| ||
|
Message-ID: <5d8726fc-e912-6954-3820-862eecff07b0@kernel.org>
Date: Tue, 28 Nov 2023 14:22:37 +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/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;
}
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