| Message ID | 20250214175709.76029-2-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, swap: remove swap slot cache | expand |
On 02/15/25 at 01:57am, Kairui Song wrote: > From: Kairui Song <kasong@tencent.com> > > Swap allocator will do swap cache reclaim to recycle HAS_CACHE slots for > allocation. It initiates the reclaim from the offset to be reclaimed and > looks up the corresponding folio. The lookup process is lockless, so it's > possible the folio will be removed from the swap cache and given > a different swap entry before the reclaim locks the folio. If > it happens, the reclaim will end up reclaiming an irrelevant folio, and > return wrong return value. > > This shouldn't cause any problem with correctness or stability, but > it is indeed confusing and unexpected, and will increase fragmentation, > decrease performance. > > Fix this by checking whether the folio is still pointing to the offset > the allocator want to reclaim before reclaiming it. > > Signed-off-by: Kairui Song <kasong@tencent.com> > --- > mm/swapfile.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 34baefb000b5..c77ffee4af86 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -210,6 +210,7 @@ static int __try_to_reclaim_swap(struct swap_info_struct *si, > int ret, nr_pages; > bool need_reclaim; > > +again: > folio = filemap_get_folio(address_space, swap_cache_index(entry)); > if (IS_ERR(folio)) > return 0; > @@ -227,8 +228,16 @@ static int __try_to_reclaim_swap(struct swap_info_struct *si, > if (!folio_trylock(folio)) > goto out; > > - /* offset could point to the middle of a large folio */ > + /* > + * Offset could point to the middle of a large folio, or folio > + * may no longer point to the expected offset before it's locked. > + */ > entry = folio->swap; > + if (offset < swp_offset(entry) || offset >= swp_offset(entry) + nr_pages) { > + folio_unlock(folio); > + folio_put(folio); > + goto again; > + } > offset = swp_offset(entry); LGTM, Reviewed-by: Baoquan He <bhe@redhat.com> While reading the code in __try_to_reclaim_swap(), I am always worried that entry indexed by offset could be accessed by other users so tht it doesn't only has cache, because we released the ci->lock and don't hold any lock during period. It could be me who think too much.
diff --git a/mm/swapfile.c b/mm/swapfile.c index 34baefb000b5..c77ffee4af86 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -210,6 +210,7 @@ static int __try_to_reclaim_swap(struct swap_info_struct *si, int ret, nr_pages; bool need_reclaim; +again: folio = filemap_get_folio(address_space, swap_cache_index(entry)); if (IS_ERR(folio)) return 0; @@ -227,8 +228,16 @@ static int __try_to_reclaim_swap(struct swap_info_struct *si, if (!folio_trylock(folio)) goto out; - /* offset could point to the middle of a large folio */ + /* + * Offset could point to the middle of a large folio, or folio + * may no longer point to the expected offset before it's locked. + */ entry = folio->swap; + if (offset < swp_offset(entry) || offset >= swp_offset(entry) + nr_pages) { + folio_unlock(folio); + folio_put(folio); + goto again; + } offset = swp_offset(entry); need_reclaim = ((flags & TTRS_ANYWAY) ||