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

[Wu Bo <wubo.oduw@gmail.com>]

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;


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