* [PATCH] erofs: fix missing folio_unlock causing lock imbalance @ 2026-03-31 2:33 Zhan Xusheng 2026-03-31 2:50 ` Gao Xiang 0 siblings, 1 reply; 9+ messages in thread From: Zhan Xusheng @ 2026-03-31 2:33 UTC (permalink / raw) To: Gao Xiang; +Cc: open list:EROFS FILE SYSTEM, linux-kernel, Zhan Xusheng folio_trylock() in erofs_try_to_free_all_cached_folios() may successfully acquire the folio lock, but the subsequent check for erofs_folio_is_managed() can skip unlocking when the folio is not managed by EROFS. This leads to a lock imbalance and leaves the folio permanently locked, which may cause reclaim stalls or interfere with memory management. Fix this by ensuring folio_unlock() is called before continuing. Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> --- fs/erofs/zdata.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index fe8121df9ef2..9d7ff22f1622 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -605,8 +605,10 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, if (!folio_trylock(folio)) return -EBUSY; - if (!erofs_folio_is_managed(sbi, folio)) + if (!erofs_folio_is_managed(sbi, folio)) { + folio_unlock(folio); continue; + } pcl->compressed_bvecs[i].page = NULL; folio_detach_private(folio); folio_unlock(folio); -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] erofs: fix missing folio_unlock causing lock imbalance 2026-03-31 2:33 [PATCH] erofs: fix missing folio_unlock causing lock imbalance Zhan Xusheng @ 2026-03-31 2:50 ` Gao Xiang 2026-03-31 2:58 ` Gao Xiang 0 siblings, 1 reply; 9+ messages in thread From: Gao Xiang @ 2026-03-31 2:50 UTC (permalink / raw) To: Zhan Xusheng, Gao Xiang Cc: open list:EROFS FILE SYSTEM, linux-kernel, Zhan Xusheng Hi Zhan, On 2026/3/31 10:33, Zhan Xusheng wrote: > folio_trylock() in erofs_try_to_free_all_cached_folios() may > successfully acquire the folio lock, but the subsequent check > for erofs_folio_is_managed() can skip unlocking when the folio > is not managed by EROFS. Do you find some real timing? I don't think it can really happen, because: > > This leads to a lock imbalance and leaves the folio permanently > locked, which may cause reclaim stalls or interfere with memory > management. > > Fix this by ensuring folio_unlock() is called before continuing. If a folio links to a pcluster, folio->private will be non-NULL, and pcl->compressed_bvecs[i] points to that folios. And z_erofs_cache_release_folio() will be called with folio lock, and pcl->compressed_bvecs[i] will be set NULL here. So I don't think erofs_try_to_free_all_cached_folios() can find !erofs_folio_is_managed(sbi, folio) in the real world. > > Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> > --- > fs/erofs/zdata.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c > index fe8121df9ef2..9d7ff22f1622 100644 > --- a/fs/erofs/zdata.c > +++ b/fs/erofs/zdata.c > @@ -605,8 +605,10 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, > if (!folio_trylock(folio)) > return -EBUSY; > > - if (!erofs_folio_is_managed(sbi, folio)) > + if (!erofs_folio_is_managed(sbi, folio)) { > + folio_unlock(folio); > continue; > + } > pcl->compressed_bvecs[i].page = NULL; > folio_detach_private(folio); But I admit that we should rewrite in function as: if (!erofs_folio_is_managed(sbi, folio)) { DBG_BUGON(1); } else { pcl->compressed_bvecs[i].page = NULL; folio_detach_private(folio); } folio_unlock(folio); Thanks, Gao Xiang > folio_unlock(folio); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] erofs: fix missing folio_unlock causing lock imbalance 2026-03-31 2:50 ` Gao Xiang @ 2026-03-31 2:58 ` Gao Xiang 2026-03-31 3:32 ` Zhan Xusheng 0 siblings, 1 reply; 9+ messages in thread From: Gao Xiang @ 2026-03-31 2:58 UTC (permalink / raw) To: Zhan Xusheng, Gao Xiang Cc: open list:EROFS FILE SYSTEM, linux-kernel, Zhan Xusheng On 2026/3/31 10:50, Gao Xiang wrote: > Hi Zhan, > > On 2026/3/31 10:33, Zhan Xusheng wrote: >> folio_trylock() in erofs_try_to_free_all_cached_folios() may >> successfully acquire the folio lock, but the subsequent check >> for erofs_folio_is_managed() can skip unlocking when the folio >> is not managed by EROFS. > > Do you find some real timing? > > I don't think it can really happen, because: > >> >> This leads to a lock imbalance and leaves the folio permanently >> locked, which may cause reclaim stalls or interfere with memory >> management. >> >> Fix this by ensuring folio_unlock() is called before continuing. > > If a folio links to a pcluster, folio->private will be non-NULL, > and pcl->compressed_bvecs[i] points to that folios. > > And z_erofs_cache_release_folio() will be called with folio lock, > and pcl->compressed_bvecs[i] will be set NULL here. > > So I don't think erofs_try_to_free_all_cached_folios() can find > !erofs_folio_is_managed(sbi, folio) in the real world. > >> >> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> >> --- >> fs/erofs/zdata.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c >> index fe8121df9ef2..9d7ff22f1622 100644 >> --- a/fs/erofs/zdata.c >> +++ b/fs/erofs/zdata.c >> @@ -605,8 +605,10 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, >> if (!folio_trylock(folio)) >> return -EBUSY; >> - if (!erofs_folio_is_managed(sbi, folio)) >> + if (!erofs_folio_is_managed(sbi, folio)) { >> + folio_unlock(folio); >> continue; >> + } >> pcl->compressed_bvecs[i].page = NULL; >> folio_detach_private(folio); > > But I admit that we should rewrite in function as: > > if (!erofs_folio_is_managed(sbi, folio)) { > DBG_BUGON(1); > } else { > pcl->compressed_bvecs[i].page = NULL; > folio_detach_private(folio); > } Or maybe just: DBG_BUGON(!erofs_folio_is_managed(sbi, folio)); pcl->compressed_bvecs[i].page = NULL; folio_detach_private(folio); folio_unlock(folio); Since if a pcluster goes here (!pcl->lockref.count), `pcl->compressed_bvecs[i]` should leave all valid cached folios (Or some should be recycled by .release_folio instead.) Unless there is the other bug somewhere, but in any case, I don't think your phenomenon is related to EROFS. Thanks, Gao Xiang > folio_unlock(folio); > > Thanks, > Gao Xiang > > >> folio_unlock(folio); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] erofs: fix missing folio_unlock causing lock imbalance 2026-03-31 2:58 ` Gao Xiang @ 2026-03-31 3:32 ` Zhan Xusheng 2026-03-31 3:46 ` [PATCH] [PATCH v2] " Zhan Xusheng 0 siblings, 1 reply; 9+ messages in thread From: Zhan Xusheng @ 2026-03-31 3:32 UTC (permalink / raw) To: Gao Xiang; +Cc: Zhan Xusheng, linux-erofs Hi Gao, Thanks for the detailed explanation. I see your point that folios attached to a pcluster should always be managed by EROFS, and the !erofs_folio_is_managed() case should not happen in practice. Instead of handling it silently, it makes more sense to treat this as an invariant and catch unexpected cases explicitly. I'll update the patch accordingly by adding DBG_BUGON() and send a v2. Thanks, Zhan Xusheng ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] [PATCH v2] erofs: fix missing folio_unlock causing lock imbalance 2026-03-31 3:32 ` Zhan Xusheng @ 2026-03-31 3:46 ` Zhan Xusheng 2026-03-31 3:55 ` Gao Xiang 0 siblings, 1 reply; 9+ messages in thread From: Zhan Xusheng @ 2026-03-31 3:46 UTC (permalink / raw) To: Gao Xiang; +Cc: Gao Xiang, linux-erofs, linux-kernel, Zhan Xusheng folio_trylock() in erofs_try_to_free_all_cached_folios() may successfully acquire the folio lock, but the subsequent check for erofs_folio_is_managed() can skip unlocking when the folio is not managed by EROFS. As Gao Xiang pointed out, this condition should not happen in practice because compressed_bvecs[] only holds valid cached folios at this point — any non-managed folio would have already been detached by z_erofs_cache_release_folio() under folio lock. Fix this by adding DBG_BUGON() to catch unexpected folios and ensure folio_unlock() is always called. Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> --- fs/erofs/zdata.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index fe8121df9ef2..b566996a0d1a 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -605,8 +605,7 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, if (!folio_trylock(folio)) return -EBUSY; - if (!erofs_folio_is_managed(sbi, folio)) - continue; + DBG_BUGON(!erofs_folio_is_managed(sbi, folio)); pcl->compressed_bvecs[i].page = NULL; folio_detach_private(folio); folio_unlock(folio); -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] [PATCH v2] erofs: fix missing folio_unlock causing lock imbalance 2026-03-31 3:46 ` [PATCH] [PATCH v2] " Zhan Xusheng @ 2026-03-31 3:55 ` Gao Xiang 2026-03-31 5:02 ` [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() Zhan Xusheng 0 siblings, 1 reply; 9+ messages in thread From: Gao Xiang @ 2026-03-31 3:55 UTC (permalink / raw) To: Zhan Xusheng, Gao Xiang; +Cc: linux-erofs, linux-kernel, Zhan Xusheng On 2026/3/31 11:46, Zhan Xusheng wrote: > folio_trylock() in erofs_try_to_free_all_cached_folios() may > successfully acquire the folio lock, but the subsequent check > for erofs_folio_is_managed() can skip unlocking when the folio > is not managed by EROFS. > > As Gao Xiang pointed out, this condition should not happen in > practice because compressed_bvecs[] only holds valid cached folios > at this point — any non-managed folio would have already been > detached by z_erofs_cache_release_folio() under folio lock. > > Fix this by adding DBG_BUGON() to catch unexpected folios > and ensure folio_unlock() is always called. > > Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com> > Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> emmm, the patch subject needs to be changed too, like: [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() Thanks, Gao Xiang > --- > fs/erofs/zdata.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c > index fe8121df9ef2..b566996a0d1a 100644 > --- a/fs/erofs/zdata.c > +++ b/fs/erofs/zdata.c > @@ -605,8 +605,7 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, > if (!folio_trylock(folio)) > return -EBUSY; > > - if (!erofs_folio_is_managed(sbi, folio)) > - continue; > + DBG_BUGON(!erofs_folio_is_managed(sbi, folio)); > pcl->compressed_bvecs[i].page = NULL; > folio_detach_private(folio); > folio_unlock(folio); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() 2026-03-31 3:55 ` Gao Xiang @ 2026-03-31 5:02 ` Zhan Xusheng 2026-03-31 5:28 ` Gao Xiang 2026-04-02 6:46 ` Chunhai Guo 0 siblings, 2 replies; 9+ messages in thread From: Zhan Xusheng @ 2026-03-31 5:02 UTC (permalink / raw) To: Gao Xiang; +Cc: Gao Xiang, linux-erofs, linux-kernel, Zhan Xusheng folio_trylock() in erofs_try_to_free_all_cached_folios() may successfully acquire the folio lock, but the subsequent check for erofs_folio_is_managed() can skip unlocking when the folio is not managed by EROFS. As Gao Xiang pointed out, this condition should not happen in practice because compressed_bvecs[] only holds valid cached folios at this point — any non-managed folio would have already been detached by z_erofs_cache_release_folio() under folio lock. Fix this by adding DBG_BUGON() to catch unexpected folios and ensure folio_unlock() is always called. Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> --- fs/erofs/zdata.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c index fe8121df9ef2..b566996a0d1a 100644 --- a/fs/erofs/zdata.c +++ b/fs/erofs/zdata.c @@ -605,8 +605,7 @@ static int erofs_try_to_free_all_cached_folios(struct erofs_sb_info *sbi, if (!folio_trylock(folio)) return -EBUSY; - if (!erofs_folio_is_managed(sbi, folio)) - continue; + DBG_BUGON(!erofs_folio_is_managed(sbi, folio)); pcl->compressed_bvecs[i].page = NULL; folio_detach_private(folio); folio_unlock(folio); -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() 2026-03-31 5:02 ` [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() Zhan Xusheng @ 2026-03-31 5:28 ` Gao Xiang 2026-04-02 6:46 ` Chunhai Guo 1 sibling, 0 replies; 9+ messages in thread From: Gao Xiang @ 2026-03-31 5:28 UTC (permalink / raw) To: Zhan Xusheng, Gao Xiang; +Cc: linux-erofs, linux-kernel, Zhan Xusheng On 2026/3/31 13:02, Zhan Xusheng wrote: > folio_trylock() in erofs_try_to_free_all_cached_folios() may > successfully acquire the folio lock, but the subsequent check > for erofs_folio_is_managed() can skip unlocking when the folio > is not managed by EROFS. > > As Gao Xiang pointed out, this condition should not happen in > practice because compressed_bvecs[] only holds valid cached folios > at this point — any non-managed folio would have already been > detached by z_erofs_cache_release_folio() under folio lock. > > Fix this by adding DBG_BUGON() to catch unexpected folios > and ensure folio_unlock() is always called. > > Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com> > Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() 2026-03-31 5:02 ` [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() Zhan Xusheng 2026-03-31 5:28 ` Gao Xiang @ 2026-04-02 6:46 ` Chunhai Guo 1 sibling, 0 replies; 9+ messages in thread From: Chunhai Guo @ 2026-04-02 6:46 UTC (permalink / raw) To: Zhan Xusheng, Gao Xiang Cc: Gao Xiang, linux-erofs@lists.ozlabs.org, linux-kernel@vger.kernel.org, Zhan Xusheng On 3/31/2026 1:02 PM, Zhan Xusheng wrote: > folio_trylock() in erofs_try_to_free_all_cached_folios() may > successfully acquire the folio lock, but the subsequent check > for erofs_folio_is_managed() can skip unlocking when the folio > is not managed by EROFS. > > As Gao Xiang pointed out, this condition should not happen in > practice because compressed_bvecs[] only holds valid cached folios > at this point — any non-managed folio would have already been > detached by z_erofs_cache_release_folio() under folio lock. > > Fix this by adding DBG_BUGON() to catch unexpected folios > and ensure folio_unlock() is always called. > > Suggested-by: Gao Xiang <hsiangkao@linux.alibaba.com> > Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com> Reviewed-by: Chunhai Guo <guochunhai@vivo.com> Thanks, ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-04-02 6:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-31 2:33 [PATCH] erofs: fix missing folio_unlock causing lock imbalance Zhan Xusheng 2026-03-31 2:50 ` Gao Xiang 2026-03-31 2:58 ` Gao Xiang 2026-03-31 3:32 ` Zhan Xusheng 2026-03-31 3:46 ` [PATCH] [PATCH v2] " Zhan Xusheng 2026-03-31 3:55 ` Gao Xiang 2026-03-31 5:02 ` [PATCH] [PATCH v3] erofs: ensure all folios are managed in erofs_try_to_free_all_cached_folios() Zhan Xusheng 2026-03-31 5:28 ` Gao Xiang 2026-04-02 6:46 ` Chunhai Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox