Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block situation
From: Chao Yu
Date: Sat Dec 09 2023 - 05:06:02 EST
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@xxxxxxxx>
---
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;