| Message ID | 20240205031415.557879-1-bo.wu@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" | expand |
On 2024/2/5 11:14, Wu Bo wrote: > This reverts commit 2e42b7f817acd6e8d78226445eb6fe44fe79c12a. > > If the GC victim section has a pinned block when fallocate() trigger > FG_GC, the section is not able to be recycled. And this will return > -EAGAIN cause fallocate() failed, even though there are much spare space > as user see. As the GC policy prone to chose the same victim, > fallocate() may not successed at a long period. > > This scenario has been found during Android OTA. > > Link: https://lore.kernel.org/linux-f2fs-devel/20231030094024.263707-1-bo.wu@vivo.com/t/#u > > CC: stable@vger.kernel.org > 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 b58ab1157b7e..19915faccee9 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1725,7 +1725,7 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset, > 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 && err != -ENODATA && err != -EAGAIN) > goto out_err; > } How about calling f2fs_balance_fs() to double check and make sure there is enough free space for following allocation. if (has_not_enough_free_secs(sbi, 0, GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { f2fs_down_write(&sbi->gc_lock); stat_inc_gc_call_count(sbi, FOREGROUND); err = f2fs_gc(sbi, &gc_control); if (err == -EAGAIN) f2fs_balance_fs(sbi, true); if (err && err != -ENODATA) goto out_err; } Thanks, >
On 2024/2/5 11:54, Chao Yu wrote: > How about calling f2fs_balance_fs() to double check and make sure > there is > enough free space for following allocation. > > if (has_not_enough_free_secs(sbi, 0, > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { > f2fs_down_write(&sbi->gc_lock); > stat_inc_gc_call_count(sbi, FOREGROUND); > err = f2fs_gc(sbi, &gc_control); > if (err == -EAGAIN) > f2fs_balance_fs(sbi, true); > if (err && err != -ENODATA) > goto out_err; > } > > Thanks, f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. I'm afraid this is a bit redundant. >
On 2024/2/8 16:11, Wu Bo wrote: > On 2024/2/5 11:54, Chao Yu wrote: >> How about calling f2fs_balance_fs() to double check and make sure there is >> enough free space for following allocation. >> >> if (has_not_enough_free_secs(sbi, 0, >> GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { >> f2fs_down_write(&sbi->gc_lock); >> stat_inc_gc_call_count(sbi, FOREGROUND); >> err = f2fs_gc(sbi, &gc_control); >> if (err == -EAGAIN) >> f2fs_balance_fs(sbi, true); >> if (err && err != -ENODATA) >> goto out_err; >> } >> >> Thanks, > > f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. > > I'm afraid this is a bit redundant. Okay. I guess maybe Jaegeuk has concern which is the reason to commit 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN happens"). Thanks, > >>
On Tue, Feb 20, 2024 at 02:50:11PM +0800, Chao Yu wrote: > On 2024/2/8 16:11, Wu Bo wrote: > > On 2024/2/5 11:54, Chao Yu wrote: > > > How about calling f2fs_balance_fs() to double check and make sure there is > > > enough free space for following allocation. > > > > > > if (has_not_enough_free_secs(sbi, 0, > > > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { > > > f2fs_down_write(&sbi->gc_lock); > > > stat_inc_gc_call_count(sbi, FOREGROUND); > > > err = f2fs_gc(sbi, &gc_control); > > > if (err == -EAGAIN) > > > f2fs_balance_fs(sbi, true); > > > if (err && err != -ENODATA) > > > goto out_err; > > > } > > > > > > Thanks, > > > > f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. > > > > I'm afraid this is a bit redundant. > > Okay. > > I guess maybe Jaegeuk has concern which is the reason to commit > 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN happens"). Hi Jaegeuk, We occasionally receive user complaints about OTA failures caused by this issue. Please consider merging this patch. Thanks > > Thanks, > > > > > > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2024/9/6 16:31, Wu Bo wrote: > On Tue, Feb 20, 2024 at 02:50:11PM +0800, Chao Yu wrote: >> On 2024/2/8 16:11, Wu Bo wrote: >>> On 2024/2/5 11:54, Chao Yu wrote: >>>> How about calling f2fs_balance_fs() to double check and make sure there is >>>> enough free space for following allocation. >>>> >>>> if (has_not_enough_free_secs(sbi, 0, >>>> GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { >>>> f2fs_down_write(&sbi->gc_lock); >>>> stat_inc_gc_call_count(sbi, FOREGROUND); >>>> err = f2fs_gc(sbi, &gc_control); >>>> if (err == -EAGAIN) >>>> f2fs_balance_fs(sbi, true); >>>> if (err && err != -ENODATA) >>>> goto out_err; >>>> } >>>> >>>> Thanks, >>> >>> f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. >>> >>> I'm afraid this is a bit redundant. >> >> Okay. >> >> I guess maybe Jaegeuk has concern which is the reason to commit >> 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN happens"). > > Hi Jaegeuk, > > We occasionally receive user complaints about OTA failures caused by this issue. > Please consider merging this patch. I'm fine w/ this patch, but one another quick fix will be triggering background GC via f2fs ioctl after fallocate() failure, once has_not_enough_free_secs(, ovp_segs) returns false, fallocate() will succeed. Reviewed-by: Chao Yu <chao@kernel.org> Thanks, > > Thanks > >> >> Thanks, >> >>> >>>> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 09/06, Chao Yu wrote: > On 2024/9/6 16:31, Wu Bo wrote: > > On Tue, Feb 20, 2024 at 02:50:11PM +0800, Chao Yu wrote: > > > On 2024/2/8 16:11, Wu Bo wrote: > > > > On 2024/2/5 11:54, Chao Yu wrote: > > > > > How about calling f2fs_balance_fs() to double check and make sure there is > > > > > enough free space for following allocation. > > > > > > > > > > if (has_not_enough_free_secs(sbi, 0, > > > > > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { > > > > > f2fs_down_write(&sbi->gc_lock); > > > > > stat_inc_gc_call_count(sbi, FOREGROUND); > > > > > err = f2fs_gc(sbi, &gc_control); > > > > > if (err == -EAGAIN) > > > > > f2fs_balance_fs(sbi, true); > > > > > if (err && err != -ENODATA) > > > > > goto out_err; > > > > > } > > > > > > > > > > Thanks, > > > > > > > > f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. > > > > > > > > I'm afraid this is a bit redundant. > > > > > > Okay. > > > > > > I guess maybe Jaegeuk has concern which is the reason to commit > > > 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN happens"). > > > > Hi Jaegeuk, > > > > We occasionally receive user complaints about OTA failures caused by this issue. > > Please consider merging this patch. What about adding a retry logic here, as it's literally EAGAIN? > > I'm fine w/ this patch, but one another quick fix will be triggering > background GC via f2fs ioctl after fallocate() failure, once > has_not_enough_free_secs(, ovp_segs) returns false, fallocate() will > succeed. > > Reviewed-by: Chao Yu <chao@kernel.org> > > Thanks, > > > > > Thanks > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On Fri, Sep 06, 2024 at 10:52:47PM +0000, Jaegeuk Kim via Linux-f2fs-devel wrote: > On 09/06, Chao Yu wrote: > > On 2024/9/6 16:31, Wu Bo wrote: > > > On Tue, Feb 20, 2024 at 02:50:11PM +0800, Chao Yu wrote: > > > > On 2024/2/8 16:11, Wu Bo wrote: > > > > > On 2024/2/5 11:54, Chao Yu wrote: > > > > > > How about calling f2fs_balance_fs() to double check and make sure there is > > > > > > enough free space for following allocation. > > > > > > > > > > > > if (has_not_enough_free_secs(sbi, 0, > > > > > > GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) { > > > > > > f2fs_down_write(&sbi->gc_lock); > > > > > > stat_inc_gc_call_count(sbi, FOREGROUND); > > > > > > err = f2fs_gc(sbi, &gc_control); > > > > > > if (err == -EAGAIN) > > > > > > f2fs_balance_fs(sbi, true); > > > > > > if (err && err != -ENODATA) > > > > > > goto out_err; > > > > > > } > > > > > > > > > > > > Thanks, > > > > > > > > > > f2fs_balance_fs() here will not change procedure branch and may just trigger another GC. > > > > > > > > > > I'm afraid this is a bit redundant. > > > > > > > > Okay. > > > > > > > > I guess maybe Jaegeuk has concern which is the reason to commit > > > > 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN happens"). > > > > > > Hi Jaegeuk, > > > > > > We occasionally receive user complaints about OTA failures caused by this issue. > > > Please consider merging this patch. > > What about adding a retry logic here, as it's literally EAGAIN? In this scenario, the remaining reclaimable sections has a block been pinned. As a result, the user sees that there is enough free space, but fallocate still fails. This happens because the GC triggered by fallocate cannot recycle the section that has been pinned. No matter how many times it’s attempted, it will continue to fail. I included steps to reproduce this scenario in my previous proposal patch: https://lore.kernel.org/linux-f2fs-devel/20231030094024.263707-1-bo.wu@vivo.com/t/#u However, this issue can't be reproduced on latest kernel. Seems this commit prevents the creation of non-segment size pinned files and also avoids this scenario. 3fdd89b452c2 (f2fs: prevent writing without fallocate() for pinned files) > > > > > I'm fine w/ this patch, but one another quick fix will be triggering > > background GC via f2fs ioctl after fallocate() failure, once > > has_not_enough_free_secs(, ovp_segs) returns false, fallocate() will > > succeed. > > > > > Reviewed-by: Chao Yu <chao@kernel.org> > > > > Thanks, > > > > > > > > Thanks > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Linux-f2fs-devel mailing list > > > > Linux-f2fs-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index b58ab1157b7e..19915faccee9 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -1725,7 +1725,7 @@ static int f2fs_expand_inode_data(struct inode *inode, loff_t offset, 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 && err != -ENODATA && err != -EAGAIN) goto out_err; }
This reverts commit 2e42b7f817acd6e8d78226445eb6fe44fe79c12a. If the GC victim section has a pinned block when fallocate() trigger FG_GC, the section is not able to be recycled. And this will return -EAGAIN cause fallocate() failed, even though there are much spare space as user see. As the GC policy prone to chose the same victim, fallocate() may not successed at a long period. This scenario has been found during Android OTA. Link: https://lore.kernel.org/linux-f2fs-devel/20231030094024.263707-1-bo.wu@vivo.com/t/#u CC: stable@vger.kernel.org Signed-off-by: Wu Bo <bo.wu@vivo.com> --- fs/f2fs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)