Re: [f2fs-dev] [PATCH 1/1] f2fs: fix fallocate failed under pinned block situation

[Chao Yu <chao@kernel.org>]

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@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;


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel