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.
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 <[email protected]>
---
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;
--
2.35.3
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 <[email protected]>
>>>> ---
>>>> 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;
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 <[email protected]>
>>>>> ---
>>>>> 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;
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 <[email protected]>
>>>>>> ---
>>>>>> 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;
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 <[email protected]>
>>>>>>> ---
>>>>>>> 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;
On 2023/12/9 17:46, Chao Yu wrote:
> 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,
Yes, this make sense. I didn't see allocation will continue after
f2fs_balance_fs() 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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;