From: Kairui Song <kasong@tencent.com>
The filemap_get_incore_folio (previously find_get_incore_page) helper
was introduced by commit 61ef18655704 ("mm: factor find_get_incore_page
out of mincore_page") to be used by later commit f5df8635c5a3 ("mm: use
find_get_incore_page in memcontrol"), so memory cgroup charge move code
can be simplified.
But commit 6b611388b626 ("memcg-v1: remove charge move code") removed
that user completely, it's only used by mincore now.
So this commit basically reverts commit 61ef18655704 ("mm: factor
find_get_incore_page out of mincore_page"). Move it back to mincore side
to simplify the code.
Signed-off-by: Kairui Song <kasong@tencent.com>
---
mm/mincore.c | 29 +++++++++++++++++++++++++++--
mm/swap.h | 10 ----------
mm/swap_state.c | 38 --------------------------------------
3 files changed, 27 insertions(+), 50 deletions(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 10dabefc3acc..f0d3c9419e58 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -64,8 +64,33 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t index)
* any other file mapping (ie. marked !present and faulted in with
* tmpfs's .fault). So swapped out tmpfs mappings are tested here.
*/
- folio = filemap_get_incore_folio(mapping, index);
- if (!IS_ERR(folio)) {
+ if (IS_ENABLED(CONFIG_SWAP) && shmem_mapping(mapping)) {
+ folio = filemap_get_entry(mapping, index);
+ /*
+ * shmem/tmpfs may return swap: account for swapcache
+ * page too.
+ */
+ if (xa_is_value(folio)) {
+ struct swap_info_struct *si;
+ swp_entry_t swp = radix_to_swp_entry(folio);
+ /* There might be swapin error entries in shmem mapping. */
+ if (non_swap_entry(swp))
+ return 0;
+ /* Prevent swap device to being swapoff under us */
+ si = get_swap_device(swp);
+ if (si) {
+ folio = filemap_get_folio(swap_address_space(swp),
+ swap_cache_index(swp));
+ put_swap_device(si);
+ } else {
+ return 0;
+ }
+ }
+ } else {
+ folio = filemap_get_folio(mapping, index);
+ }
+
+ if (folio) {
present = folio_test_uptodate(folio);
folio_put(folio);
}
diff --git a/mm/swap.h b/mm/swap.h
index 911ad5ff0f89..1ae44d4193b1 100644
--- a/mm/swap.h
+++ b/mm/swap.h
@@ -64,9 +64,6 @@ void clear_shadow_from_swap_cache(int type, unsigned long begin,
void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry, int nr);
struct folio *swap_cache_get_folio(swp_entry_t entry,
struct vm_area_struct *vma, unsigned long addr);
-struct folio *filemap_get_incore_folio(struct address_space *mapping,
- pgoff_t index);
-
struct folio *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
struct vm_area_struct *vma, unsigned long addr,
struct swap_iocb **plug);
@@ -178,13 +175,6 @@ static inline struct folio *swap_cache_get_folio(swp_entry_t entry,
return NULL;
}
-static inline
-struct folio *filemap_get_incore_folio(struct address_space *mapping,
- pgoff_t index)
-{
- return filemap_get_folio(mapping, index);
-}
-
static inline void *get_shadow_from_swap_cache(swp_entry_t entry)
{
return NULL;
diff --git a/mm/swap_state.c b/mm/swap_state.c
index c354435a0923..99513b74b5d8 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -323,44 +323,6 @@ struct folio *swap_cache_get_folio(swp_entry_t entry,
return folio;
}
-/**
- * filemap_get_incore_folio - Find and get a folio from the page or swap caches.
- * @mapping: The address_space to search.
- * @index: The page cache index.
- *
- * This differs from filemap_get_folio() in that it will also look for the
- * folio in the swap cache.
- *
- * Return: The found folio or %NULL.
- */
-struct folio *filemap_get_incore_folio(struct address_space *mapping,
- pgoff_t index)
-{
- swp_entry_t swp;
- struct swap_info_struct *si;
- struct folio *folio = filemap_get_entry(mapping, index);
-
- if (!folio)
- return ERR_PTR(-ENOENT);
- if (!xa_is_value(folio))
- return folio;
- if (!shmem_mapping(mapping))
- return ERR_PTR(-ENOENT);
-
- swp = radix_to_swp_entry(folio);
- /* There might be swapin error entries in shmem mapping. */
- if (non_swap_entry(swp))
- return ERR_PTR(-ENOENT);
- /* Prevent swapoff from happening to us */
- si = get_swap_device(swp);
- if (!si)
- return ERR_PTR(-ENOENT);
- index = swap_cache_index(swp);
- folio = filemap_get_folio(swap_address_space(swp), index);
- put_swap_device(si);
- return folio;
-}
-
struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
struct mempolicy *mpol, pgoff_t ilx, bool *new_page_allocated,
bool skip_if_exists)
--
2.50.1On Thu, Aug 7, 2025 at 8:27 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> The filemap_get_incore_folio (previously find_get_incore_page) helper
> was introduced by commit 61ef18655704 ("mm: factor find_get_incore_page
> out of mincore_page") to be used by later commit f5df8635c5a3 ("mm: use
> find_get_incore_page in memcontrol"), so memory cgroup charge move code
> can be simplified.
>
> But commit 6b611388b626 ("memcg-v1: remove charge move code") removed
> that user completely, it's only used by mincore now.
>
> So this commit basically reverts commit 61ef18655704 ("mm: factor
> find_get_incore_page out of mincore_page"). Move it back to mincore side
> to simplify the code.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
Seems reasonable to me for the most part - just a couple of questions below.
> ---
> mm/mincore.c | 29 +++++++++++++++++++++++++++--
> mm/swap.h | 10 ----------
> mm/swap_state.c | 38 --------------------------------------
> 3 files changed, 27 insertions(+), 50 deletions(-)
>
> diff --git a/mm/mincore.c b/mm/mincore.c
> index 10dabefc3acc..f0d3c9419e58 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -64,8 +64,33 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t index)
> * any other file mapping (ie. marked !present and faulted in with
> * tmpfs's .fault). So swapped out tmpfs mappings are tested here.
> */
> - folio = filemap_get_incore_folio(mapping, index);
> - if (!IS_ERR(folio)) {
> + if (IS_ENABLED(CONFIG_SWAP) && shmem_mapping(mapping)) {
Do we need CONFIG_SWAP check here? I suppose if !CONFIG_SWAP we'll
never end up with an ordinary swap entry stored here right?
Saves a couple of cycles, I suppose. No strong opinions.
> + folio = filemap_get_entry(mapping, index);
> + /*
> + * shmem/tmpfs may return swap: account for swapcache
> + * page too.
> + */
> + if (xa_is_value(folio)) {
> + struct swap_info_struct *si;
> + swp_entry_t swp = radix_to_swp_entry(folio);
> + /* There might be swapin error entries in shmem mapping. */
> + if (non_swap_entry(swp))
> + return 0;
> + /* Prevent swap device to being swapoff under us */
> + si = get_swap_device(swp);
> + if (si) {
> + folio = filemap_get_folio(swap_address_space(swp),
> + swap_cache_index(swp));
> + put_swap_device(si);
> + } else {
> + return 0;
> + }
> + }
> + } else {
> + folio = filemap_get_folio(mapping, index);
> + }
> +
> + if (folio) {
Should this check be "if (!IS_ERR(folio))"? Seems like that's how we
inspect the output of filemap_get_folio() in other locations (for e.g,
in filemap_fault()).
> present = folio_test_uptodate(folio);
> folio_put(folio);
> }
On Fri, Aug 8, 2025 at 2:07 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Thu, Aug 7, 2025 at 8:27 AM Kairui Song <ryncsn@gmail.com> wrote:
> >
> > From: Kairui Song <kasong@tencent.com>
> >
> > The filemap_get_incore_folio (previously find_get_incore_page) helper
> > was introduced by commit 61ef18655704 ("mm: factor find_get_incore_page
> > out of mincore_page") to be used by later commit f5df8635c5a3 ("mm: use
> > find_get_incore_page in memcontrol"), so memory cgroup charge move code
> > can be simplified.
> >
> > But commit 6b611388b626 ("memcg-v1: remove charge move code") removed
> > that user completely, it's only used by mincore now.
> >
> > So this commit basically reverts commit 61ef18655704 ("mm: factor
> > find_get_incore_page out of mincore_page"). Move it back to mincore side
> > to simplify the code.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
>
> Seems reasonable to me for the most part - just a couple of questions below.
>
> > ---
> > mm/mincore.c | 29 +++++++++++++++++++++++++++--
> > mm/swap.h | 10 ----------
> > mm/swap_state.c | 38 --------------------------------------
> > 3 files changed, 27 insertions(+), 50 deletions(-)
> >
> > diff --git a/mm/mincore.c b/mm/mincore.c
> > index 10dabefc3acc..f0d3c9419e58 100644
> > --- a/mm/mincore.c
> > +++ b/mm/mincore.c
> > @@ -64,8 +64,33 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t index)
> > * any other file mapping (ie. marked !present and faulted in with
> > * tmpfs's .fault). So swapped out tmpfs mappings are tested here.
> > */
> > - folio = filemap_get_incore_folio(mapping, index);
> > - if (!IS_ERR(folio)) {
> > + if (IS_ENABLED(CONFIG_SWAP) && shmem_mapping(mapping)) {
>
> Do we need CONFIG_SWAP check here? I suppose if !CONFIG_SWAP we'll
> never end up with an ordinary swap entry stored here right?
Yes, and in the next patch I'd like to introduce a WARN_ON if we see
swap entries with !CONFIG_SWAP. That means the memory is corrupted.
>
> Saves a couple of cycles, I suppose. No strong opinions.
Before 61ef18655704 it used a `#ifdef CONFIG_SWAP`, I used
IS_ENABLED(CONFIG_SWAP) here, same thing, the compiler will optimize
out the unused branch. Just with fewer lines of code and I personally
think this looks prettier.
>
> > + folio = filemap_get_entry(mapping, index);
> > + /*
> > + * shmem/tmpfs may return swap: account for swapcache
> > + * page too.
> > + */
> > + if (xa_is_value(folio)) {
> > + struct swap_info_struct *si;
> > + swp_entry_t swp = radix_to_swp_entry(folio);
> > + /* There might be swapin error entries in shmem mapping. */
> > + if (non_swap_entry(swp))
> > + return 0;
> > + /* Prevent swap device to being swapoff under us */
> > + si = get_swap_device(swp);
> > + if (si) {
> > + folio = filemap_get_folio(swap_address_space(swp),
> > + swap_cache_index(swp));
> > + put_swap_device(si);
> > + } else {
> > + return 0;
> > + }
> > + }
> > + } else {
> > + folio = filemap_get_folio(mapping, index);
> > + }
> > +
> > + if (folio) {
>
> Should this check be "if (!IS_ERR(folio))"? Seems like that's how we
> inspect the output of filemap_get_folio() in other locations (for e.g,
> in filemap_fault()).
Yeah you are right, actuall should be IS_ERR_OR_NULL here as it uses
both filemap_get_entry and filemap_get_folio.
I wanted to change to always use filemap_get_entry in the next patch
for better performance, but somehow forgot it...
Will fix it.
>
> > present = folio_test_uptodate(folio);
> > folio_put(folio);
> > }
>
© 2016 - 2025 Red Hat, Inc.