[PATCH 0/5] Readahead fixes / improvements

[PATCH 0/5] Readahead fixes / improvements

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

Following are some of the fixes / improvements to the page cache readahead:
first four are minor fixes along the readahead path, the last one is an
improvement to use find_get_pages in __do_page_cache_readahead.

Raghavendra D Prabhu (5):
  mm/readahead: Check return value of read_pages
  mm/readahead: Change the condition for SetPageReadahead
  Remove file_ra_state from arguments of count_history_pages.
  Move the check for ra_pages after VM_SequentialReadHint()
  mm/readahead: Use find_get_pages instead of radix_tree_lookup.

 mm/filemap.c   |  5 +++--
 mm/readahead.c | 50 ++++++++++++++++++++++++++++++++++----------------
 2 files changed, 37 insertions(+), 18 deletions(-)

-- 
1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[raghu.prabhu13]

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.

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

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

If page lookup from radix_tree_lookup is successful and its index page_idx ==
nr_to_read - lookahead_size, then SetPageReadahead never gets called, so this
fixes that.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 mm/readahead.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 461fcc0..fec726c 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -189,8 +189,10 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
 			break;
 		page->index = page_offset;
 		list_add(&page->lru, &page_pool);
-		if (page_idx == nr_to_read - lookahead_size)
+		if (page_idx >= nr_to_read - lookahead_size) {
 			SetPageReadahead(page);
+			lookahead_size = 0;
+		}
 		ret++;
 	}
 
-- 
1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/5] Remove file_ra_state from arguments of count_history_pages.

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

count_history_pages doesn't require readahead state to calculate the offset from history.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 mm/readahead.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index fec726c..3977455 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -349,7 +349,6 @@ static unsigned long get_next_ra_size(struct file_ra_state *ra,
  * 	- thrashing threshold in memory tight systems
  */
 static pgoff_t count_history_pages(struct address_space *mapping,
-				   struct file_ra_state *ra,
 				   pgoff_t offset, unsigned long max)
 {
 	pgoff_t head;
@@ -372,7 +371,7 @@ static int try_context_readahead(struct address_space *mapping,
 {
 	pgoff_t size;
 
-	size = count_history_pages(mapping, ra, offset, max);
+	size = count_history_pages(mapping, offset, max);
 
 	/*
 	 * no history pages:
-- 
1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/5] Move the check for ra_pages after VM_SequentialReadHint()

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

page_cache_sync_readahead checks for ra->ra_pages again, so moving the check
after VM_SequentialReadHint.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 mm/filemap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 3843445..606a648 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1523,8 +1523,6 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
 	/* If we don't want any read-ahead, don't bother */
 	if (VM_RandomReadHint(vma))
 		return;
-	if (!ra->ra_pages)
-		return;
 
 	if (VM_SequentialReadHint(vma)) {
 		page_cache_sync_readahead(mapping, ra, file, offset,
@@ -1532,6 +1530,9 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
 		return;
 	}
 
+	if (!ra->ra_pages)
+		return;
+
 	/* Avoid banging the cache line if not needed */
 	if (ra->mmap_miss < MMAP_LOTSAMISS * 10)
 		ra->mmap_miss++;
-- 
1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

Instead of running radix_tree_lookup in a loop and lock/unlocking in the
process, find_get_pages is called once, which returns a page_list, some of which
are not NULL and are in core.

Also, since find_get_pages returns number of pages, if all pages are already
cached, it can return early.

This will be mostly helpful when a higher proportion of nr_to_read pages are
already in the cache, which will mean less locking for page cache hits.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 mm/readahead.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/mm/readahead.c b/mm/readahead.c
index 3977455..3a1798d 100644
--- a/mm/readahead.c
+++ b/mm/readahead.c
@@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
 {
 	struct inode *inode = mapping->host;
 	struct page *page;
+	struct page **page_list = NULL;
 	unsigned long end_index;	/* The last page we want to read */
 	LIST_HEAD(page_pool);
 	int page_idx;
 	int ret = 0;
 	int ret_read = 0;
+	unsigned long num;
+	pgoff_t page_offset;
 	loff_t isize = i_size_read(inode);
 
 	if (isize == 0)
 		goto out;
 
+	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
+	if (!page_list)
+		goto out;
+
 	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
+	num = find_get_pages(mapping, offset, nr_to_read, page_list);
 
 	/*
 	 * Preallocate as many pages as we will need.
 	 */
 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
-		pgoff_t page_offset = offset + page_idx;
+		if (page_list[page_idx]) {
+			page_cache_release(page_list[page_idx]);
+			continue;
+		}
+
+		page_offset = offset + page_idx;
 
 		if (page_offset > end_index)
 			break;
 
-		rcu_read_lock();
-		page = radix_tree_lookup(&mapping->page_tree, page_offset);
-		rcu_read_unlock();
-		if (page)
-			continue;
-
 		page = page_cache_alloc_readahead(mapping);
-		if (!page)
+		if (unlikely(!page))
 			break;
 		page->index = page_offset;
 		list_add(&page->lru, &page_pool);
@@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
 			lookahead_size = 0;
 		}
 		ret++;
+
+		/*
+		 * Since num pages are already returned, bail out after
+		 * nr_to_read - num pages are allocated and added.
+		 */
+		if (ret == nr_to_read - num)
+			break;
 	}
 
 	/*
@@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
 		ret_read = read_pages(mapping, filp, &page_pool, ret);
 	BUG_ON(!list_empty(&page_pool));
 out:
+	kfree(page_list);
 	return (ret_read < 0 ? ret_read : ret);
 }
 
-- 
1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/5] Remove file_ra_state from arguments of count_history_pages.

[Fengguang Wu]

On Sat, Sep 22, 2012 at 04:03:12PM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> count_history_pages doesn't require readahead state to calculate the offset from history.
> 
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>

Acked-by: Fengguang Wu <fengguang.wu@intel.com>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/5] Move the check for ra_pages after VM_SequentialReadHint()

[Fengguang Wu]

On Sat, Sep 22, 2012 at 04:03:13PM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> page_cache_sync_readahead checks for ra->ra_pages again, so moving the check
> after VM_SequentialReadHint.

Well it depends on what case you are optimizing for. I suspect there
are much more tmpfs users than VM_SequentialReadHint users. So this
change is actually not desirable wrt the more widely used cases.

Thanks,
Fengguang

> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  mm/filemap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 3843445..606a648 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1523,8 +1523,6 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>  	/* If we don't want any read-ahead, don't bother */
>  	if (VM_RandomReadHint(vma))
>  		return;
> -	if (!ra->ra_pages)
> -		return;
>  
>  	if (VM_SequentialReadHint(vma)) {
>  		page_cache_sync_readahead(mapping, ra, file, offset,
> @@ -1532,6 +1530,9 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>  		return;
>  	}
>  
> +	if (!ra->ra_pages)
> +		return;
> +
>  	/* Avoid banging the cache line if not needed */
>  	if (ra->mmap_miss < MMAP_LOTSAMISS * 10)
>  		ra->mmap_miss++;
> -- 
> 1.7.12.1
> 
> --
> 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/ .
> 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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Fengguang Wu]

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

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

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Fengguang Wu]

On Sat, Sep 22, 2012 at 04:03:11PM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> If page lookup from radix_tree_lookup is successful and its index page_idx ==
> nr_to_read - lookahead_size, then SetPageReadahead never gets called, so this
> fixes that.

NAK. Sorry. It's actually an intentional behavior, so that for the
common cases of many cached files that are accessed frequently, no
PG_readahead will be set at all to pointlessly trap into the readahead
routines once and again.

Perhaps we need a patch for commenting that case. :)

Thanks,
Fengguang

> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  mm/readahead.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 461fcc0..fec726c 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -189,8 +189,10 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>  			break;
>  		page->index = page_offset;
>  		list_add(&page->lru, &page_pool);
> -		if (page_idx == nr_to_read - lookahead_size)
> +		if (page_idx >= nr_to_read - lookahead_size) {
>  			SetPageReadahead(page);
> +			lookahead_size = 0;
> +		}
>  		ret++;
>  	}
>  
> -- 
> 1.7.12.1

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Fengguang Wu]

On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> Instead of running radix_tree_lookup in a loop and lock/unlocking in the
> process, find_get_pages is called once, which returns a page_list, some of which
> are not NULL and are in core.
> 
> Also, since find_get_pages returns number of pages, if all pages are already
> cached, it can return early.
> 
> This will be mostly helpful when a higher proportion of nr_to_read pages are
> already in the cache, which will mean less locking for page cache hits.

Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
the benefit of this patch is questionable. Will need real performance
numbers to support it.

> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  mm/readahead.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 3977455..3a1798d 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>  {
>  	struct inode *inode = mapping->host;
>  	struct page *page;
> +	struct page **page_list = NULL;
>  	unsigned long end_index;	/* The last page we want to read */
>  	LIST_HEAD(page_pool);
>  	int page_idx;
>  	int ret = 0;
>  	int ret_read = 0;
> +	unsigned long num;
> +	pgoff_t page_offset;
>  	loff_t isize = i_size_read(inode);
>  
>  	if (isize == 0)
>  		goto out;
>  
> +	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
> +	if (!page_list)
> +		goto out;

That cost one more memory allocation and added code to maintain the
page list. The original code also don't have the cost of grabbing the
page count, which eliminate the trouble of page release.

>  	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
> +	num = find_get_pages(mapping, offset, nr_to_read, page_list);

Assume we want to readahead pages for indexes [0, 100] and the cached
pages are in [1000, 1100]. find_get_pages() will return the latter.
Which is probably not the your expected results.

>  	/*
>  	 * Preallocate as many pages as we will need.
>  	 */
>  	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
> -		pgoff_t page_offset = offset + page_idx;
> +		if (page_list[page_idx]) {
> +			page_cache_release(page_list[page_idx]);
> +			continue;
> +		}
> +
> +		page_offset = offset + page_idx;
>  
>  		if (page_offset > end_index)
>  			break;
>  
> -		rcu_read_lock();
> -		page = radix_tree_lookup(&mapping->page_tree, page_offset);
> -		rcu_read_unlock();
> -		if (page)
> -			continue;
> -
>  		page = page_cache_alloc_readahead(mapping);
> -		if (!page)
> +		if (unlikely(!page))
>  			break;

That break will leave the remaining pages' page_count lifted and lead
to memory leak.

>  		page->index = page_offset;
>  		list_add(&page->lru, &page_pool);
> @@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>  			lookahead_size = 0;
>  		}
>  		ret++;
> +
> +		/*
> +		 * Since num pages are already returned, bail out after
> +		 * nr_to_read - num pages are allocated and added.
> +		 */
> +		if (ret == nr_to_read - num)
> +			break;

Confused. That break seems unnecessary?

Thanks,
Fengguang

>  	}
>  
>  	/*
> @@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>  		ret_read = read_pages(mapping, filp, &page_pool, ret);
>  	BUG_ON(!list_empty(&page_pool));
>  out:
> +	kfree(page_list);
>  	return (ret_read < 0 ? ret_read : ret);
>  }
>  
> -- 
> 1.7.12.1
> 
> --
> 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/ .
> 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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Raghavendra D Prabhu]

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

Re: Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Raghavendra D Prabhu]

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

Hi,


* On Sat, Sep 22, 2012 at 08:49:20PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Sat, Sep 22, 2012 at 04:03:11PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> If page lookup from radix_tree_lookup is successful and its index page_idx ==
>> nr_to_read - lookahead_size, then SetPageReadahead never gets called, so this
>> fixes that.
>
>NAK. Sorry. It's actually an intentional behavior, so that for the
>common cases of many cached files that are accessed frequently, no
>PG_readahead will be set at all to pointlessly trap into the readahead
>routines once and again.

ACK, thanks for explaining that. However, regarding this, I would 
like to know if the implications of the patch 
51daa88ebd8e0d437289f589af29d4b39379ea76 will still apply if 
PG_readahead is not set.

>
>Perhaps we need a patch for commenting that case. :)
>
>Thanks,
>Fengguang
>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  mm/readahead.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index 461fcc0..fec726c 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -189,8 +189,10 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  			break;
>>  		page->index = page_offset;
>>  		list_add(&page->lru, &page_pool);
>> -		if (page_idx == nr_to_read - lookahead_size)
>> +		if (page_idx >= nr_to_read - lookahead_size) {
>>  			SetPageReadahead(page);
>> +			lookahead_size = 0;
>> +		}
>>  		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 --]

Re: Re: [PATCH 4/5] Move the check for ra_pages after VM_SequentialReadHint()

[Raghavendra D Prabhu]

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

Hi,


* On Sat, Sep 22, 2012 at 08:42:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Sat, Sep 22, 2012 at 04:03:13PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> page_cache_sync_readahead checks for ra->ra_pages again, so moving the check
>> after VM_SequentialReadHint.
>
>Well it depends on what case you are optimizing for. I suspect there
>are much more tmpfs users than VM_SequentialReadHint users. So this
>change is actually not desirable wrt the more widely used cases.

Yes, that is true. However, it was meant to eliminate duplicate 
checking of the same condition in two places. But you are right, 
it may impose overhead for majority of the cases (assuming 
POSIX_FADVISE is used less than tmpfs).

>
>Thanks,
>Fengguang
>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  mm/filemap.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 3843445..606a648 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -1523,8 +1523,6 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>>  	/* If we don't want any read-ahead, don't bother */
>>  	if (VM_RandomReadHint(vma))
>>  		return;
>> -	if (!ra->ra_pages)
>> -		return;
>>
>>  	if (VM_SequentialReadHint(vma)) {
>>  		page_cache_sync_readahead(mapping, ra, file, offset,
>> @@ -1532,6 +1530,9 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>>  		return;
>>  	}
>>
>> +	if (!ra->ra_pages)
>> +		return;
>> +
>>  	/* Avoid banging the cache line if not needed */
>>  	if (ra->mmap_miss < MMAP_LOTSAMISS * 10)
>>  		ra->mmap_miss++;
>> --
>> 1.7.12.1
>>
>> --
>> 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/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>




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

Re: Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Raghavendra D Prabhu]

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

Hi,


* On Sat, Sep 22, 2012 at 09:15:07PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> Instead of running radix_tree_lookup in a loop and lock/unlocking in the
>> process, find_get_pages is called once, which returns a page_list, some of which
>> are not NULL and are in core.
>>
>> Also, since find_get_pages returns number of pages, if all pages are already
>> cached, it can return early.
>>
>> This will be mostly helpful when a higher proportion of nr_to_read pages are
>> already in the cache, which will mean less locking for page cache hits.
>
>Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
>the benefit of this patch is questionable. Will need real performance
>numbers to support it.

Aside from the rcu lock/unlock, isn't it better to not make 
separate calls to radix_tree_lookup and merge them into one call? 
Similar approach is used with pagevec_lookup which is usually 
used when one needs to deal with a set of pages.

>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  mm/readahead.c | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index 3977455..3a1798d 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  {
>>  	struct inode *inode = mapping->host;
>>  	struct page *page;
>> +	struct page **page_list = NULL;
>>  	unsigned long end_index;	/* The last page we want to read */
>>  	LIST_HEAD(page_pool);
>>  	int page_idx;
>>  	int ret = 0;
>>  	int ret_read = 0;
>> +	unsigned long num;
>> +	pgoff_t page_offset;
>>  	loff_t isize = i_size_read(inode);
>>
>>  	if (isize == 0)
>>  		goto out;
>>
>> +	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
>> +	if (!page_list)
>> +		goto out;
>
>That cost one more memory allocation and added code to maintain the
>page list. The original code also don't have the cost of grabbing the
>page count, which eliminate the trouble of page release.
>
>>  	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
>> +	num = find_get_pages(mapping, offset, nr_to_read, page_list);
>
>Assume we want to readahead pages for indexes [0, 100] and the cached
>pages are in [1000, 1100]. find_get_pages() will return the latter.
>Which is probably not the your expected results.

I thought if I ask for pages in the range [0,100] it will 
return a sparse array [0,100] but with holes (NULL) for pages not in 
cache and references to pages in cache. Isn't that the expected 
behavior?

>
>>  	/*
>>  	 * Preallocate as many pages as we will need.
>>  	 */
>>  	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
>> -		pgoff_t page_offset = offset + page_idx;
>> +		if (page_list[page_idx]) {
>> +			page_cache_release(page_list[page_idx]);
>> +			continue;
>> +		}
>> +
>> +		page_offset = offset + page_idx;
>>
>>  		if (page_offset > end_index)
>>  			break;
>>
>> -		rcu_read_lock();
>> -		page = radix_tree_lookup(&mapping->page_tree, page_offset);
>> -		rcu_read_unlock();
>> -		if (page)
>> -			continue;
>> -
>>  		page = page_cache_alloc_readahead(mapping);
>> -		if (!page)
>> +		if (unlikely(!page))
>>  			break;
>
>That break will leave the remaining pages' page_count lifted and lead
>to memory leak.

Thanks. Yes, I realized that now.
>
>>  		page->index = page_offset;
>>  		list_add(&page->lru, &page_pool);
>> @@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  			lookahead_size = 0;
>>  		}
>>  		ret++;
>> +
>> +		/*
>> +		 * Since num pages are already returned, bail out after
>> +		 * nr_to_read - num pages are allocated and added.
>> +		 */
>> +		if (ret == nr_to_read - num)
>> +			break;
>
>Confused. That break seems unnecessary?

I fixed that:


  -               pgoff_t page_offset = offset + page_idx;
  -
  -               if (page_offset > end_index)
  -                       break;
  -
  -               rcu_read_lock();
  -               page = radix_tree_lookup(&mapping->page_tree, page_offset);
  -               rcu_read_unlock();
  -               if (page)
  +               if (page_list[page_idx]) {
  +                       page_cache_release(page_list[page_idx]);
  +                       num--;
                          continue;
  +               }
  +
  +               page_offset = offset + page_idx;
  +
  +               /*
  +                * Break only if all the previous
  +                * references have been released
  +                */
  +               if (page_offset > end_index) {
  +                       if (!num)
  +                               break;
  +                       else
  +                               continue;
  +               }

                  page = page_cache_alloc_readahead(mapping);
  -               if (!page)
  -                       break;
  +               if (unlikely(!page))
  +                       continue;

>
>Thanks,
>Fengguang
>
>>  	}
>>
>>  	/*
>> @@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>>  		ret_read = read_pages(mapping, filp, &page_pool, ret);
>>  	BUG_ON(!list_empty(&page_pool));
>>  out:
>> +	kfree(page_list);
>>  	return (ret_read < 0 ? ret_read : ret);
>>  }
>>
>> --
>> 1.7.12.1
>>
>> --
>> 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/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>




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

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

[Fengguang Wu]

On Wed, Sep 26, 2012 at 06:55:03AM +0530, Raghavendra D Prabhu wrote:
> 
> 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.

force_page_cache_readahead()'s return value, in turn, is never used by
its callers.. Nor does the other __do_page_cache_readahead() callers
care about the error state. So until we find an actual user of the
error code, I'd recommend to avoid changing the current code.

Thanks,
Fengguang

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Fengguang Wu]

On Wed, Sep 26, 2012 at 06:59:00AM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Sat, Sep 22, 2012 at 08:49:20PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Sat, Sep 22, 2012 at 04:03:11PM +0530, raghu.prabhu13@gmail.com wrote:
> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>
> >>If page lookup from radix_tree_lookup is successful and its index page_idx ==
> >>nr_to_read - lookahead_size, then SetPageReadahead never gets called, so this
> >>fixes that.
> >
> >NAK. Sorry. It's actually an intentional behavior, so that for the
> >common cases of many cached files that are accessed frequently, no
> >PG_readahead will be set at all to pointlessly trap into the readahead
> >routines once and again.
> 
> ACK, thanks for explaining that. However, regarding this, I would
> like to know if the implications of the patch
> 51daa88ebd8e0d437289f589af29d4b39379ea76 will still apply if
> PG_readahead is not set.

Would you elaborate the implication and the possible problematic case?

Thanks,
Fengguang

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Fengguang Wu]

On Wed, Sep 26, 2012 at 08:28:20AM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Sat, Sep 22, 2012 at 09:15:07PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>
> >>Instead of running radix_tree_lookup in a loop and lock/unlocking in the
> >>process, find_get_pages is called once, which returns a page_list, some of which
> >>are not NULL and are in core.
> >>
> >>Also, since find_get_pages returns number of pages, if all pages are already
> >>cached, it can return early.
> >>
> >>This will be mostly helpful when a higher proportion of nr_to_read pages are
> >>already in the cache, which will mean less locking for page cache hits.
> >
> >Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
> >the benefit of this patch is questionable. Will need real performance
> >numbers to support it.
> 
> Aside from the rcu lock/unlock, isn't it better to not make separate
> calls to radix_tree_lookup and merge them into one call? Similar
> approach is used with pagevec_lookup which is usually used when one
> needs to deal with a set of pages.

Yeah, batching is generally good, however find_get_pages() is not the
right tool. It costs:
- get/release page counts
- likely a lot more searches in the address space, because it does not
  limit the end index of the search.

radix_tree_next_hole() will be the right tool, and I have a patch to
make it actually smarter than the current dumb loop.

> >>Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>---
> >> mm/readahead.c | 31 +++++++++++++++++++++++--------
> >> 1 file changed, 23 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/mm/readahead.c b/mm/readahead.c
> >>index 3977455..3a1798d 100644
> >>--- a/mm/readahead.c
> >>+++ b/mm/readahead.c
> >>@@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >> {
> >> 	struct inode *inode = mapping->host;
> >> 	struct page *page;
> >>+	struct page **page_list = NULL;
> >> 	unsigned long end_index;	/* The last page we want to read */
> >> 	LIST_HEAD(page_pool);
> >> 	int page_idx;
> >> 	int ret = 0;
> >> 	int ret_read = 0;
> >>+	unsigned long num;
> >>+	pgoff_t page_offset;
> >> 	loff_t isize = i_size_read(inode);
> >>
> >> 	if (isize == 0)
> >> 		goto out;
> >>
> >>+	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
> >>+	if (!page_list)
> >>+		goto out;
> >
> >That cost one more memory allocation and added code to maintain the
> >page list. The original code also don't have the cost of grabbing the
> >page count, which eliminate the trouble of page release.
> >
> >> 	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
> >>+	num = find_get_pages(mapping, offset, nr_to_read, page_list);
> >
> >Assume we want to readahead pages for indexes [0, 100] and the cached
> >pages are in [1000, 1100]. find_get_pages() will return the latter.
> >Which is probably not the your expected results.
> 
> I thought if I ask for pages in the range [0,100] it will return a
> sparse array [0,100] but with holes (NULL) for pages not in cache
> and references to pages in cache. Isn't that the expected behavior?

Nope. The comments above find_get_pages() made it clear, that it's
limited by the number of pages rather than the end page index.

> >
> >> 	/*
> >> 	 * Preallocate as many pages as we will need.
> >> 	 */
> >> 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
> >>-		pgoff_t page_offset = offset + page_idx;
> >>+		if (page_list[page_idx]) {
> >>+			page_cache_release(page_list[page_idx]);
> >>+			continue;
> >>+		}
> >>+
> >>+		page_offset = offset + page_idx;
> >>
> >> 		if (page_offset > end_index)
> >> 			break;
> >>
> >>-		rcu_read_lock();
> >>-		page = radix_tree_lookup(&mapping->page_tree, page_offset);
> >>-		rcu_read_unlock();
> >>-		if (page)
> >>-			continue;
> >>-
> >> 		page = page_cache_alloc_readahead(mapping);
> >>-		if (!page)
> >>+		if (unlikely(!page))
> >> 			break;
> >
> >That break will leave the remaining pages' page_count lifted and lead
> >to memory leak.
> 
> Thanks. Yes, I realized that now.
> >
> >> 		page->index = page_offset;
> >> 		list_add(&page->lru, &page_pool);
> >>@@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >> 			lookahead_size = 0;
> >> 		}
> >> 		ret++;
> >>+
> >>+		/*
> >>+		 * Since num pages are already returned, bail out after
> >>+		 * nr_to_read - num pages are allocated and added.
> >>+		 */
> >>+		if (ret == nr_to_read - num)
> >>+			break;
> >
> >Confused. That break seems unnecessary?
> 
> I fixed that:
> 
> 
>  -               pgoff_t page_offset = offset + page_idx;
>  -
>  -               if (page_offset > end_index)
>  -                       break;
>  -
>  -               rcu_read_lock();
>  -               page = radix_tree_lookup(&mapping->page_tree, page_offset);
>  -               rcu_read_unlock();
>  -               if (page)

>  +               if (page_list[page_idx]) {
>  +                       page_cache_release(page_list[page_idx]);

No, you cannot expect:

        page_list[page_idx]->index == page_idx

Thanks,
Fengguang


>  +                       num--;
>                          continue;
>  +               }
>  +
>  +               page_offset = offset + page_idx;
>  +
>  +               /*
>  +                * Break only if all the previous
>  +                * references have been released
>  +                */
>  +               if (page_offset > end_index) {
>  +                       if (!num)
>  +                               break;
>  +                       else
>  +                               continue;
>  +               }
> 
>                  page = page_cache_alloc_readahead(mapping);
>  -               if (!page)
>  -                       break;
>  +               if (unlikely(!page))
>  +                       continue;
> 
> >
> >Thanks,
> >Fengguang
> >
> >> 	}
> >>
> >> 	/*
> >>@@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >> 		ret_read = read_pages(mapping, filp, &page_pool, ret);
> >> 	BUG_ON(!list_empty(&page_pool));
> >> out:
> >>+	kfree(page_list);
> >> 	return (ret_read < 0 ? ret_read : ret);
> >> }
> >>
> >>--
> >>1.7.12.1
> >>
> >>--
> >>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/ .
> >>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >
> 
> 
> 
> 
> Regards,
> -- 
> Raghavendra Prabhu
> GPG Id : 0xD72BE977
> Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> www: wnohang.net


--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Raghavendra D Prabhu]

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

Hi,


* On Fri, Sep 28, 2012 at 08:18:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Wed, Sep 26, 2012 at 08:28:20AM +0530, Raghavendra D Prabhu wrote:
>> Hi,
>>
>>
>> * On Sat, Sep 22, 2012 at 09:15:07PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> >On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
>> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> >>
>> >>Instead of running radix_tree_lookup in a loop and lock/unlocking in the
>> >>process, find_get_pages is called once, which returns a page_list, some of which
>> >>are not NULL and are in core.
>> >>
>> >>Also, since find_get_pages returns number of pages, if all pages are already
>> >>cached, it can return early.
>> >>
>> >>This will be mostly helpful when a higher proportion of nr_to_read pages are
>> >>already in the cache, which will mean less locking for page cache hits.
>> >
>> >Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
>> >the benefit of this patch is questionable. Will need real performance
>> >numbers to support it.
>>
>> Aside from the rcu lock/unlock, isn't it better to not make separate
>> calls to radix_tree_lookup and merge them into one call? Similar
>> approach is used with pagevec_lookup which is usually used when one
>> needs to deal with a set of pages.
>
>Yeah, batching is generally good, however find_get_pages() is not the
>right tool. It costs:
>- get/release page counts
>- likely a lot more searches in the address space, because it does not
>  limit the end index of the search.
>
>radix_tree_next_hole() will be the right tool, and I have a patch to
>make it actually smarter than the current dumb loop.

Good to know.

>
>> >>Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> >>---
>> >> mm/readahead.c | 31 +++++++++++++++++++++++--------
>> >> 1 file changed, 23 insertions(+), 8 deletions(-)
>> >>
>> >>diff --git a/mm/readahead.c b/mm/readahead.c
>> >>index 3977455..3a1798d 100644
>> >>--- a/mm/readahead.c
>> >>+++ b/mm/readahead.c
>> >>@@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>> >> {
>> >> 	struct inode *inode = mapping->host;
>> >> 	struct page *page;
>> >>+	struct page **page_list = NULL;
>> >> 	unsigned long end_index;	/* The last page we want to read */
>> >> 	LIST_HEAD(page_pool);
>> >> 	int page_idx;
>> >> 	int ret = 0;
>> >> 	int ret_read = 0;
>> >>+	unsigned long num;
>> >>+	pgoff_t page_offset;
>> >> 	loff_t isize = i_size_read(inode);
>> >>
>> >> 	if (isize == 0)
>> >> 		goto out;
>> >>
>> >>+	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
>> >>+	if (!page_list)
>> >>+		goto out;
>> >
>> >That cost one more memory allocation and added code to maintain the
>> >page list. The original code also don't have the cost of grabbing the
>> >page count, which eliminate the trouble of page release.
>> >
>> >> 	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
>> >>+	num = find_get_pages(mapping, offset, nr_to_read, page_list);
>> >
>> >Assume we want to readahead pages for indexes [0, 100] and the cached
>> >pages are in [1000, 1100]. find_get_pages() will return the latter.
>> >Which is probably not the your expected results.
>>
>> I thought if I ask for pages in the range [0,100] it will return a
>> sparse array [0,100] but with holes (NULL) for pages not in cache
>> and references to pages in cache. Isn't that the expected behavior?
>
>Nope. The comments above find_get_pages() made it clear, that it's
>limited by the number of pages rather than the end page index.

Yes, I noticed that, however since nr_to_read in this case is 
equal to nr_pages.

However, I think I understand what you are saying -- ie. if 
offset +  nr_pages exceeds the end_index then it will return 
pages not belonging to the file, is that right?

In that case, won't capping nr_pages do, ie. check if offset + 
nr_pages > end_index  and if that is true, then reduce  
nr_to_read by end_index. Won't that work?

>
>> >
>> >> 	/*
>> >> 	 * Preallocate as many pages as we will need.
>> >> 	 */
>> >> 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
>> >>-		pgoff_t page_offset = offset + page_idx;
>> >>+		if (page_list[page_idx]) {
>> >>+			page_cache_release(page_list[page_idx]);
>> >>+			continue;
>> >>+		}
>> >>+
>> >>+		page_offset = offset + page_idx;
>> >>
>> >> 		if (page_offset > end_index)
>> >> 			break;
>> >>
>> >>-		rcu_read_lock();
>> >>-		page = radix_tree_lookup(&mapping->page_tree, page_offset);
>> >>-		rcu_read_unlock();
>> >>-		if (page)
>> >>-			continue;
>> >>-
>> >> 		page = page_cache_alloc_readahead(mapping);
>> >>-		if (!page)
>> >>+		if (unlikely(!page))
>> >> 			break;
>> >
>> >That break will leave the remaining pages' page_count lifted and lead
>> >to memory leak.
>>
>> Thanks. Yes, I realized that now.
>> >
>> >> 		page->index = page_offset;
>> >> 		list_add(&page->lru, &page_pool);
>> >>@@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>> >> 			lookahead_size = 0;
>> >> 		}
>> >> 		ret++;
>> >>+
>> >>+		/*
>> >>+		 * Since num pages are already returned, bail out after
>> >>+		 * nr_to_read - num pages are allocated and added.
>> >>+		 */
>> >>+		if (ret == nr_to_read - num)
>> >>+			break;
>> >
>> >Confused. That break seems unnecessary?
>>
>> I fixed that:
>>
>>
>>  -               pgoff_t page_offset = offset + page_idx;
>>  -
>>  -               if (page_offset > end_index)
>>  -                       break;
>>  -
>>  -               rcu_read_lock();
>>  -               page = radix_tree_lookup(&mapping->page_tree, page_offset);
>>  -               rcu_read_unlock();
>>  -               if (page)
>
>>  +               if (page_list[page_idx]) {
>>  +                       page_cache_release(page_list[page_idx]);
>
>No, you cannot expect:
>
>        page_list[page_idx]->index == page_idx
>
>Thanks,
>Fengguang
>
>
>>  +                       num--;
>>                          continue;
>>  +               }
>>  +
>>  +               page_offset = offset + page_idx;
>>  +
>>  +               /*
>>  +                * Break only if all the previous
>>  +                * references have been released
>>  +                */
>>  +               if (page_offset > end_index) {
>>  +                       if (!num)
>>  +                               break;
>>  +                       else
>>  +                               continue;
>>  +               }
>>
>>                  page = page_cache_alloc_readahead(mapping);
>>  -               if (!page)
>>  -                       break;
>>  +               if (unlikely(!page))
>>  +                       continue;
>>
>> >
>> >Thanks,
>> >Fengguang
>> >
>> >> 	}
>> >>
>> >> 	/*
>> >>@@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
>> >> 		ret_read = read_pages(mapping, filp, &page_pool, ret);
>> >> 	BUG_ON(!list_empty(&page_pool));
>> >> out:
>> >>+	kfree(page_list);
>> >> 	return (ret_read < 0 ? ret_read : ret);
>> >> }
>> >>
>> >>--
>> >>1.7.12.1
>> >>
>> >>--
>> >>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/ .
>> >>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>> >
>>
>>
>>
>>
>> Regards,
>> --
>> Raghavendra Prabhu
>> GPG Id : 0xD72BE977
>> Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
>> www: wnohang.net
>
>




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

Re: Re: Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Raghavendra D Prabhu]

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

Hi,


* On Fri, Sep 28, 2012 at 07:56:23PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Wed, Sep 26, 2012 at 06:59:00AM +0530, Raghavendra D Prabhu wrote:
>> Hi,
>>
>>
>> * On Sat, Sep 22, 2012 at 08:49:20PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> >On Sat, Sep 22, 2012 at 04:03:11PM +0530, raghu.prabhu13@gmail.com wrote:
>> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> >>
>> >>If page lookup from radix_tree_lookup is successful and its index page_idx ==
>> >>nr_to_read - lookahead_size, then SetPageReadahead never gets called, so this
>> >>fixes that.
>> >
>> >NAK. Sorry. It's actually an intentional behavior, so that for the
>> >common cases of many cached files that are accessed frequently, no
>> >PG_readahead will be set at all to pointlessly trap into the readahead
>> >routines once and again.
>>
>> ACK, thanks for explaining that. However, regarding this, I would
>> like to know if the implications of the patch
>> 51daa88ebd8e0d437289f589af29d4b39379ea76 will still apply if
>> PG_readahead is not set.
>
>Would you elaborate the implication and the possible problematic case?

Certainly.


An implication of 51daa88ebd8e0d437289f589af29d4b39379ea76 is 
that, a redundant check for PageReadahead(page) is avoided since  
async is piggy-backed into the synchronous readahead itself.


So, in case of 

     page = find_get_page()
     if(!page)
         page_cache_sync_readahead()
     else if (PageReadahead(page))
         page_cache_async_readahead();

isnt' there a possibility that PG_readahead won't be set at all 
if page is not in cache (causing page_cache_sync_readahead) but 
page at index  (nr_to_read - lookahead_size) is already in the 
cache? (due to if (page) continue; in the code)?


Hence, I changed the condition from equality to >= for setting 
SetPageReadahead(page) (and added a 
variable so that it is done only once). 


>
>Thanks,
>Fengguang
>




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

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

[Raghavendra D Prabhu]

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

Hi,


* On Fri, Sep 28, 2012 at 07:54:05PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Wed, Sep 26, 2012 at 06:55:03AM +0530, Raghavendra D Prabhu wrote:
>>
>> 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.
>
>force_page_cache_readahead()'s return value, in turn, is never used by
>its callers..
Yes, it is not called by its callers, however, since it is called 
in a loop, shouldn't we bail out if force_page_cache_readahead 
fails once? Without the appropriate return value, it will 
continue  and  in 

force_page_cache_readahead


		if (err < 0) {
			ret = err;
			break;
		}

	is never hit.
Nor does the other __do_page_cache_readahead() callers
>care about the error state. So until we find an actual user of the
>error code, I'd recommend to avoid changing the current code.
>
>Thanks,
>Fengguang
>




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

Re: Re: [PATCH 4/5] Move the check for ra_pages after VM_SequentialReadHint()

[Raghavendra D Prabhu]

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

Hi,


* On Sat, Sep 22, 2012 at 08:42:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>it.rprabhu@wnohang.net>
>User-Agent: Mutt/1.5.21 (2010-09-15)
>X-Date: Sat Sep 22 18:12:50 IST 2012
>
>On Sat, Sep 22, 2012 at 04:03:13PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> page_cache_sync_readahead checks for ra->ra_pages again, so moving the check
>> after VM_SequentialReadHint.
>
>Well it depends on what case you are optimizing for. I suspect there
>are much more tmpfs users than VM_SequentialReadHint users. So this
>change is actually not desirable wrt the more widely used cases.

shm/tmpfs doesn't use this function for fault. They have 
shmem_fault for that.  So, that shouldn't matter here. Agree?

>
>Thanks,
>Fengguang
>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  mm/filemap.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 3843445..606a648 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -1523,8 +1523,6 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>>  	/* If we don't want any read-ahead, don't bother */
>>  	if (VM_RandomReadHint(vma))
>>  		return;
>> -	if (!ra->ra_pages)
>> -		return;
>>
>>  	if (VM_SequentialReadHint(vma)) {
>>  		page_cache_sync_readahead(mapping, ra, file, offset,
>> @@ -1532,6 +1530,9 @@ static void do_sync_mmap_readahead(struct vm_area_struct *vma,
>>  		return;
>>  	}
>>
>> +	if (!ra->ra_pages)
>> +		return;
>> +
>>  	/* Avoid banging the cache line if not needed */
>>  	if (ra->mmap_miss < MMAP_LOTSAMISS * 10)
>>  		ra->mmap_miss++;
>> --
>> 1.7.12.1
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see




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

Re: Re: [PATCH 3/5] Remove file_ra_state from arguments of count_history_pages.

[Raghavendra D Prabhu]

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

Hi,


* On Sat, Sep 22, 2012 at 08:40:28PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
>On Sat, Sep 22, 2012 at 04:03:12PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> count_history_pages doesn't require readahead state to calculate the offset from history.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>
>Acked-by: Fengguang Wu <fengguang.wu@intel.com>
>

Good. Do I need do anything else to get this into mm-tree? Few months 
back, I had sent and few were acked, but they didn't end up 
anywhere.




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

Re: Re: Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Fengguang Wu]

On Tue, Oct 16, 2012 at 10:29:31PM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Fri, Sep 28, 2012 at 08:18:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Wed, Sep 26, 2012 at 08:28:20AM +0530, Raghavendra D Prabhu wrote:
> >>Hi,
> >>
> >>
> >>* On Sat, Sep 22, 2012 at 09:15:07PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >>>On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
> >>>>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>>>
> >>>>Instead of running radix_tree_lookup in a loop and lock/unlocking in the
> >>>>process, find_get_pages is called once, which returns a page_list, some of which
> >>>>are not NULL and are in core.
> >>>>
> >>>>Also, since find_get_pages returns number of pages, if all pages are already
> >>>>cached, it can return early.
> >>>>
> >>>>This will be mostly helpful when a higher proportion of nr_to_read pages are
> >>>>already in the cache, which will mean less locking for page cache hits.
> >>>
> >>>Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
> >>>the benefit of this patch is questionable. Will need real performance
> >>>numbers to support it.
> >>
> >>Aside from the rcu lock/unlock, isn't it better to not make separate
> >>calls to radix_tree_lookup and merge them into one call? Similar
> >>approach is used with pagevec_lookup which is usually used when one
> >>needs to deal with a set of pages.
> >
> >Yeah, batching is generally good, however find_get_pages() is not the
> >right tool. It costs:
> >- get/release page counts
> >- likely a lot more searches in the address space, because it does not
> > limit the end index of the search.
> >
> >radix_tree_next_hole() will be the right tool, and I have a patch to
> >make it actually smarter than the current dumb loop.
> 
> Good to know.
> 
> >
> >>>>Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>>>---
> >>>> mm/readahead.c | 31 +++++++++++++++++++++++--------
> >>>> 1 file changed, 23 insertions(+), 8 deletions(-)
> >>>>
> >>>>diff --git a/mm/readahead.c b/mm/readahead.c
> >>>>index 3977455..3a1798d 100644
> >>>>--- a/mm/readahead.c
> >>>>+++ b/mm/readahead.c
> >>>>@@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> {
> >>>> 	struct inode *inode = mapping->host;
> >>>> 	struct page *page;
> >>>>+	struct page **page_list = NULL;
> >>>> 	unsigned long end_index;	/* The last page we want to read */
> >>>> 	LIST_HEAD(page_pool);
> >>>> 	int page_idx;
> >>>> 	int ret = 0;
> >>>> 	int ret_read = 0;
> >>>>+	unsigned long num;
> >>>>+	pgoff_t page_offset;
> >>>> 	loff_t isize = i_size_read(inode);
> >>>>
> >>>> 	if (isize == 0)
> >>>> 		goto out;
> >>>>
> >>>>+	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
> >>>>+	if (!page_list)
> >>>>+		goto out;
> >>>
> >>>That cost one more memory allocation and added code to maintain the
> >>>page list. The original code also don't have the cost of grabbing the
> >>>page count, which eliminate the trouble of page release.
> >>>
> >>>> 	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
> >>>>+	num = find_get_pages(mapping, offset, nr_to_read, page_list);
> >>>
> >>>Assume we want to readahead pages for indexes [0, 100] and the cached
> >>>pages are in [1000, 1100]. find_get_pages() will return the latter.
> >>>Which is probably not the your expected results.
> >>
> >>I thought if I ask for pages in the range [0,100] it will return a
> >>sparse array [0,100] but with holes (NULL) for pages not in cache
> >>and references to pages in cache. Isn't that the expected behavior?
> >
> >Nope. The comments above find_get_pages() made it clear, that it's
> >limited by the number of pages rather than the end page index.
> 
> Yes, I noticed that, however since nr_to_read in this case is equal
> to nr_pages.
> 
> However, I think I understand what you are saying -- ie. if offset +

You seem to still don't understand it after all the explanations..

> nr_pages exceeds the end_index then it will return pages not
> belonging to the file, is that right?

Nope.

> In that case, won't capping nr_pages do, ie. check if offset +
> nr_pages > end_index  and if that is true, then reduce  nr_to_read
> by end_index. Won't that work?

Either you or me are get lost in the discussion. Please research the
code and emails and show me working code (that's actually tested) that
can serve as the new base for our discussions.

Thanks,
Fengguang

> >>>> 	/*
> >>>> 	 * Preallocate as many pages as we will need.
> >>>> 	 */
> >>>> 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
> >>>>-		pgoff_t page_offset = offset + page_idx;
> >>>>+		if (page_list[page_idx]) {
> >>>>+			page_cache_release(page_list[page_idx]);
> >>>>+			continue;
> >>>>+		}
> >>>>+
> >>>>+		page_offset = offset + page_idx;
> >>>>
> >>>> 		if (page_offset > end_index)
> >>>> 			break;
> >>>>
> >>>>-		rcu_read_lock();
> >>>>-		page = radix_tree_lookup(&mapping->page_tree, page_offset);
> >>>>-		rcu_read_unlock();
> >>>>-		if (page)
> >>>>-			continue;
> >>>>-
> >>>> 		page = page_cache_alloc_readahead(mapping);
> >>>>-		if (!page)
> >>>>+		if (unlikely(!page))
> >>>> 			break;
> >>>
> >>>That break will leave the remaining pages' page_count lifted and lead
> >>>to memory leak.
> >>
> >>Thanks. Yes, I realized that now.
> >>>
> >>>> 		page->index = page_offset;
> >>>> 		list_add(&page->lru, &page_pool);
> >>>>@@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> 			lookahead_size = 0;
> >>>> 		}
> >>>> 		ret++;
> >>>>+
> >>>>+		/*
> >>>>+		 * Since num pages are already returned, bail out after
> >>>>+		 * nr_to_read - num pages are allocated and added.
> >>>>+		 */
> >>>>+		if (ret == nr_to_read - num)
> >>>>+			break;
> >>>
> >>>Confused. That break seems unnecessary?
> >>
> >>I fixed that:
> >>
> >>
> >> -               pgoff_t page_offset = offset + page_idx;
> >> -
> >> -               if (page_offset > end_index)
> >> -                       break;
> >> -
> >> -               rcu_read_lock();
> >> -               page = radix_tree_lookup(&mapping->page_tree, page_offset);
> >> -               rcu_read_unlock();
> >> -               if (page)
> >
> >> +               if (page_list[page_idx]) {
> >> +                       page_cache_release(page_list[page_idx]);
> >
> >No, you cannot expect:
> >
> >       page_list[page_idx]->index == page_idx
> >
> >Thanks,
> >Fengguang
> >
> >
> >> +                       num--;
> >>                         continue;
> >> +               }
> >> +
> >> +               page_offset = offset + page_idx;
> >> +
> >> +               /*
> >> +                * Break only if all the previous
> >> +                * references have been released
> >> +                */
> >> +               if (page_offset > end_index) {
> >> +                       if (!num)
> >> +                               break;
> >> +                       else
> >> +                               continue;
> >> +               }
> >>
> >>                 page = page_cache_alloc_readahead(mapping);
> >> -               if (!page)
> >> -                       break;
> >> +               if (unlikely(!page))
> >> +                       continue;
> >>
> >>>
> >>>Thanks,
> >>>Fengguang
> >>>
> >>>> 	}
> >>>>
> >>>> 	/*
> >>>>@@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> 		ret_read = read_pages(mapping, filp, &page_pool, ret);
> >>>> 	BUG_ON(!list_empty(&page_pool));
> >>>> out:
> >>>>+	kfree(page_list);
> >>>> 	return (ret_read < 0 ? ret_read : ret);
> >>>> }
> >>>>
> >>>>--
> >>>>1.7.12.1
> >>>>
> >>>>--
> >>>>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/ .
> >>>>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >>>
> >>
> >>
> >>
> >>
> >>Regards,
> >>--
> >>Raghavendra Prabhu
> >>GPG Id : 0xD72BE977
> >>Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> >>www: wnohang.net
> >
> >
> 
> 
> 
> 
> Regards,
> -- 
> Raghavendra Prabhu
> GPG Id : 0xD72BE977
> Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> www: wnohang.net


--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: Re: [PATCH 2/5] mm/readahead: Change the condition for SetPageReadahead

[Fengguang Wu]

Hi Raghavendra,

> An implication of 51daa88ebd8e0d437289f589af29d4b39379ea76 is that,
> a redundant check for PageReadahead(page) is avoided since  async is
> piggy-backed into the synchronous readahead itself.

That's right.

> So, in case of
> 
>     page = find_get_page()
>     if(!page)
>         page_cache_sync_readahead()
>     else if (PageReadahead(page))
>         page_cache_async_readahead();
> 
> isnt' there a possibility that PG_readahead won't be set at all if
> page is not in cache (causing page_cache_sync_readahead) but page at
> index  (nr_to_read - lookahead_size) is already in the cache? (due
> to if (page) continue; in the code)?

Yes, and I'm fully aware of that. It's left alone because it's assumed
to be a rare case. The nature of readahead is, there are all kinds of
less common cases that we deliberately ignore in order to keep the
code simple and maintainable.

> Hence, I changed the condition from equality to >= for setting
> SetPageReadahead(page) (and added a variable so that it is done only
> once).

It's excellent that you noticed that case. And sorry that I come to
realize that your change

-               if (page_idx == nr_to_read - lookahead_size)
+               if (page_idx >= nr_to_read - lookahead_size) {
                        SetPageReadahead(page);
+                       lookahead_size = 0;
+               }

won't negatively impact cache hot reads. So I have no strong feelings
about the patch now.

Thanks,
Fengguang

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Fengguang Wu]

On Tue, Oct 16, 2012 at 11:17:05PM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Fri, Sep 28, 2012 at 07:54:05PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Wed, Sep 26, 2012 at 06:55:03AM +0530, Raghavendra D Prabhu wrote:
> >>
> >>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.
> >
> >force_page_cache_readahead()'s return value, in turn, is never used by
> >its callers..
> Yes, it is not called by its callers, however, since it is called in
> a loop, shouldn't we bail out if force_page_cache_readahead fails
> once? Without the appropriate return value, it will continue  and
> in
> 
> force_page_cache_readahead
> 
> 
> 		if (err < 0) {
> 			ret = err;
> 			break;
> 		}
> 
> 	is never hit.
> Nor does the other __do_page_cache_readahead() callers

That sounds all reasonable, but please don't change the meaning of
__do_page_cache_readahead()'s return value. It should always return
the number of new pages put to IO, which will be used by some
readahead tracing/accounting feature. So it will need another parameter
for passing the error code from ->readpages(). However since the major
filesystems always return 0 in ->readpages(), I'm not sure it worth
the efforts.

Thanks,
Fengguang

> >care about the error state. So until we find an actual user of the
> >error code, I'd recommend to avoid changing the current code.
> >
> >Thanks,
> >Fengguang
> >

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH 4/5] Move the check for ra_pages after VM_SequentialReadHint()

[Fengguang Wu]

On Tue, Oct 16, 2012 at 11:45:21PM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Sat, Sep 22, 2012 at 08:42:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >it.rprabhu@wnohang.net>
> >User-Agent: Mutt/1.5.21 (2010-09-15)
> >X-Date: Sat Sep 22 18:12:50 IST 2012
> >
> >On Sat, Sep 22, 2012 at 04:03:13PM +0530, raghu.prabhu13@gmail.com wrote:
> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>
> >>page_cache_sync_readahead checks for ra->ra_pages again, so moving the check
> >>after VM_SequentialReadHint.
> >
> >Well it depends on what case you are optimizing for. I suspect there
> >are much more tmpfs users than VM_SequentialReadHint users. So this
> >change is actually not desirable wrt the more widely used cases.
> 
> shm/tmpfs doesn't use this function for fault. They have shmem_fault
> for that.  So, that shouldn't matter here. Agree?

That's true for the regular tmpfs and it still calls filemap_fault()
in the !CONFIG_SHMEM case and squashfs/cramfs etc. They together
should still overweight the VM_SequentialReadHint users?

Thanks,
Fengguang

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH 3/5] Remove file_ra_state from arguments of count_history_pages.

[Fengguang Wu]

On Tue, Oct 16, 2012 at 11:51:08PM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Sat, Sep 22, 2012 at 08:40:28PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Sat, Sep 22, 2012 at 04:03:12PM +0530, raghu.prabhu13@gmail.com wrote:
> >>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>
> >>count_history_pages doesn't require readahead state to calculate the offset from history.
> >>
> >>Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >
> >Acked-by: Fengguang Wu <fengguang.wu@intel.com>
> >
> 
> Good. Do I need do anything else to get this into mm-tree? Few
> months back, I had sent and few were acked, but they didn't end up
> anywhere.

Raghavendra, you may repost a revised patchset to the lists to move
things forward.

Thanks,
Fengguang

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>