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) <[email protected]>
---
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
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) <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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) <[email protected]>
>
> 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!
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) <[email protected]>
> >
> > 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 <[email protected]>
SUSE Labs, CR
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/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
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/[email protected]/
> [2] https://lore.kernel.org/lkml/[email protected]/
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
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"?)