>
>> 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?
>
>> 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.
>> 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
>
>>
>> Signed-off-by: Huang Shijie <b32955@xxxxxxxxxxxxx>
>> ---
>> Â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@xxxxxxxxxx Â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@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx 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