Re: [PATCH] swap : check the return value of swap_readpage()

[Huang Shijie <shijie8@gmail.com>]

>
>> 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@freescale.com>
>> ---
>>  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>