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

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

[Huang Shijie]

The read_swap_cache_async() does not check the return value
of swap_readpage().

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.

The patch fixs the problem.

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

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>

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

[Hugh Dickins]

On Fri, 14 Jan 2011, Huang Shijie wrote:
> The read_swap_cache_async() does not check the return value
> of swap_readpage().
> 

Thanks, we may want to fix that.


> 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().


> 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).

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?


> 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.

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>

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

[Huang Shijie]

>
>> 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>