[PATCH 0/7] Overhaul find_get_entries and pagevec_lookup_entries

[PATCH 0/7] Overhaul find_get_entries and pagevec_lookup_entries

[Matthew Wilcox (Oracle)]

This started out as part of the THP patchset, but it's turned into a
nice simplification in its own right.  Essentially we end up unifying
find_get_entries() and pagevec_lookup_entries() into one function that's
better than either, and we get rid of a lot of code in the callers as
a result.

I'm running this through xfstests right now, but something similar to
this has already passed xfstests as part of the THP patchset.

I've done my best to avoid off-by-one errors for 'end', but I wouldn't be
surprised if I made a mistake.  We're not consistent with whether 'end'
is inclusive or exclusive and I didn't want to make extensive changes
to ensure they were consistent.

Matthew Wilcox (Oracle) (7):
  mm: Use pagevec_lookup in shmem_unlock_mapping
  mm: Rewrite shmem_seek_hole_data
  mm: Add an 'end' parameter to find_get_entries
  mm: Add an 'end' parameter to pagevec_lookup_entries
  mm: Remove nr_entries parameter from pagevec_lookup_entries
  mm: Pass pvec directly to find_get_entries
  mm: Remove pagevec_lookup_entries

 include/linux/pagemap.h |  3 +-
 include/linux/pagevec.h |  4 --
 mm/filemap.c            | 19 +++++----
 mm/shmem.c              | 85 ++++++++++++++---------------------------
 mm/swap.c               | 38 +-----------------
 mm/truncate.c           | 33 +++-------------
 6 files changed, 45 insertions(+), 137 deletions(-)

-- 
2.28.0

[PATCH 1/7] mm: Use pagevec_lookup in shmem_unlock_mapping

[Matthew Wilcox (Oracle)]

The comment shows that the reason for using find_get_entries() is now
stale; find_get_pages() will not return 0 if it hits a consecutive run
of swap entries, and I don't believe it has since 2011.  pagevec_lookup()
is a simpler function to use than find_get_pages(), so use it instead.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/shmem.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 271548ca20f3..a7bbc4ed9677 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -840,7 +840,6 @@ unsigned long shmem_swap_usage(struct vm_area_struct *vma)
 void shmem_unlock_mapping(struct address_space *mapping)
 {
 	struct pagevec pvec;
-	pgoff_t indices[PAGEVEC_SIZE];
 	pgoff_t index = 0;
 
 	pagevec_init(&pvec);
@@ -848,16 +847,8 @@ void shmem_unlock_mapping(struct address_space *mapping)
 	 * Minor point, but we might as well stop if someone else SHM_LOCKs it.
 	 */
 	while (!mapping_unevictable(mapping)) {
-		/*
-		 * Avoid pagevec_lookup(): find_get_pages() returns 0 as if it
-		 * has finished, if it hits a row of PAGEVEC_SIZE swap entries.
-		 */
-		pvec.nr = find_get_entries(mapping, index,
-					   PAGEVEC_SIZE, pvec.pages, indices);
-		if (!pvec.nr)
+		if (!pagevec_lookup(&pvec, mapping, &index))
 			break;
-		index = indices[pvec.nr - 1] + 1;
-		pagevec_remove_exceptionals(&pvec);
 		check_move_unevictable_pages(&pvec);
 		pagevec_release(&pvec);
 		cond_resched();
-- 
2.28.0

Re: [PATCH 1/7] mm: Use pagevec_lookup in shmem_unlock_mapping

[Jan Kara]

On Wed 19-08-20 16:05:49, Matthew Wilcox (Oracle) wrote:
> The comment shows that the reason for using find_get_entries() is now
> stale; find_get_pages() will not return 0 if it hits a consecutive run
> of swap entries, and I don't believe it has since 2011.  pagevec_lookup()
> is a simpler function to use than find_get_pages(), so use it instead.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

This looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/shmem.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 271548ca20f3..a7bbc4ed9677 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -840,7 +840,6 @@ unsigned long shmem_swap_usage(struct vm_area_struct *vma)
>  void shmem_unlock_mapping(struct address_space *mapping)
>  {
>  	struct pagevec pvec;
> -	pgoff_t indices[PAGEVEC_SIZE];
>  	pgoff_t index = 0;
>  
>  	pagevec_init(&pvec);
> @@ -848,16 +847,8 @@ void shmem_unlock_mapping(struct address_space *mapping)
>  	 * Minor point, but we might as well stop if someone else SHM_LOCKs it.
>  	 */
>  	while (!mapping_unevictable(mapping)) {
> -		/*
> -		 * Avoid pagevec_lookup(): find_get_pages() returns 0 as if it
> -		 * has finished, if it hits a row of PAGEVEC_SIZE swap entries.
> -		 */
> -		pvec.nr = find_get_entries(mapping, index,
> -					   PAGEVEC_SIZE, pvec.pages, indices);
> -		if (!pvec.nr)
> +		if (!pagevec_lookup(&pvec, mapping, &index))
>  			break;
> -		index = indices[pvec.nr - 1] + 1;
> -		pagevec_remove_exceptionals(&pvec);
>  		check_move_unevictable_pages(&pvec);
>  		pagevec_release(&pvec);
>  		cond_resched();
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/7] mm: Rewrite shmem_seek_hole_data

[Matthew Wilcox (Oracle)]

use the XArray directly instead of using the pagevec abstraction.
The code is simpler and more efficient.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/shmem.c | 61 +++++++++++++++++++++---------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index a7bbc4ed9677..0f9f149f4b5e 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2659,53 +2659,40 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 }
 
 /*
- * llseek SEEK_DATA or SEEK_HOLE through the page cache.
+ * llseek SEEK_DATA or SEEK_HOLE through the page cache.  We don't need
+ * to get a reference on the page because this interface is racy anyway.
+ * The page we find will have had the state at some point.
  */
 static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
 				    pgoff_t index, pgoff_t end, int whence)
 {
+	XA_STATE(xas, &mapping->i_pages, index);
 	struct page *page;
-	struct pagevec pvec;
-	pgoff_t indices[PAGEVEC_SIZE];
-	bool done = false;
-	int i;
 
-	pagevec_init(&pvec);
-	pvec.nr = 1;		/* start small: we may be there already */
-	while (!done) {
-		pvec.nr = find_get_entries(mapping, index,
-					pvec.nr, pvec.pages, indices);
-		if (!pvec.nr) {
-			if (whence == SEEK_DATA)
-				index = end;
-			break;
+	rcu_read_lock();
+	if (whence == SEEK_DATA) {
+		for (;;) {
+			page = xas_find(&xas, end);
+			if (xas_retry(&xas, page))
+				continue;
+			if (!page || xa_is_value(page) || PageUptodate(page))
+				break;
 		}
-		for (i = 0; i < pvec.nr; i++, index++) {
-			if (index < indices[i]) {
-				if (whence == SEEK_HOLE) {
-					done = true;
-					break;
-				}
-				index = indices[i];
-			}
-			page = pvec.pages[i];
-			if (page && !xa_is_value(page)) {
-				if (!PageUptodate(page))
-					page = NULL;
-			}
-			if (index >= end ||
-			    (page && whence == SEEK_DATA) ||
-			    (!page && whence == SEEK_HOLE)) {
-				done = true;
+	} else /* SEEK_HOLE */ {
+		for (;;) {
+			page = xas_next(&xas);
+			if (xas_retry(&xas, page))
+				continue;
+			if (!xa_is_value(page) &&
+					(!page || !PageUptodate(page)))
+				break;
+			if (xas.xa_index >= end)
 				break;
-			}
 		}
-		pagevec_remove_exceptionals(&pvec);
-		pagevec_release(&pvec);
-		pvec.nr = PAGEVEC_SIZE;
-		cond_resched();
 	}
-	return index;
+	rcu_read_unlock();
+
+	return xas.xa_index;
 }
 
 static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
-- 
2.28.0

Re: [PATCH 2/7] mm: Rewrite shmem_seek_hole_data

[Mike Rapoport]

On Wed, Aug 19, 2020 at 04:05:50PM +0100, Matthew Wilcox (Oracle) wrote:
> use the XArray directly instead of using the pagevec abstraction.
> The code is simpler and more efficient.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/shmem.c | 61 +++++++++++++++++++++---------------------------------
>  1 file changed, 24 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index a7bbc4ed9677..0f9f149f4b5e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2659,53 +2659,40 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  }
>  
>  /*
> - * llseek SEEK_DATA or SEEK_HOLE through the page cache.
> + * llseek SEEK_DATA or SEEK_HOLE through the page cache.  We don't need
> + * to get a reference on the page because this interface is racy anyway.
> + * The page we find will have had the state at some point.

For my non-native ear "will have had" is too complex ;-)

>   */
>  static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
>  				    pgoff_t index, pgoff_t end, int whence)
>  {
> +	XA_STATE(xas, &mapping->i_pages, index);
>  	struct page *page;
> -	struct pagevec pvec;
> -	pgoff_t indices[PAGEVEC_SIZE];
> -	bool done = false;
> -	int i;
>  
> -	pagevec_init(&pvec);
> -	pvec.nr = 1;		/* start small: we may be there already */
> -	while (!done) {
> -		pvec.nr = find_get_entries(mapping, index,
> -					pvec.nr, pvec.pages, indices);
> -		if (!pvec.nr) {
> -			if (whence == SEEK_DATA)
> -				index = end;
> -			break;
> +	rcu_read_lock();
> +	if (whence == SEEK_DATA) {
> +		for (;;) {
> +			page = xas_find(&xas, end);
> +			if (xas_retry(&xas, page))
> +				continue;
> +			if (!page || xa_is_value(page) || PageUptodate(page))
> +				break;
>  		}
> -		for (i = 0; i < pvec.nr; i++, index++) {
> -			if (index < indices[i]) {
> -				if (whence == SEEK_HOLE) {
> -					done = true;
> -					break;
> -				}
> -				index = indices[i];
> -			}
> -			page = pvec.pages[i];
> -			if (page && !xa_is_value(page)) {
> -				if (!PageUptodate(page))
> -					page = NULL;
> -			}
> -			if (index >= end ||
> -			    (page && whence == SEEK_DATA) ||
> -			    (!page && whence == SEEK_HOLE)) {
> -				done = true;
> +	} else /* SEEK_HOLE */ {
> +		for (;;) {
> +			page = xas_next(&xas);
> +			if (xas_retry(&xas, page))
> +				continue;
> +			if (!xa_is_value(page) &&
> +					(!page || !PageUptodate(page)))
> +				break;
> +			if (xas.xa_index >= end)
>  				break;
> -			}
>  		}
> -		pagevec_remove_exceptionals(&pvec);
> -		pagevec_release(&pvec);
> -		pvec.nr = PAGEVEC_SIZE;
> -		cond_resched();
>  	}
> -	return index;
> +	rcu_read_unlock();
> +
> +	return xas.xa_index;
>  }
>  
>  static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> -- 
> 2.28.0
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 2/7] mm: Rewrite shmem_seek_hole_data

[Matthew Wilcox]

On Thu, Aug 20, 2020 at 07:45:46PM +0300, Mike Rapoport wrote:
> > - * llseek SEEK_DATA or SEEK_HOLE through the page cache.
> > + * llseek SEEK_DATA or SEEK_HOLE through the page cache.  We don't need
> > + * to get a reference on the page because this interface is racy anyway.
> > + * The page we find will have had the state at some point.
> 
> For my non-native ear "will have had" is too complex ;-)

Fair!  How about simply "The page we find did have the state at some point".

But now I'm thinking about it some more, and I'm not so sure of it now.
What if we put a page in the cache that was !Uptodate, then we do a
lookup, find this pointer, then the page is removed from the cache,
then it's allocated somewhere else, marked Uptodate, and now we're
scheduled back in and find it's Uptodate, when there was never a page
at this location that was Uptodate?

So maybe I have to retract this patch after all.

> >   */
> >  static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
> >  				    pgoff_t index, pgoff_t end, int whence)
> >  {
> > +	XA_STATE(xas, &mapping->i_pages, index);
> >  	struct page *page;
> > -	struct pagevec pvec;
> > -	pgoff_t indices[PAGEVEC_SIZE];
> > -	bool done = false;
> > -	int i;
> >  
> > -	pagevec_init(&pvec);
> > -	pvec.nr = 1;		/* start small: we may be there already */
> > -	while (!done) {
> > -		pvec.nr = find_get_entries(mapping, index,
> > -					pvec.nr, pvec.pages, indices);
> > -		if (!pvec.nr) {
> > -			if (whence == SEEK_DATA)
> > -				index = end;
> > -			break;
> > +	rcu_read_lock();
> > +	if (whence == SEEK_DATA) {
> > +		for (;;) {
> > +			page = xas_find(&xas, end);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!page || xa_is_value(page) || PageUptodate(page))
> > +				break;
> >  		}
> > -		for (i = 0; i < pvec.nr; i++, index++) {
> > -			if (index < indices[i]) {
> > -				if (whence == SEEK_HOLE) {
> > -					done = true;
> > -					break;
> > -				}
> > -				index = indices[i];
> > -			}
> > -			page = pvec.pages[i];
> > -			if (page && !xa_is_value(page)) {
> > -				if (!PageUptodate(page))
> > -					page = NULL;
> > -			}
> > -			if (index >= end ||
> > -			    (page && whence == SEEK_DATA) ||
> > -			    (!page && whence == SEEK_HOLE)) {
> > -				done = true;
> > +	} else /* SEEK_HOLE */ {
> > +		for (;;) {
> > +			page = xas_next(&xas);
> > +			if (xas_retry(&xas, page))
> > +				continue;
> > +			if (!xa_is_value(page) &&
> > +					(!page || !PageUptodate(page)))
> > +				break;
> > +			if (xas.xa_index >= end)
> >  				break;
> > -			}
> >  		}
> > -		pagevec_remove_exceptionals(&pvec);
> > -		pagevec_release(&pvec);
> > -		pvec.nr = PAGEVEC_SIZE;
> > -		cond_resched();
> >  	}
> > -	return index;
> > +	rcu_read_unlock();
> > +
> > +	return xas.xa_index;
> >  }
> >  
> >  static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> > -- 
> > 2.28.0
> > 
> > 
> 
> -- 
> Sincerely yours,
> Mike.

[PATCH 3/7] mm: Add an 'end' parameter to find_get_entries

[Matthew Wilcox (Oracle)]

This simplifies the callers and leads to a more efficient implementation
since the XArray has this functionality already.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagemap.h |  4 ++--
 mm/filemap.c            |  9 +++++----
 mm/shmem.c              | 10 ++++------
 mm/swap.c               |  2 +-
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 7de11dcd534d..3f0dc8d00f2a 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -387,8 +387,8 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
 struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
 struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
 unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
-			  unsigned int nr_entries, struct page **entries,
-			  pgoff_t *indices);
+		pgoff_t end, unsigned int nr_entries, struct page **entries,
+		pgoff_t *indices);
 unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
 			pgoff_t end, unsigned int nr_pages,
 			struct page **pages);
diff --git a/mm/filemap.c b/mm/filemap.c
index 1aaea26556cc..159cf3d6f1ae 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1742,6 +1742,7 @@ EXPORT_SYMBOL(pagecache_get_page);
  * find_get_entries - gang pagecache lookup
  * @mapping:	The address_space to search
  * @start:	The starting page cache index
+ * @end:	The highest page cache index to return.
  * @nr_entries:	The maximum number of entries
  * @entries:	Where the resulting entries are placed
  * @indices:	The cache indices corresponding to the entries in @entries
@@ -1765,9 +1766,9 @@ EXPORT_SYMBOL(pagecache_get_page);
  *
  * Return: the number of pages and shadow entries which were found.
  */
-unsigned find_get_entries(struct address_space *mapping,
-			  pgoff_t start, unsigned int nr_entries,
-			  struct page **entries, pgoff_t *indices)
+unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
+		pgoff_t end, unsigned int nr_entries, struct page **entries,
+		pgoff_t *indices)
 {
 	XA_STATE(xas, &mapping->i_pages, start);
 	struct page *page;
@@ -1777,7 +1778,7 @@ unsigned find_get_entries(struct address_space *mapping,
 		return 0;
 
 	rcu_read_lock();
-	xas_for_each(&xas, page, ULONG_MAX) {
+	xas_for_each(&xas, page, end) {
 		if (xas_retry(&xas, page))
 			continue;
 		/*
diff --git a/mm/shmem.c b/mm/shmem.c
index 0f9f149f4b5e..abdbe61a1aa7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -906,9 +906,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 	pagevec_init(&pvec);
 	index = start;
 	while (index < end) {
-		pvec.nr = find_get_entries(mapping, index,
-			min(end - index, (pgoff_t)PAGEVEC_SIZE),
-			pvec.pages, indices);
+		pvec.nr = find_get_entries(mapping, index, end - 1,
+				PAGEVEC_SIZE, pvec.pages, indices);
 		if (!pvec.nr)
 			break;
 		for (i = 0; i < pagevec_count(&pvec); i++) {
@@ -977,9 +976,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 	while (index < end) {
 		cond_resched();
 
-		pvec.nr = find_get_entries(mapping, index,
-				min(end - index, (pgoff_t)PAGEVEC_SIZE),
-				pvec.pages, indices);
+		pvec.nr = find_get_entries(mapping, index, end - 1,
+				PAGEVEC_SIZE, pvec.pages, indices);
 		if (!pvec.nr) {
 			/* If all gone or hole-punch or unfalloc, we're done */
 			if (index == start || end != -1)
diff --git a/mm/swap.c b/mm/swap.c
index d16d65d9b4e0..fcf6ccb94b09 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1060,7 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
 				pgoff_t start, unsigned nr_entries,
 				pgoff_t *indices)
 {
-	pvec->nr = find_get_entries(mapping, start, nr_entries,
+	pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
 				    pvec->pages, indices);
 	return pagevec_count(pvec);
 }
-- 
2.28.0

Re: [PATCH 3/7] mm: Add an 'end' parameter to find_get_entries

[Mike Rapoport]

On Wed, Aug 19, 2020 at 04:05:51PM +0100, Matthew Wilcox (Oracle) wrote:
> This simplifies the callers and leads to a more efficient implementation
> since the XArray has this functionality already.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/pagemap.h |  4 ++--
>  mm/filemap.c            |  9 +++++----
>  mm/shmem.c              | 10 ++++------
>  mm/swap.c               |  2 +-
>  4 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 7de11dcd534d..3f0dc8d00f2a 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -387,8 +387,8 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
>  struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
>  struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
>  unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> -			  unsigned int nr_entries, struct page **entries,
> -			  pgoff_t *indices);
> +		pgoff_t end, unsigned int nr_entries, struct page **entries,
> +		pgoff_t *indices);
>  unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
>  			pgoff_t end, unsigned int nr_pages,
>  			struct page **pages);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..159cf3d6f1ae 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1742,6 +1742,7 @@ EXPORT_SYMBOL(pagecache_get_page);
>   * find_get_entries - gang pagecache lookup
>   * @mapping:	The address_space to search
>   * @start:	The starting page cache index
> + * @end:	The highest page cache index to return.

Maybe add here whether 'end' is inclusive or exclusive?

>   * @nr_entries:	The maximum number of entries
>   * @entries:	Where the resulting entries are placed
>   * @indices:	The cache indices corresponding to the entries in @entries
> @@ -1765,9 +1766,9 @@ EXPORT_SYMBOL(pagecache_get_page);
>   *
>   * Return: the number of pages and shadow entries which were found.
>   */
> -unsigned find_get_entries(struct address_space *mapping,
> -			  pgoff_t start, unsigned int nr_entries,
> -			  struct page **entries, pgoff_t *indices)
> +unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> +		pgoff_t end, unsigned int nr_entries, struct page **entries,
> +		pgoff_t *indices)
>  {
>  	XA_STATE(xas, &mapping->i_pages, start);
>  	struct page *page;
> @@ -1777,7 +1778,7 @@ unsigned find_get_entries(struct address_space *mapping,
>  		return 0;
>  
>  	rcu_read_lock();
> -	xas_for_each(&xas, page, ULONG_MAX) {
> +	xas_for_each(&xas, page, end) {
>  		if (xas_retry(&xas, page))
>  			continue;
>  		/*
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0f9f149f4b5e..abdbe61a1aa7 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -906,9 +906,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  	pagevec_init(&pvec);
>  	index = start;
>  	while (index < end) {
> -		pvec.nr = find_get_entries(mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE),
> -			pvec.pages, indices);
> +		pvec.nr = find_get_entries(mapping, index, end - 1,
> +				PAGEVEC_SIZE, pvec.pages, indices);
>  		if (!pvec.nr)
>  			break;
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
> @@ -977,9 +976,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  	while (index < end) {
>  		cond_resched();
>  
> -		pvec.nr = find_get_entries(mapping, index,
> -				min(end - index, (pgoff_t)PAGEVEC_SIZE),
> -				pvec.pages, indices);
> +		pvec.nr = find_get_entries(mapping, index, end - 1,
> +				PAGEVEC_SIZE, pvec.pages, indices);
>  		if (!pvec.nr) {
>  			/* If all gone or hole-punch or unfalloc, we're done */
>  			if (index == start || end != -1)
> diff --git a/mm/swap.c b/mm/swap.c
> index d16d65d9b4e0..fcf6ccb94b09 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1060,7 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
>  				pgoff_t start, unsigned nr_entries,
>  				pgoff_t *indices)
>  {
> -	pvec->nr = find_get_entries(mapping, start, nr_entries,
> +	pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
>  				    pvec->pages, indices);
>  	return pagevec_count(pvec);
>  }
> -- 
> 2.28.0
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 3/7] mm: Add an 'end' parameter to find_get_entries

[Jan Kara]

On Wed 19-08-20 16:05:51, Matthew Wilcox (Oracle) wrote:
> This simplifies the callers and leads to a more efficient implementation
> since the XArray has this functionality already.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

The patch looks good to me. Just I'd note that you could drop some:

	if (index >= end)
		break;

checks in shmem_undo_range() as well.

In the past I was considering moving find_get_entries() to the same API as
find_get_pages_range() has (which is essentially what you do now, but I
also had 'start' to be a pgoff_t * so that we can return there where the
iteration ended in the range). But in the end I've decided the churn is not
worth the few removed lines and didn't push the patch in the end. What you
did in this patch seems to be a reasonable middle-ground :)

								Honza
> ---
>  include/linux/pagemap.h |  4 ++--
>  mm/filemap.c            |  9 +++++----
>  mm/shmem.c              | 10 ++++------
>  mm/swap.c               |  2 +-
>  4 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 7de11dcd534d..3f0dc8d00f2a 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -387,8 +387,8 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
>  struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
>  struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
>  unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> -			  unsigned int nr_entries, struct page **entries,
> -			  pgoff_t *indices);
> +		pgoff_t end, unsigned int nr_entries, struct page **entries,
> +		pgoff_t *indices);
>  unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
>  			pgoff_t end, unsigned int nr_pages,
>  			struct page **pages);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..159cf3d6f1ae 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1742,6 +1742,7 @@ EXPORT_SYMBOL(pagecache_get_page);
>   * find_get_entries - gang pagecache lookup
>   * @mapping:	The address_space to search
>   * @start:	The starting page cache index
> + * @end:	The highest page cache index to return.
>   * @nr_entries:	The maximum number of entries
>   * @entries:	Where the resulting entries are placed
>   * @indices:	The cache indices corresponding to the entries in @entries
> @@ -1765,9 +1766,9 @@ EXPORT_SYMBOL(pagecache_get_page);
>   *
>   * Return: the number of pages and shadow entries which were found.
>   */
> -unsigned find_get_entries(struct address_space *mapping,
> -			  pgoff_t start, unsigned int nr_entries,
> -			  struct page **entries, pgoff_t *indices)
> +unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> +		pgoff_t end, unsigned int nr_entries, struct page **entries,
> +		pgoff_t *indices)
>  {
>  	XA_STATE(xas, &mapping->i_pages, start);
>  	struct page *page;
> @@ -1777,7 +1778,7 @@ unsigned find_get_entries(struct address_space *mapping,
>  		return 0;
>  
>  	rcu_read_lock();
> -	xas_for_each(&xas, page, ULONG_MAX) {
> +	xas_for_each(&xas, page, end) {
>  		if (xas_retry(&xas, page))
>  			continue;
>  		/*
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0f9f149f4b5e..abdbe61a1aa7 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -906,9 +906,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  	pagevec_init(&pvec);
>  	index = start;
>  	while (index < end) {
> -		pvec.nr = find_get_entries(mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE),
> -			pvec.pages, indices);
> +		pvec.nr = find_get_entries(mapping, index, end - 1,
> +				PAGEVEC_SIZE, pvec.pages, indices);
>  		if (!pvec.nr)
>  			break;
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
> @@ -977,9 +976,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  	while (index < end) {
>  		cond_resched();
>  
> -		pvec.nr = find_get_entries(mapping, index,
> -				min(end - index, (pgoff_t)PAGEVEC_SIZE),
> -				pvec.pages, indices);
> +		pvec.nr = find_get_entries(mapping, index, end - 1,
> +				PAGEVEC_SIZE, pvec.pages, indices);
>  		if (!pvec.nr) {
>  			/* If all gone or hole-punch or unfalloc, we're done */
>  			if (index == start || end != -1)
> diff --git a/mm/swap.c b/mm/swap.c
> index d16d65d9b4e0..fcf6ccb94b09 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1060,7 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
>  				pgoff_t start, unsigned nr_entries,
>  				pgoff_t *indices)
>  {
> -	pvec->nr = find_get_entries(mapping, start, nr_entries,
> +	pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
>  				    pvec->pages, indices);
>  	return pagevec_count(pvec);
>  }
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/7] mm: Add an 'end' parameter to find_get_entries

[Matthew Wilcox]

On Fri, Aug 21, 2020 at 06:07:59PM +0200, Jan Kara wrote:
> On Wed 19-08-20 16:05:51, Matthew Wilcox (Oracle) wrote:
> > This simplifies the callers and leads to a more efficient implementation
> > since the XArray has this functionality already.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> The patch looks good to me. Just I'd note that you could drop some:
> 
> 	if (index >= end)
> 		break;
> 
> checks in shmem_undo_range() as well.

Oh yes, missed a couple ;-)  Thanks, I'll add.

> In the past I was considering moving find_get_entries() to the same API as
> find_get_pages_range() has (which is essentially what you do now, but I
> also had 'start' to be a pgoff_t * so that we can return there where the
> iteration ended in the range). But in the end I've decided the churn is not
> worth the few removed lines and didn't push the patch in the end. What you
> did in this patch seems to be a reasonable middle-ground :)

I did look at that, but since we're returning the indices, we don't _need_
to update the index here.

I have some other ideas for this family of interfaces, but I'm trying
to get the THP work off my plate before getting distracted by that ;-)

Re: [PATCH 3/7] mm: Add an 'end' parameter to find_get_entries

[Jan Kara]

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

On Fri 21-08-20 17:33:06, Matthew Wilcox wrote:
> On Fri, Aug 21, 2020 at 06:07:59PM +0200, Jan Kara wrote:
> > On Wed 19-08-20 16:05:51, Matthew Wilcox (Oracle) wrote:
> > > This simplifies the callers and leads to a more efficient implementation
> > > since the XArray has this functionality already.
> > > 
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > The patch looks good to me. Just I'd note that you could drop some:
> > 
> > 	if (index >= end)
> > 		break;
> > 
> > checks in shmem_undo_range() as well.
> 
> Oh yes, missed a couple ;-)  Thanks, I'll add.
> 
> > In the past I was considering moving find_get_entries() to the same API as
> > find_get_pages_range() has (which is essentially what you do now, but I
> > also had 'start' to be a pgoff_t * so that we can return there where the
> > iteration ended in the range). But in the end I've decided the churn is not
> > worth the few removed lines and didn't push the patch in the end. What you
> > did in this patch seems to be a reasonable middle-ground :)
> 
> I did look at that, but since we're returning the indices, we don't _need_
> to update the index here.
> 
> I have some other ideas for this family of interfaces, but I'm trying
> to get the THP work off my plate before getting distracted by that ;-)

I have one thing which I wanted to do for a long time but never got to it.
IMHO the pagevec abstraction makes the loops unnecessarily complex. I'd
rather have helpers like:

for_each_mapping_page(mapping, page, start, end)

or

for_each_mapping_entry(mapping, entry, index, start, end)

and hide all the pagevec magic inside those. And it's even not that hard to
do including the handling of premature exit from the loop - sample
userspace code is attached...

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: pvec_looping.c --]
[-- Type: text/x-c, Size: 1401 bytes --]

#include <stdio.h>

#define PAGEVEC_SIZE 15

struct pagevec {
	unsigned char nr;
	int vals[PAGEVEC_SIZE];
};

struct pagevec_iter {
	unsigned char idx;
	int last_index;
	struct pagevec pvec;
};

void pagevec_release(struct pagevec *pvec)
{
	int i;

	for (i = 0; i < pvec->nr; i++)
		printf("Freeing val %d\n", pvec->vals[i]);
	pvec->nr = 0;
}

int pagevec_lookup(struct pagevec *pvec, void *mapping, int *start, int end)
{
	int i;

	for (i = 0; i < PAGEVEC_SIZE && *start < 29; i++) {
		pvec->vals[pvec->nr++] = 100 + *start;
		(*start)++;
	}
	return pvec->nr;
}

static inline int get_next_pvec_val(struct pagevec_iter *pvec_i, void *mapping,
				    int end)
{
	if (pvec_i->idx >= pvec_i->pvec.nr) {
		pagevec_release(&pvec_i->pvec);
		if (!pagevec_lookup(&pvec_i->pvec, mapping, &pvec_i->last_index,
				    end))
			return 0;
		pvec_i->idx = 0;
	}
	return pvec_i->pvec.vals[pvec_i->idx++];
}

void pagevec_release_iter(struct pagevec_iter *pvec_i)
{
	pagevec_release(&pvec_i->pvec);
}

#define for_each_page(mapping, page, start, end)			\
	for (struct pagevec_iter pvec_i					\
		__attribute__ ((cleanup (pagevec_release_iter))) = {	\
			.idx = 0, .last_index = start, .pvec = { .nr = 0 } }; \
	     (page = get_next_pvec_val(&pvec_i, mapping, end));)

int main(void)
{
	int page;

	for_each_page(NULL, page, 5, 32) {
		printf("Seeing val %d\n", page);
		if (page > 122)
			break;
	}
	return 0;
}

[PATCH 4/7] mm: Add an 'end' parameter to pagevec_lookup_entries

[Matthew Wilcox (Oracle)]

Simplifies the callers and uses the existing functionality of
find_get_entries().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagevec.h |  5 ++---
 mm/swap.c               |  8 ++++----
 mm/truncate.c           | 36 ++++++++----------------------------
 3 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
index 081d934eda64..4b245592262c 100644
--- a/include/linux/pagevec.h
+++ b/include/linux/pagevec.h
@@ -26,9 +26,8 @@ struct pagevec {
 void __pagevec_release(struct pagevec *pvec);
 void __pagevec_lru_add(struct pagevec *pvec);
 unsigned pagevec_lookup_entries(struct pagevec *pvec,
-				struct address_space *mapping,
-				pgoff_t start, unsigned nr_entries,
-				pgoff_t *indices);
+		struct address_space *mapping, pgoff_t start, pgoff_t end,
+		unsigned nr_entries, pgoff_t *indices);
 void pagevec_remove_exceptionals(struct pagevec *pvec);
 unsigned pagevec_lookup_range(struct pagevec *pvec,
 			      struct address_space *mapping,
diff --git a/mm/swap.c b/mm/swap.c
index fcf6ccb94b09..b6e56a84b466 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1036,6 +1036,7 @@ void __pagevec_lru_add(struct pagevec *pvec)
  * @pvec:	Where the resulting entries are placed
  * @mapping:	The address_space to search
  * @start:	The starting entry index
+ * @end:	The highest index to return (inclusive).
  * @nr_entries:	The maximum number of pages
  * @indices:	The cache indices corresponding to the entries in @pvec
  *
@@ -1056,11 +1057,10 @@ void __pagevec_lru_add(struct pagevec *pvec)
  * found.
  */
 unsigned pagevec_lookup_entries(struct pagevec *pvec,
-				struct address_space *mapping,
-				pgoff_t start, unsigned nr_entries,
-				pgoff_t *indices)
+		struct address_space *mapping, pgoff_t start, pgoff_t end,
+		unsigned nr_entries, pgoff_t *indices)
 {
-	pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
+	pvec->nr = find_get_entries(mapping, start, end, nr_entries,
 				    pvec->pages, indices);
 	return pagevec_count(pvec);
 }
diff --git a/mm/truncate.c b/mm/truncate.c
index dd9ebc1da356..9c617291fb1e 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -326,9 +326,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (index < end && pagevec_lookup_entries(&pvec, mapping, index,
-			min(end - index, (pgoff_t)PAGEVEC_SIZE),
-			indices)) {
+	while (pagevec_lookup_entries(&pvec, mapping, index, end - 1,
+			PAGEVEC_SIZE, indices)) {
 		/*
 		 * Pagevec array has exceptional entries and we may also fail
 		 * to lock some pages. So we store pages that can be deleted
@@ -342,8 +341,6 @@ void truncate_inode_pages_range(struct address_space *mapping,
 
 			/* We rely upon deletion not changing page->index */
 			index = indices[i];
-			if (index >= end)
-				break;
 
 			if (xa_is_value(page))
 				continue;
@@ -413,8 +410,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	index = start;
 	for ( ; ; ) {
 		cond_resched();
-		if (!pagevec_lookup_entries(&pvec, mapping, index,
-			min(end - index, (pgoff_t)PAGEVEC_SIZE), indices)) {
+		if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
+				PAGEVEC_SIZE, indices)) {
 			/* If all gone from start onwards, we're done */
 			if (index == start)
 				break;
@@ -422,23 +419,12 @@ void truncate_inode_pages_range(struct address_space *mapping,
 			index = start;
 			continue;
 		}
-		if (index == start && indices[0] >= end) {
-			/* All gone out of hole to be punched, we're done */
-			pagevec_remove_exceptionals(&pvec);
-			pagevec_release(&pvec);
-			break;
-		}
 
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
 			/* We rely upon deletion not changing page->index */
 			index = indices[i];
-			if (index >= end) {
-				/* Restart punch to make sure all gone */
-				index = start - 1;
-				break;
-			}
 
 			if (xa_is_value(page))
 				continue;
@@ -554,16 +540,13 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 	int i;
 
 	pagevec_init(&pvec);
-	while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
-			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
-			indices)) {
+	while (pagevec_lookup_entries(&pvec, mapping, index, end,
+			PAGEVEC_SIZE, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
 			/* We rely upon deletion not changing page->index */
 			index = indices[i];
-			if (index > end)
-				break;
 
 			if (xa_is_value(page)) {
 				invalidate_exceptional_entry(mapping, index,
@@ -697,16 +680,13 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
-			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
-			indices)) {
+	while (pagevec_lookup_entries(&pvec, mapping, index, end,
+			PAGEVEC_SIZE, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
 			/* We rely upon deletion not changing page->index */
 			index = indices[i];
-			if (index > end)
-				break;
 
 			if (xa_is_value(page)) {
 				if (!invalidate_exceptional_entry2(mapping,
-- 
2.28.0

Re: [PATCH 4/7] mm: Add an 'end' parameter to pagevec_lookup_entries

[Jan Kara]

On Wed 19-08-20 16:05:52, Matthew Wilcox (Oracle) wrote:
> Simplifies the callers and uses the existing functionality of
> find_get_entries().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/pagevec.h |  5 ++---
>  mm/swap.c               |  8 ++++----
>  mm/truncate.c           | 36 ++++++++----------------------------
>  3 files changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
> index 081d934eda64..4b245592262c 100644
> --- a/include/linux/pagevec.h
> +++ b/include/linux/pagevec.h
> @@ -26,9 +26,8 @@ struct pagevec {
>  void __pagevec_release(struct pagevec *pvec);
>  void __pagevec_lru_add(struct pagevec *pvec);
>  unsigned pagevec_lookup_entries(struct pagevec *pvec,
> -				struct address_space *mapping,
> -				pgoff_t start, unsigned nr_entries,
> -				pgoff_t *indices);
> +		struct address_space *mapping, pgoff_t start, pgoff_t end,
> +		unsigned nr_entries, pgoff_t *indices);
>  void pagevec_remove_exceptionals(struct pagevec *pvec);
>  unsigned pagevec_lookup_range(struct pagevec *pvec,
>  			      struct address_space *mapping,
> diff --git a/mm/swap.c b/mm/swap.c
> index fcf6ccb94b09..b6e56a84b466 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1036,6 +1036,7 @@ void __pagevec_lru_add(struct pagevec *pvec)
>   * @pvec:	Where the resulting entries are placed
>   * @mapping:	The address_space to search
>   * @start:	The starting entry index
> + * @end:	The highest index to return (inclusive).
>   * @nr_entries:	The maximum number of pages
>   * @indices:	The cache indices corresponding to the entries in @pvec
>   *
> @@ -1056,11 +1057,10 @@ void __pagevec_lru_add(struct pagevec *pvec)
>   * found.
>   */
>  unsigned pagevec_lookup_entries(struct pagevec *pvec,
> -				struct address_space *mapping,
> -				pgoff_t start, unsigned nr_entries,
> -				pgoff_t *indices)
> +		struct address_space *mapping, pgoff_t start, pgoff_t end,
> +		unsigned nr_entries, pgoff_t *indices)
>  {
> -	pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
> +	pvec->nr = find_get_entries(mapping, start, end, nr_entries,
>  				    pvec->pages, indices);
>  	return pagevec_count(pvec);
>  }
> diff --git a/mm/truncate.c b/mm/truncate.c
> index dd9ebc1da356..9c617291fb1e 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -326,9 +326,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  
>  	pagevec_init(&pvec);
>  	index = start;
> -	while (index < end && pagevec_lookup_entries(&pvec, mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE),
> -			indices)) {
> +	while (pagevec_lookup_entries(&pvec, mapping, index, end - 1,
> +			PAGEVEC_SIZE, indices)) {
>  		/*
>  		 * Pagevec array has exceptional entries and we may also fail
>  		 * to lock some pages. So we store pages that can be deleted
> @@ -342,8 +341,6 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  
>  			/* We rely upon deletion not changing page->index */
>  			index = indices[i];
> -			if (index >= end)
> -				break;
>  
>  			if (xa_is_value(page))
>  				continue;
> @@ -413,8 +410,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  	index = start;
>  	for ( ; ; ) {
>  		cond_resched();
> -		if (!pagevec_lookup_entries(&pvec, mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE), indices)) {
> +		if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
> +				PAGEVEC_SIZE, indices)) {
>  			/* If all gone from start onwards, we're done */
>  			if (index == start)
>  				break;
> @@ -422,23 +419,12 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  			index = start;
>  			continue;
>  		}
> -		if (index == start && indices[0] >= end) {
> -			/* All gone out of hole to be punched, we're done */
> -			pagevec_remove_exceptionals(&pvec);
> -			pagevec_release(&pvec);
> -			break;
> -		}
>  
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
>  			/* We rely upon deletion not changing page->index */
>  			index = indices[i];
> -			if (index >= end) {
> -				/* Restart punch to make sure all gone */
> -				index = start - 1;
> -				break;
> -			}
>  
>  			if (xa_is_value(page))
>  				continue;
> @@ -554,16 +540,13 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
>  	int i;
>  
>  	pagevec_init(&pvec);
> -	while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
> -			indices)) {
> +	while (pagevec_lookup_entries(&pvec, mapping, index, end,
> +			PAGEVEC_SIZE, indices)) {
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
>  			/* We rely upon deletion not changing page->index */
>  			index = indices[i];
> -			if (index > end)
> -				break;
>  
>  			if (xa_is_value(page)) {
>  				invalidate_exceptional_entry(mapping, index,
> @@ -697,16 +680,13 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
>  
>  	pagevec_init(&pvec);
>  	index = start;
> -	while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
> -			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
> -			indices)) {
> +	while (pagevec_lookup_entries(&pvec, mapping, index, end,
> +			PAGEVEC_SIZE, indices)) {
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
>  			/* We rely upon deletion not changing page->index */
>  			index = indices[i];
> -			if (index > end)
> -				break;
>  
>  			if (xa_is_value(page)) {
>  				if (!invalidate_exceptional_entry2(mapping,
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 5/7] mm: Remove nr_entries parameter from pagevec_lookup_entries

[Matthew Wilcox (Oracle)]

All callers want to fetch the full size of the pvec.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagevec.h |  2 +-
 mm/swap.c               |  4 ++--
 mm/truncate.c           | 10 ++++------
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
index 4b245592262c..ce77724a2ab7 100644
--- a/include/linux/pagevec.h
+++ b/include/linux/pagevec.h
@@ -27,7 +27,7 @@ void __pagevec_release(struct pagevec *pvec);
 void __pagevec_lru_add(struct pagevec *pvec);
 unsigned pagevec_lookup_entries(struct pagevec *pvec,
 		struct address_space *mapping, pgoff_t start, pgoff_t end,
-		unsigned nr_entries, pgoff_t *indices);
+		pgoff_t *indices);
 void pagevec_remove_exceptionals(struct pagevec *pvec);
 unsigned pagevec_lookup_range(struct pagevec *pvec,
 			      struct address_space *mapping,
diff --git a/mm/swap.c b/mm/swap.c
index b6e56a84b466..d4e3ba4c967c 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1058,9 +1058,9 @@ void __pagevec_lru_add(struct pagevec *pvec)
  */
 unsigned pagevec_lookup_entries(struct pagevec *pvec,
 		struct address_space *mapping, pgoff_t start, pgoff_t end,
-		unsigned nr_entries, pgoff_t *indices)
+		pgoff_t *indices)
 {
-	pvec->nr = find_get_entries(mapping, start, end, nr_entries,
+	pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
 				    pvec->pages, indices);
 	return pagevec_count(pvec);
 }
diff --git a/mm/truncate.c b/mm/truncate.c
index 9c617291fb1e..96a45ba28042 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -327,7 +327,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	pagevec_init(&pvec);
 	index = start;
 	while (pagevec_lookup_entries(&pvec, mapping, index, end - 1,
-			PAGEVEC_SIZE, indices)) {
+			indices)) {
 		/*
 		 * Pagevec array has exceptional entries and we may also fail
 		 * to lock some pages. So we store pages that can be deleted
@@ -411,7 +411,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	for ( ; ; ) {
 		cond_resched();
 		if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
-				PAGEVEC_SIZE, indices)) {
+				indices)) {
 			/* If all gone from start onwards, we're done */
 			if (index == start)
 				break;
@@ -540,8 +540,7 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 	int i;
 
 	pagevec_init(&pvec);
-	while (pagevec_lookup_entries(&pvec, mapping, index, end,
-			PAGEVEC_SIZE, indices)) {
+	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
@@ -680,8 +679,7 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (pagevec_lookup_entries(&pvec, mapping, index, end,
-			PAGEVEC_SIZE, indices)) {
+	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
-- 
2.28.0

Re: [PATCH 5/7] mm: Remove nr_entries parameter from pagevec_lookup_entries

[Jan Kara]

On Wed 19-08-20 16:05:53, Matthew Wilcox (Oracle) wrote:
> All callers want to fetch the full size of the pvec.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/pagevec.h |  2 +-
>  mm/swap.c               |  4 ++--
>  mm/truncate.c           | 10 ++++------
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
> index 4b245592262c..ce77724a2ab7 100644
> --- a/include/linux/pagevec.h
> +++ b/include/linux/pagevec.h
> @@ -27,7 +27,7 @@ void __pagevec_release(struct pagevec *pvec);
>  void __pagevec_lru_add(struct pagevec *pvec);
>  unsigned pagevec_lookup_entries(struct pagevec *pvec,
>  		struct address_space *mapping, pgoff_t start, pgoff_t end,
> -		unsigned nr_entries, pgoff_t *indices);
> +		pgoff_t *indices);
>  void pagevec_remove_exceptionals(struct pagevec *pvec);
>  unsigned pagevec_lookup_range(struct pagevec *pvec,
>  			      struct address_space *mapping,
> diff --git a/mm/swap.c b/mm/swap.c
> index b6e56a84b466..d4e3ba4c967c 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1058,9 +1058,9 @@ void __pagevec_lru_add(struct pagevec *pvec)
>   */
>  unsigned pagevec_lookup_entries(struct pagevec *pvec,
>  		struct address_space *mapping, pgoff_t start, pgoff_t end,
> -		unsigned nr_entries, pgoff_t *indices)
> +		pgoff_t *indices)
>  {
> -	pvec->nr = find_get_entries(mapping, start, end, nr_entries,
> +	pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
>  				    pvec->pages, indices);
>  	return pagevec_count(pvec);
>  }
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 9c617291fb1e..96a45ba28042 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -327,7 +327,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  	pagevec_init(&pvec);
>  	index = start;
>  	while (pagevec_lookup_entries(&pvec, mapping, index, end - 1,
> -			PAGEVEC_SIZE, indices)) {
> +			indices)) {
>  		/*
>  		 * Pagevec array has exceptional entries and we may also fail
>  		 * to lock some pages. So we store pages that can be deleted
> @@ -411,7 +411,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  	for ( ; ; ) {
>  		cond_resched();
>  		if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
> -				PAGEVEC_SIZE, indices)) {
> +				indices)) {
>  			/* If all gone from start onwards, we're done */
>  			if (index == start)
>  				break;
> @@ -540,8 +540,7 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
>  	int i;
>  
>  	pagevec_init(&pvec);
> -	while (pagevec_lookup_entries(&pvec, mapping, index, end,
> -			PAGEVEC_SIZE, indices)) {
> +	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
> @@ -680,8 +679,7 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
>  
>  	pagevec_init(&pvec);
>  	index = start;
> -	while (pagevec_lookup_entries(&pvec, mapping, index, end,
> -			PAGEVEC_SIZE, indices)) {
> +	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Matthew Wilcox (Oracle)]

All callers of find_get_entries() use a pvec, so pass it directly
instead of manipulating it in the caller.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagemap.h |  3 +--
 mm/filemap.c            | 14 ++++++--------
 mm/shmem.c              | 11 +++--------
 mm/swap.c               |  4 +---
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 3f0dc8d00f2a..9d465dd8b379 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -387,8 +387,7 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
 struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
 struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
 unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
-		pgoff_t end, unsigned int nr_entries, struct page **entries,
-		pgoff_t *indices);
+		pgoff_t end, struct pagevec *pvec, pgoff_t *indices);
 unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
 			pgoff_t end, unsigned int nr_pages,
 			struct page **pages);
diff --git a/mm/filemap.c b/mm/filemap.c
index 159cf3d6f1ae..892c7beef392 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1743,8 +1743,7 @@ EXPORT_SYMBOL(pagecache_get_page);
  * @mapping:	The address_space to search
  * @start:	The starting page cache index
  * @end:	The highest page cache index to return.
- * @nr_entries:	The maximum number of entries
- * @entries:	Where the resulting entries are placed
+ * @pvec:	Where the resulting entries are placed
  * @indices:	The cache indices corresponding to the entries in @entries
  *
  * find_get_entries() will search for and return a group of up to
@@ -1767,15 +1766,12 @@ EXPORT_SYMBOL(pagecache_get_page);
  * Return: the number of pages and shadow entries which were found.
  */
 unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
-		pgoff_t end, unsigned int nr_entries, struct page **entries,
-		pgoff_t *indices)
+		pgoff_t end, struct pagevec *pvec, pgoff_t *indices)
 {
 	XA_STATE(xas, &mapping->i_pages, start);
 	struct page *page;
 	unsigned int ret = 0;
-
-	if (!nr_entries)
-		return 0;
+	unsigned nr_entries = PAGEVEC_SIZE;
 
 	rcu_read_lock();
 	xas_for_each(&xas, page, end) {
@@ -1806,7 +1802,7 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
 		}
 export:
 		indices[ret] = xas.xa_index;
-		entries[ret] = page;
+		pvec->pages[ret] = page;
 		if (++ret == nr_entries)
 			break;
 		continue;
@@ -1816,6 +1812,8 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
 		xas_reset(&xas);
 	}
 	rcu_read_unlock();
+
+	pvec->nr = ret;
 	return ret;
 }
 
diff --git a/mm/shmem.c b/mm/shmem.c
index abdbe61a1aa7..e73c0b2ba99c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -905,11 +905,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (index < end) {
-		pvec.nr = find_get_entries(mapping, index, end - 1,
-				PAGEVEC_SIZE, pvec.pages, indices);
-		if (!pvec.nr)
-			break;
+	while (find_get_entries(mapping, index, end - 1, &pvec, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
@@ -976,9 +972,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 	while (index < end) {
 		cond_resched();
 
-		pvec.nr = find_get_entries(mapping, index, end - 1,
-				PAGEVEC_SIZE, pvec.pages, indices);
-		if (!pvec.nr) {
+		if (!find_get_entries(mapping, index, end - 1, &pvec,
+				indices)) {
 			/* If all gone or hole-punch or unfalloc, we're done */
 			if (index == start || end != -1)
 				break;
diff --git a/mm/swap.c b/mm/swap.c
index d4e3ba4c967c..40b23300d353 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1060,9 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
 		struct address_space *mapping, pgoff_t start, pgoff_t end,
 		pgoff_t *indices)
 {
-	pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
-				    pvec->pages, indices);
-	return pagevec_count(pvec);
+	return find_get_entries(mapping, start, end, pvec, indices);
 }
 
 /**
-- 
2.28.0

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Jan Kara]

On Wed 19-08-20 16:05:54, Matthew Wilcox (Oracle) wrote:
> All callers of find_get_entries() use a pvec, so pass it directly
> instead of manipulating it in the caller.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Rather than passing pvec to find_get_entries() and then making everybody
use it, won't it more consistent WRT the naming to make everybody use
pagevec_lookup_entries() (which is trivial at this point in the series) and
then rename find_get_entries() to pagevec_lookup_entries()? I.e., I'd prefer
if the final function was called pagevec_lookup_entries() because that is
IMO more consistent with how other functions are named in this area...

								Honza

> ---
>  include/linux/pagemap.h |  3 +--
>  mm/filemap.c            | 14 ++++++--------
>  mm/shmem.c              | 11 +++--------
>  mm/swap.c               |  4 +---
>  4 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 3f0dc8d00f2a..9d465dd8b379 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -387,8 +387,7 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
>  struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
>  struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset);
>  unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> -		pgoff_t end, unsigned int nr_entries, struct page **entries,
> -		pgoff_t *indices);
> +		pgoff_t end, struct pagevec *pvec, pgoff_t *indices);
>  unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
>  			pgoff_t end, unsigned int nr_pages,
>  			struct page **pages);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 159cf3d6f1ae..892c7beef392 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1743,8 +1743,7 @@ EXPORT_SYMBOL(pagecache_get_page);
>   * @mapping:	The address_space to search
>   * @start:	The starting page cache index
>   * @end:	The highest page cache index to return.
> - * @nr_entries:	The maximum number of entries
> - * @entries:	Where the resulting entries are placed
> + * @pvec:	Where the resulting entries are placed
>   * @indices:	The cache indices corresponding to the entries in @entries
>   *
>   * find_get_entries() will search for and return a group of up to
> @@ -1767,15 +1766,12 @@ EXPORT_SYMBOL(pagecache_get_page);
>   * Return: the number of pages and shadow entries which were found.
>   */
>  unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> -		pgoff_t end, unsigned int nr_entries, struct page **entries,
> -		pgoff_t *indices)
> +		pgoff_t end, struct pagevec *pvec, pgoff_t *indices)
>  {
>  	XA_STATE(xas, &mapping->i_pages, start);
>  	struct page *page;
>  	unsigned int ret = 0;
> -
> -	if (!nr_entries)
> -		return 0;
> +	unsigned nr_entries = PAGEVEC_SIZE;
>  
>  	rcu_read_lock();
>  	xas_for_each(&xas, page, end) {
> @@ -1806,7 +1802,7 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
>  		}
>  export:
>  		indices[ret] = xas.xa_index;
> -		entries[ret] = page;
> +		pvec->pages[ret] = page;
>  		if (++ret == nr_entries)
>  			break;
>  		continue;
> @@ -1816,6 +1812,8 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
>  		xas_reset(&xas);
>  	}
>  	rcu_read_unlock();
> +
> +	pvec->nr = ret;
>  	return ret;
>  }
>  
> diff --git a/mm/shmem.c b/mm/shmem.c
> index abdbe61a1aa7..e73c0b2ba99c 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -905,11 +905,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  
>  	pagevec_init(&pvec);
>  	index = start;
> -	while (index < end) {
> -		pvec.nr = find_get_entries(mapping, index, end - 1,
> -				PAGEVEC_SIZE, pvec.pages, indices);
> -		if (!pvec.nr)
> -			break;
> +	while (find_get_entries(mapping, index, end - 1, &pvec, indices)) {
>  		for (i = 0; i < pagevec_count(&pvec); i++) {
>  			struct page *page = pvec.pages[i];
>  
> @@ -976,9 +972,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
>  	while (index < end) {
>  		cond_resched();
>  
> -		pvec.nr = find_get_entries(mapping, index, end - 1,
> -				PAGEVEC_SIZE, pvec.pages, indices);
> -		if (!pvec.nr) {
> +		if (!find_get_entries(mapping, index, end - 1, &pvec,
> +				indices)) {
>  			/* If all gone or hole-punch or unfalloc, we're done */
>  			if (index == start || end != -1)
>  				break;
> diff --git a/mm/swap.c b/mm/swap.c
> index d4e3ba4c967c..40b23300d353 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1060,9 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
>  		struct address_space *mapping, pgoff_t start, pgoff_t end,
>  		pgoff_t *indices)
>  {
> -	pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
> -				    pvec->pages, indices);
> -	return pagevec_count(pvec);
> +	return find_get_entries(mapping, start, end, pvec, indices);
>  }
>  
>  /**
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Matthew Wilcox]

On Mon, Aug 24, 2020 at 06:16:20PM +0200, Jan Kara wrote:
> On Wed 19-08-20 16:05:54, Matthew Wilcox (Oracle) wrote:
> > All callers of find_get_entries() use a pvec, so pass it directly
> > instead of manipulating it in the caller.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Rather than passing pvec to find_get_entries() and then making everybody
> use it, won't it more consistent WRT the naming to make everybody use
> pagevec_lookup_entries() (which is trivial at this point in the series) and
> then rename find_get_entries() to pagevec_lookup_entries()? I.e., I'd prefer
> if the final function was called pagevec_lookup_entries() because that is
> IMO more consistent with how other functions are named in this area...

It seemed more consistent to me to have everybody using
find_get_entries().  To me the pagevec functions:

1. Are in mm/swap.c (not really sure why)
2. Take pvec as the first argument, not the last
3. Wrap a find_* function

Whereas the find_* functions:

1. Are in mm/filemap.c
2. Take mapping as the first argument
3. Manipulate the XArray directly

We already have functions in filemap which take a pagevec, eg
page_cache_delete_batch() and delete_from_page_cache_batch().

So if we're going to merge the two functions, it seems more natural to
have it in filemap.c and called find_get_entries(), but I'm definitely
open to persuasion on this!

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Jan Kara]

On Mon 24-08-20 18:36:39, Matthew Wilcox wrote:
> On Mon, Aug 24, 2020 at 06:16:20PM +0200, Jan Kara wrote:
> > On Wed 19-08-20 16:05:54, Matthew Wilcox (Oracle) wrote:
> > > All callers of find_get_entries() use a pvec, so pass it directly
> > > instead of manipulating it in the caller.
> > > 
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > Rather than passing pvec to find_get_entries() and then making everybody
> > use it, won't it more consistent WRT the naming to make everybody use
> > pagevec_lookup_entries() (which is trivial at this point in the series) and
> > then rename find_get_entries() to pagevec_lookup_entries()? I.e., I'd prefer
> > if the final function was called pagevec_lookup_entries() because that is
> > IMO more consistent with how other functions are named in this area...
> 
> It seemed more consistent to me to have everybody using
> find_get_entries().  To me the pagevec functions:
> 
> 1. Are in mm/swap.c (not really sure why)

Historical :). AFAIK pagevec abstraction was first created to make swapping
out and reclaim of pages more effective. It has grown a bit since then...

> 2. Take pvec as the first argument, not the last

Well, yes, I'd keep the argument order to match original
pagevec_lookup_entries().

> 3. Wrap a find_* function
> 
> Whereas the find_* functions:
> 
> 1. Are in mm/filemap.c
> 2. Take mapping as the first argument
> 3. Manipulate the XArray directly

Agreed.

> We already have functions in filemap which take a pagevec, eg
> page_cache_delete_batch() and delete_from_page_cache_batch().

Right but those are really pretty internal helper functions so I don't
think they form or strong precedence.

> So if we're going to merge the two functions, it seems more natural to
> have it in filemap.c and called find_get_entries(), but I'm definitely
> open to persuasion on this!

I agree that having non-trivial xarray code in mm/swap.c isn't attractive
either. Dunno, I dislike the inconsistency between find_get_pages() and
find_get_entries() you create but they aren't completely consistent anyway
so I can live with that. Or we can just leave the pagevec_lookup_entries()
wrapper and the API will stay consistent...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Matthew Wilcox]

On Tue, Aug 25, 2020 at 02:33:24PM +0200, Jan Kara wrote:
> On Mon 24-08-20 18:36:39, Matthew Wilcox wrote:
> > We already have functions in filemap which take a pagevec, eg
> > page_cache_delete_batch() and delete_from_page_cache_batch().
> 
> Right but those are really pretty internal helper functions so I don't
> think they form or strong precedence.

To be honest, I saw that as being the way forward for the page cache APIs.
If we're going to use a batching mechanism, it should be pagevecs, and
it should be built into the page cache interfaces rather than hanging
out off on the side.

> > So if we're going to merge the two functions, it seems more natural to
> > have it in filemap.c and called find_get_entries(), but I'm definitely
> > open to persuasion on this!
> 
> I agree that having non-trivial xarray code in mm/swap.c isn't attractive
> either. Dunno, I dislike the inconsistency between find_get_pages() and
> find_get_entries() you create but they aren't completely consistent anyway
> so I can live with that. Or we can just leave the pagevec_lookup_entries()
> wrapper and the API will stay consistent...

I was thinking about this some more [1] [2].  I think we can get to the
point where find_get_pages(), find_get_entries() and find_get_pages_tag()
(and all their variants) end up taking a pagevec as their last argument.

Also, I was thinking that all these names are wrong.  Really, they're
mapping_get_pages(), mapping_get_entries() and mapping_get_marked_pages().
So maybe I should move in that direction.

[1] https://lore.kernel.org/lkml/20200824214841.17132-1-willy@infradead.org/
[2] https://lore.kernel.org/lkml/20200824183424.4222-1-willy@infradead.org/

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Jan Kara]

On Tue 25-08-20 14:28:14, Matthew Wilcox wrote:
> On Tue, Aug 25, 2020 at 02:33:24PM +0200, Jan Kara wrote:
> > On Mon 24-08-20 18:36:39, Matthew Wilcox wrote:
> > > We already have functions in filemap which take a pagevec, eg
> > > page_cache_delete_batch() and delete_from_page_cache_batch().
> > 
> > Right but those are really pretty internal helper functions so I don't
> > think they form or strong precedence.
> 
> To be honest, I saw that as being the way forward for the page cache APIs.
> If we're going to use a batching mechanism, it should be pagevecs, and
> it should be built into the page cache interfaces rather than hanging
> out off on the side.
> 
> > > So if we're going to merge the two functions, it seems more natural to
> > > have it in filemap.c and called find_get_entries(), but I'm definitely
> > > open to persuasion on this!
> > 
> > I agree that having non-trivial xarray code in mm/swap.c isn't attractive
> > either. Dunno, I dislike the inconsistency between find_get_pages() and
> > find_get_entries() you create but they aren't completely consistent anyway
> > so I can live with that. Or we can just leave the pagevec_lookup_entries()
> > wrapper and the API will stay consistent...
> 
> I was thinking about this some more [1] [2].  I think we can get to the
> point where find_get_pages(), find_get_entries() and find_get_pages_tag()
> (and all their variants) end up taking a pagevec as their last argument.
> 
> Also, I was thinking that all these names are wrong.  Really, they're
> mapping_get_pages(), mapping_get_entries() and mapping_get_marked_pages().
> So maybe I should move in that direction.

Well, as I wrote to you in one of the replies. IMO pagevec unnecessarily
complicate matters and we should rather have for_each_mapping_page() and
for_each_mapping_entry() magic macros that hide pagevecs inside. Most of
users process returned pages/entries one by one so these macros would
simplify them. So it would seem better to me to go more into this direction
than to spread pagevecs...

> [1] https://lore.kernel.org/lkml/20200824214841.17132-1-willy@infradead.org/
> [2] https://lore.kernel.org/lkml/20200824183424.4222-1-willy@infradead.org/

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries

[Johannes Weiner]

On Tue, Aug 25, 2020 at 02:28:14PM +0100, Matthew Wilcox wrote:
> On Tue, Aug 25, 2020 at 02:33:24PM +0200, Jan Kara wrote:
> > On Mon 24-08-20 18:36:39, Matthew Wilcox wrote:
> > > We already have functions in filemap which take a pagevec, eg
> > > page_cache_delete_batch() and delete_from_page_cache_batch().
> > 
> > Right but those are really pretty internal helper functions so I don't
> > think they form or strong precedence.
> 
> To be honest, I saw that as being the way forward for the page cache APIs.
> If we're going to use a batching mechanism, it should be pagevecs, and
> it should be built into the page cache interfaces rather than hanging
> out off on the side.

Agreed.

> > > So if we're going to merge the two functions, it seems more natural to
> > > have it in filemap.c and called find_get_entries(), but I'm definitely
> > > open to persuasion on this!
> > 
> > I agree that having non-trivial xarray code in mm/swap.c isn't attractive
> > either. Dunno, I dislike the inconsistency between find_get_pages() and
> > find_get_entries() you create but they aren't completely consistent anyway
> > so I can live with that. Or we can just leave the pagevec_lookup_entries()
> > wrapper and the API will stay consistent...
> 
> I was thinking about this some more [1] [2].  I think we can get to the
> point where find_get_pages(), find_get_entries() and find_get_pages_tag()
> (and all their variants) end up taking a pagevec as their last argument.

Agreed.

> Also, I was thinking that all these names are wrong.  Really, they're
> mapping_get_pages(), mapping_get_entries() and mapping_get_marked_pages().
> So maybe I should move in that direction.

That sounds like a lateral move in naming to me. The mapping prefix is
a slight improvement, but without the "find" it sounds like a refcount
operation and hides the fact that this is doing some sort of lookup
and has higher complexity.

It also makes working on this code easier on people not yet familiar
with it at the cost of people familiar with it. Remembering new names
for known concepts is a ton of mental churn.

So IMO the new names should be unambigously and significantly better
than the old ones to justify this.

Signed: somebody who is still struggling with the change from
exceptional entries in the radix tree to value entries in the xarray
(Are pointers or integers the values? Aren't they both "values"?)

[PATCH 7/7] mm: Remove pagevec_lookup_entries

[Matthew Wilcox (Oracle)]

pagevec_lookup_entries() is now just a wrapper around find_get_entries()
so remove it and convert all its callers.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/pagevec.h |  3 ---
 mm/swap.c               | 36 ++----------------------------------
 mm/truncate.c           |  9 ++++-----
 3 files changed, 6 insertions(+), 42 deletions(-)

diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
index ce77724a2ab7..a45bea4b4d08 100644
--- a/include/linux/pagevec.h
+++ b/include/linux/pagevec.h
@@ -25,9 +25,6 @@ struct pagevec {
 
 void __pagevec_release(struct pagevec *pvec);
 void __pagevec_lru_add(struct pagevec *pvec);
-unsigned pagevec_lookup_entries(struct pagevec *pvec,
-		struct address_space *mapping, pgoff_t start, pgoff_t end,
-		pgoff_t *indices);
 void pagevec_remove_exceptionals(struct pagevec *pvec);
 unsigned pagevec_lookup_range(struct pagevec *pvec,
 			      struct address_space *mapping,
diff --git a/mm/swap.c b/mm/swap.c
index 40b23300d353..cc0d648f79d4 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1031,44 +1031,12 @@ void __pagevec_lru_add(struct pagevec *pvec)
 	pagevec_lru_move_fn(pvec, __pagevec_lru_add_fn, NULL);
 }
 
-/**
- * pagevec_lookup_entries - gang pagecache lookup
- * @pvec:	Where the resulting entries are placed
- * @mapping:	The address_space to search
- * @start:	The starting entry index
- * @end:	The highest index to return (inclusive).
- * @nr_entries:	The maximum number of pages
- * @indices:	The cache indices corresponding to the entries in @pvec
- *
- * pagevec_lookup_entries() will search for and return a group of up
- * to @nr_pages pages and shadow entries in the mapping.  All
- * entries are placed in @pvec.  pagevec_lookup_entries() takes a
- * reference against actual pages in @pvec.
- *
- * The search returns a group of mapping-contiguous entries with
- * ascending indexes.  There may be holes in the indices due to
- * not-present entries.
- *
- * Only one subpage of a Transparent Huge Page is returned in one call:
- * allowing truncate_inode_pages_range() to evict the whole THP without
- * cycling through a pagevec of extra references.
- *
- * pagevec_lookup_entries() returns the number of entries which were
- * found.
- */
-unsigned pagevec_lookup_entries(struct pagevec *pvec,
-		struct address_space *mapping, pgoff_t start, pgoff_t end,
-		pgoff_t *indices)
-{
-	return find_get_entries(mapping, start, end, pvec, indices);
-}
-
 /**
  * pagevec_remove_exceptionals - pagevec exceptionals pruning
  * @pvec:	The pagevec to prune
  *
- * pagevec_lookup_entries() fills both pages and exceptional radix
- * tree entries into the pagevec.  This function prunes all
+ * find_get_entries() fills both pages and XArray value entries (aka
+ * exceptional entries) into the pagevec.  This function prunes all
  * exceptionals from @pvec without leaving holes, so that it can be
  * passed on to page-only pagevec operations.
  */
diff --git a/mm/truncate.c b/mm/truncate.c
index 96a45ba28042..90b07884db9f 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -326,8 +326,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (pagevec_lookup_entries(&pvec, mapping, index, end - 1,
-			indices)) {
+	while (find_get_entries(mapping, index, end - 1, &pvec, indices)) {
 		/*
 		 * Pagevec array has exceptional entries and we may also fail
 		 * to lock some pages. So we store pages that can be deleted
@@ -410,7 +409,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	index = start;
 	for ( ; ; ) {
 		cond_resched();
-		if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
+		if (!find_get_entries(mapping, index, end - 1, &pvec,
 				indices)) {
 			/* If all gone from start onwards, we're done */
 			if (index == start)
@@ -540,7 +539,7 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 	int i;
 
 	pagevec_init(&pvec);
-	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
+	while (find_get_entries(mapping, index, end, &pvec, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
@@ -679,7 +678,7 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
 
 	pagevec_init(&pvec);
 	index = start;
-	while (pagevec_lookup_entries(&pvec, mapping, index, end, indices)) {
+	while (find_get_entries(mapping, index, end, &pvec, indices)) {
 		for (i = 0; i < pagevec_count(&pvec); i++) {
 			struct page *page = pvec.pages[i];
 
-- 
2.28.0

Re: [PATCH 0/7] Overhaul find_get_entries and pagevec_lookup_entries

[William Kucharski]

> On Aug 19, 2020, at 9:05 AM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> This started out as part of the THP patchset, but it's turned into a
> nice simplification in its own right.  Essentially we end up unifying
> find_get_entries() and pagevec_lookup_entries() into one function that's
> better than either, and we get rid of a lot of code in the callers as
> a result.
> 
> I'm running this through xfstests right now, but something similar to
> this has already passed xfstests as part of the THP patchset.
> 
> I've done my best to avoid off-by-one errors for 'end', but I wouldn't be
> surprised if I made a mistake.  We're not consistent with whether 'end'
> is inclusive or exclusive and I didn't want to make extensive changes
> to ensure they were consistent.
> 
> Matthew Wilcox (Oracle) (7):
>  mm: Use pagevec_lookup in shmem_unlock_mapping
>  mm: Rewrite shmem_seek_hole_data
>  mm: Add an 'end' parameter to find_get_entries
>  mm: Add an 'end' parameter to pagevec_lookup_entries
>  mm: Remove nr_entries parameter from pagevec_lookup_entries
>  mm: Pass pvec directly to find_get_entries
>  mm: Remove pagevec_lookup_entries
> 
> include/linux/pagemap.h |  3 +-
> include/linux/pagevec.h |  4 --
> mm/filemap.c            | 19 +++++----
> mm/shmem.c              | 85 ++++++++++++++---------------------------
> mm/swap.c               | 38 +-----------------
> mm/truncate.c           | 33 +++-------------
> 6 files changed, 45 insertions(+), 137 deletions(-)

Very nice cleanups and the code makes more sense, thanks.

Reviewed-by: William Kucharski <william.kucharski@oracle.com>