| 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: <eedad403-5754-4d5e-965d-19eff02e3d40@gmail.com>
Date: Tue, 28 Nov 2023 20:51:31 +0800
From: Wu Bo <wubo.oduw@...il.com>
To: Chao Yu <chao@...nel.org>, 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 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;
>
> 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