On 2020/3/3 16:47, 王矛 wrote:
> 
> Hi Chao,
> 
> Can u kindly help to check Sahitya's comments?
> 
> Thanks,
> Mao
> 
> 
> At 2020-02-17 13:28:29, "Sahitya Tummala" <stumm...@codeaurora.org> wrote:
>>Hi Chao,
>>
>>On Mon, Feb 17, 2020 at 11:50:56AM +0800, Chao Yu wrote:
>>> Hi 王矛,
>>> 
>>> On 2020/2/16 19:39, 王矛 wrote:
>>> > *So the problem is:*
>>> > 
>>> > 1. in new_curseg(), if the segno allocated is invalid(no free segment, 
>>> > max segno
>>> > is returned).
>>> > 
>>> > F2fs should do something to indicate this exception.
>>> > 
>>> > 2. otherwise, we may hit the f2fs panic(se invalid).
>>> > 
>>> > Maybe we should do sanity check in update_sit_entry() to see if segno is 
>>> > really
>>> > out of range and caused this panic.
>>> 
>>> I'm afraid it's too late to handle such error in update_sit_entry(), since 
>>> we
>>> expect all procedures in do_write_page() will be successful, it's a little 
>>> hard
>>> to handle such error in that context.
>>> 
>>> So the problem here is why we can not find any free segments w/ LFS 
>>> allocation,
>>> because in case of lack of free segments (check via 
>>> has_not_enough_free_secs()),
>>> f2fs will force to trigger f2fs_gc() to recycle free sections.
>>> 
>>
>>I think this issue can happen when the __write_data_page is invoked in
>>non-reclaim path (wbc->for_reclaim is false) and there aren't enough free_secs
>>available. In this case, we won't at all check for has_not_enough_free_secs() 
>>and
>>don't do f2fs_balance_fs().
>>
>>I think __write_data_page() can be enhanced to fix this case as below.
>>Can you please check and provide your feedback?
>>
>>diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>index 66b02d3..71e1221 100644
>>--- a/fs/f2fs/data.c
>>+++ b/fs/f2fs/data.c
>>@@ -2176,10 +2176,18 @@ static int __write_data_page(struct page *page, bool 
>>*submitted,
>>
>>        if (!wbc->for_reclaim)
>>                need_balance_fs = true;
>>-       else if (has_not_enough_free_secs(sbi, 0, 0))
>>-               goto redirty_out;
>>-       else
>>-               set_inode_flag(inode, FI_HOT_DATA);
>>+
>>+       if (has_not_enough_free_secs(sbi, 0, 0)) {
>>+               if (!wbc->for_reclaim && !free_sections(sbi))

It's still too late if there is no free sections, as even we trigger 
f2fs_balance_fs
to run foreground GC, we may need a dirty segment to write data w/ SSR mode, 
however
all dirty segment could be node type...and prefree segment couldn't be reused 
as we
didn't finish calling last checkpoint during FGGC.

And with this way, we may cover the root cause of this issue, I guess we'd 
better
check why we have no free sections here first.

Thanks,

>>+                       goto redirty_out;
>>+               if (wbc->for_reclaim) {
>>+                       if (!free_sections(sbi))
>>+                               need_balance_fs = true;
>>+                       goto redirty_out;
>>+               }
>>+       }
>>+
>>+       set_inode_flag(inode, FI_HOT_DATA);
>>
>>        err = -EAGAIN;
>>        if (f2fs_has_inline_data(inode)) {
>>@@ -2252,6 +2260,9 @@ static int __write_data_page(struct page *page, bool 
>>*submitted,
>>        if (!err || wbc->for_reclaim)
>>                return AOP_WRITEPAGE_ACTIVATE;
>>        unlock_page(page);
>>+       if (!err && !S_ISDIR(inode->i_mode) && !IS_NOQUOTA(inode) &&
>>+                                       !F2FS_I(inode)->cp_task)
>>+               f2fs_balance_fs(sbi, need_balance_fs);
>>        return err;
>> }
>>
>>Thanks,
>>
>>> I doubt there may be some corner case we haven't considered, result all free
>>> segments (including reserved free segments) was exhausted by data/node 
>>> writes
>>> when last checkpoint is triggered during umount.
>>> 
>>> If this issue can be reproduced (during umount, free_segments() <
>>> reserved_segments()), we can add some logs to see why f2fs_balance_fs() 
>>> fail to
>>> recycle enough free segments previously.
>>> 
>>> Thanks,
>>> 
>>> > 
>>> > 
>>> > Thanks,
>>> > Mao
>>> > 
>>> > 
>>> > 
>>> >  
>>> > 
>>> 
>>> 
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
>>-- 
>>--
>>Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> 
> 
> 
>  
> 


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

Reply via email to