Messages in this thread Patch in this message | | | Date | Tue, 28 Nov 2023 20:51:31 +0800 | Subject | Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block situation | From | Wu Bo <> |
| |
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@vivo.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;
| |