Re: Re: [PATCH 1/5] mm/readahead: Check return value of read_pages

[Raghavendra D Prabhu <raghu.prabhu13@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3246 bytes --]

 
Hi,


* On Sat, Sep 22, 2012 at 08:43:37PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Sat, Sep 22, 2012 at 04:03:10PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> Return value of a_ops->readpage will be propagated to return value of read_pages
>> and __do_page_cache_readahead.
>
>That does not explain the intention and benefit of this patch..

I noticed that force_page_cache_readahead checks return value of 
__do_page_cache_readahead but the actual error if any is never 
propagated.


Also, I made a slight change there:


+	int alloc = 1;
  
  	blk_start_plug(&plug);
  
@@ -127,13 +128,18 @@ static int read_pages(struct address_space *mapping, struct file *filp,
  	for (page_idx = 0; page_idx < nr_pages; page_idx++) {
  		struct page *page = list_to_page(pages);
  		list_del(&page->lru);
-		if (!add_to_page_cache_lru(page, mapping,
+		if (alloc && !add_to_page_cache_lru(page, mapping,
  					page->index, GFP_KERNEL)) {
-			mapping->a_ops->readpage(filp, page);
+			ret = mapping->a_ops->readpage(filp, page);
+			/* 
+			 * If readpage fails, don't proceed with further
+			 * readpage
+			 * */
+			if (ret < 0)
+				alloc = 0;

Before, this the page_cache_release was not happening for the 
rest of the pages.

I will send it in separate patch if this is fine.

>
>Thanks,
>Fengguang
>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  mm/readahead.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index ea8f8fa..461fcc0 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -113,7 +113,7 @@ static int read_pages(struct address_space *mapping, struct file *filp,
>>  {
>>  	struct blk_plug plug;
>>  	unsigned page_idx;
>> -	int ret;
>> +	int ret = 0;
>>
>>  	blk_start_plug(&plug);
>>
>> @@ -129,11 +129,12 @@ static int read_pages(struct address_space *mapping, struct file *filp,
>>  		list_del(&page->lru);
>>  		if (!add_to_page_cache_lru(page, mapping,
>>  					page->index, GFP_KERNEL)) {
>> -			mapping->a_ops->readpage(filp, page);
>> +			ret = mapping->a_ops->readpage(filp, page);
>> +			if (ret < 0)
>> +				break;
>>  		}
>>  		page_cache_release(page);
>>  	}
>> -	ret = 0;
>>
>>  out:
>>  	blk_finish_plug(&plug);
>> @@ -160,6 +161,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  	LIST_HEAD(page_pool);
>>  	int page_idx;
>>  	int ret = 0;
>> +	int ret_read = 0;
>>  	loff_t isize = i_size_read(inode);
>>
>>  	if (isize == 0)
>> @@ -198,10 +200,10 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  	 * will then handle the error.
>>  	 */
>>  	if (ret)
>> -		read_pages(mapping, filp, &page_pool, ret);
>> +		ret_read = read_pages(mapping, filp, &page_pool, ret);
>>  	BUG_ON(!list_empty(&page_pool));
>>  out:
>> -	return ret;
>> +	return (ret_read < 0 ? ret_read : ret);
>>  }
>>
>>  /*
>> --
>> 1.7.12.1
>








Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]