Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Aug 2020 12:23:42 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Jan Kara <jack@suse.cz>, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        William Kucharski <william.kucharski@oracle.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries
Message-ID: <20200825162342.GC932571@cmpxchg.org>
References: <20200819150555.31669-1-willy@infradead.org>
 <20200819150555.31669-7-willy@infradead.org>
 <20200824161620.GK24877@quack2.suse.cz>
 <20200824173639.GD17456@casper.infradead.org>
 <20200825123324.GB32298@quack2.suse.cz>
 <20200825132814.GO17456@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200825132814.GO17456@casper.infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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"?)