Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <faa90acc-c03e-2913-c19a-bd50fd93d197@gmail.com>
Date:   Fri, 17 Nov 2023 07:34:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block
 situation
Content-Language: en-US
To:     Chao Yu <chao@kernel.org>, Wu Bo <bo.wu@vivo.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
References: <20231030094024.263707-1-bo.wu@vivo.com>
 <c181256e-9f6e-d43e-4d02-a7d8d5286d56@kernel.org>
 <670ce4a6-f00c-dbe9-86e2-366311221cf3@gmail.com>
 <a69b7544-2312-486c-d655-8b86e370c55e@kernel.org>
From:   Wu Bo <wubo.oduw@gmail.com>
In-Reply-To: <a69b7544-2312-486c-d655-8b86e370c55e@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

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