Messages in this thread | | | Date | Tue, 25 Aug 2020 14:33:24 +0200 | From | Jan Kara <> | Subject | Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries |
| |
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
| |