* [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" @ 2024-02-05 3:14 Wu Bo 2024-02-05 3:54 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Wu Bo @ 2024-02-05 3:14 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Wu Bo, Wu Bo, stable 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; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-02-05 3:14 [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" Wu Bo @ 2024-02-05 3:54 ` Chao Yu 2024-02-08 8:11 ` Wu Bo 0 siblings, 1 reply; 8+ messages in thread From: Chao Yu @ 2024-02-05 3:54 UTC (permalink / raw) To: Wu Bo, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, Wu Bo, stable 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, > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-02-05 3:54 ` Chao Yu @ 2024-02-08 8:11 ` Wu Bo 2024-02-20 6:50 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Wu Bo @ 2024-02-08 8:11 UTC (permalink / raw) To: Chao Yu, Wu Bo, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, stable 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. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-02-08 8:11 ` Wu Bo @ 2024-02-20 6:50 ` Chao Yu 2024-09-06 8:31 ` [f2fs-dev] " Wu Bo 0 siblings, 1 reply; 8+ messages in thread From: Chao Yu @ 2024-02-20 6:50 UTC (permalink / raw) To: Wu Bo, Wu Bo, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, stable 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, > >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-02-20 6:50 ` Chao Yu @ 2024-09-06 8:31 ` Wu Bo 2024-09-06 10:07 ` Chao Yu 0 siblings, 1 reply; 8+ messages in thread From: Wu Bo @ 2024-09-06 8:31 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Wu Bo, Wu Bo, Chao Yu, linux-kernel, stable, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-09-06 8:31 ` [f2fs-dev] " Wu Bo @ 2024-09-06 10:07 ` Chao Yu 2024-09-06 22:52 ` Jaegeuk Kim 0 siblings, 1 reply; 8+ messages in thread From: Chao Yu @ 2024-09-06 10:07 UTC (permalink / raw) To: Wu Bo, Jaegeuk Kim; +Cc: chao, Wu Bo, linux-kernel, stable, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-09-06 10:07 ` Chao Yu @ 2024-09-06 22:52 ` Jaegeuk Kim 2024-09-10 8:33 ` Wu Bo 0 siblings, 1 reply; 8+ messages in thread From: Jaegeuk Kim @ 2024-09-06 22:52 UTC (permalink / raw) To: Chao Yu; +Cc: Wu Bo, Wu Bo, linux-kernel, stable, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [f2fs-dev] [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" 2024-09-06 22:52 ` Jaegeuk Kim @ 2024-09-10 8:33 ` Wu Bo 0 siblings, 0 replies; 8+ messages in thread From: Wu Bo @ 2024-09-10 8:33 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Wu Bo, Chao Yu, Wu Bo, stable, linux-kernel, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-09-10 8:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-05 3:14 [PATCH] Revert "f2fs: stop allocating pinned sections if EAGAIN happens" Wu Bo 2024-02-05 3:54 ` Chao Yu 2024-02-08 8:11 ` Wu Bo 2024-02-20 6:50 ` Chao Yu 2024-09-06 8:31 ` [f2fs-dev] " Wu Bo 2024-09-06 10:07 ` Chao Yu 2024-09-06 22:52 ` Jaegeuk Kim 2024-09-10 8:33 ` Wu Bo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox