Re: [PATCH RFC 02/13] mm/huge_memory: fix rejection of swap cache folios with a mapping
"Zi Yan" <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.linux.kernel.mm |
|---|---|
| Message-ID | <[email protected]> |
On Fri Aug 7, 2026 at 5:17 PM EDT, Kairui Song via B4 Relay wrote: > From: Kairui Song <[email protected]> > > A folio in the swap cache cannot be split if it has a mapping (shmem). > The split code only checks for this in __folio_freeze_and_split_unmapped, > after the folio ref has been frozen and the NR_SHMEM_THPS/NR_FILE_THPS > counters have been decremented, and returns -EINVAL without unfreezing > the folio or restoring the counters. That error path is fragile: if it > is ever taken, the folio is left frozen and stuck, the counters are > skewed, and the VM_WARN_ON_ONCE_FOLIO would fire for a state that is > actually legitimate. > > Check for this case up front in folio_check_splittable and return > -EINVAL before any state is modified. Under DEBUG_VM, the existing > "Tried to split an unsplittable folio" warning in __folio_split > reports the rejection. Should we return -EBUSY instead? -EINVAL means the caller should not split a swapcache shmem with a mapping and the caller needs to avoid that. The Fixes tag tells me a caller can split a swapcache shmem with a mapping, so with -EINVAL, we will want to add checks at callers to avoid it from happening. > > Fixes: 00527733d0dc ("mm/huge_memory: add two new (not yet used) functions for folio_split()") > Fixes: 714b056c8321 ("mm/huge_memory: convert VM_BUG* to VM_WARN* in __folio_split") > Signed-off-by: Kairui Song <[email protected]> > --- > mm/huge_memory.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index ced400f72d43..2fa72158e063 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -3878,6 +3878,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order, > int folio_check_splittable(struct folio *folio, unsigned int new_order, > enum split_type split_type) > { > + bool is_anon = folio_test_anon(folio); > + bool is_swapcache = folio_test_swapcache(folio); > + > VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio); > /* > * Folios that just got truncated cannot get split. Signal to the > @@ -3886,11 +3889,11 @@ int folio_check_splittable(struct folio *folio, unsigned int new_order, > * TODO: this will also currently refuse folios without a mapping in the > * swapcache (shmem or to-be-anon folios). > */ > - if (!folio->mapping && !folio_test_anon(folio)) > + if (!folio->mapping && !is_anon) > return -EBUSY; > > /* order-1 is not supported for anonymous THP. */ > - if (folio_test_anon(folio) && new_order == 1) > + if (is_anon && new_order == 1) > return -EINVAL; > > /* > @@ -3901,7 +3904,7 @@ int folio_check_splittable(struct folio *folio, unsigned int new_order, > * swapcache folio split. Only uniform split to order-0 can be used > * here. > */ > - if ((split_type == SPLIT_TYPE_NON_UNIFORM || new_order) && folio_test_swapcache(folio)) { > + if ((split_type == SPLIT_TYPE_NON_UNIFORM || new_order) && is_swapcache) { > return -EINVAL; > } > > @@ -3911,6 +3914,15 @@ int folio_check_splittable(struct folio *folio, unsigned int new_order, > if (folio_test_writeback(folio)) > return -EBUSY; > > + /* > + * A non-anon swapcache folio that still has a mapping should only > + * be a shmem folio under IO, there is little benefit in splitting > + * them hence not supported. Reject it here up front: the split > + * routine cannot back out cleanly once the folio ref is frozen. > + */ > + if (!is_anon && is_swapcache && folio->mapping) > + return -EINVAL; > + > return 0; > } > > @@ -3983,14 +3995,8 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n > } > } > > - if (folio_test_swapcache(folio)) { > - if (mapping) { > - VM_WARN_ON_ONCE_FOLIO(mapping, folio); > - return -EINVAL; > - } > - > + if (folio_test_swapcache(folio)) > ci = swap_cluster_get_and_lock(folio); > - } > > /* lock lru list/PageCompound, ref frozen by page_ref_freeze */ > if (do_lru) -- Best Regards, Yan, Zi