quoted

If swap_readpage() returns -ENOMEM, the read_swap_cache_async()
still returns the `new_page` which has nothing. The caller will
do some wrong operations on the `new_page` such as copy.

But what's really wrong is not to be checking PageUptodate.
Looks like swapoff's try_to_unuse() never checked it (I'm largely
guilty of that), and my ksm_does_need_to_copy() would blindly copy
(from a !PageUptodate to a PageUptodate), averting the PageUptodate
check which comes later in do_swap_page().

yes.

 I noticed that your  ksm_does_need_to_copy()  does not check the
PageUptodate(), and
copy the data. Is it good you do so?

quoted

The patch fixs the problem.

It may fix a part of the problem, but - forgive me for saying! -
your patch is not so beautiful that I want to push it as is.

I'm more worried by the cases when the read gets an error and fails:
we ought to be looking at what filemap.c does in it !PageUptodate case,
and following a similar strategy (perhaps we shall want to distinguish
the ENOMEM case, perhaps not: depends on the implementation).

IMHO, it's better to  distinguish the ENOMEN case as soon as possible.
Following the similar strategy in filemap.c only makes the situation more
complicated. Maybe I am not catching your meaning.

Is this ENOMEM case something you noticed by looking at the source,
or something that has hit you in practice?  If the latter, then it's
more urgent to fix it: but I'd be wondering how it comes about that
bio's mempools have let you down, and even their GFP_KERNEL allocation
is failing?

Do not worry :)   I just noticed it by reading the code.

quoted

Also remove the unlock_ page() in swap_readpage() in the wrong case
, since __delete_from_swap_cache() needs a locked page.

That change is only required because we're not checking PageUptodate
properly everywhere.

Thanks  a lot

Huang Shijie

Hugh

quoted

Signed-off-by: Huang Shijie <redacted>
---
 mm/page_io.c    |    1 -
 mm/swap_state.c |   12 +++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index 2dee975..5c759f2 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c

@@ -124,7 +124,6 @@ int swap_readpage(struct page *page)

      VM_BUG_ON(PageUptodate(page));
      bio = get_swap_bio(GFP_KERNEL, page, end_swap_bio_read);
      if (bio == NULL) {
-             unlock_page(page);
              ret = -ENOMEM;
              goto out;
      }

diff --git a/mm/swap_state.c b/mm/swap_state.c
index 5c8cfab..3bd7238 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c

@@ -331,16 +331,18 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,

              __set_page_locked(new_page);
              SetPageSwapBacked(new_page);
              err = __add_to_swap_cache(new_page, entry);
+             radix_tree_preload_end();
              if (likely(!err)) {
-                     radix_tree_preload_end();
                      /*
                       * Initiate read into locked page and return.
                       */
-                     lru_cache_add_anon(new_page);
-                     swap_readpage(new_page);
-                     return new_page;
+                     err = swap_readpage(new_page);
+                     if (likely(!err)) {
+                             lru_cache_add_anon(new_page);
+                             return new_page;
+                     }
+                     __delete_from_swap_cache(new_page);
              }
-             radix_tree_preload_end();
              ClearPageSwapBacked(new_page);
              __clear_page_locked(new_page);
              /*
--
1.7.3.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>