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:   Sat, 15 May 2021 16:51:38 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Christoph Hellwig <hch@lst.de>,
        Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v10 12/33] mm/filemap: Add folio_index, folio_file_page
 and folio_contains
Message-ID: <YJ/uCv8iwWYYXqVQ@casper.infradead.org>
References: <20210511214735.1836149-1-willy@infradead.org>
 <20210511214735.1836149-13-willy@infradead.org>
 <77357d4f-5f56-6c12-7602-697773c2f125@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <77357d4f-5f56-6c12-7602-697773c2f125@suse.cz>
Precedence: bulk

On Fri, May 14, 2021 at 05:55:46PM +0200, Vlastimil Babka wrote:
> On 5/11/21 11:47 PM, Matthew Wilcox (Oracle) wrote:
> > folio_index() is the equivalent of page_index() for folios.
> > folio_file_page() is the equivalent of find_subpage().
> 
> find_subpage() special cases hugetlbfs, folio_file_page() doesn't.
> 
> > folio_contains() is the equivalent of thp_contains().
> 
> Yet here, both thp_contains() and folio_contains() does.
> 
> This patch doesn't add users so maybe it becomes obvious later, but perhaps
> worth explaining in the changelog or comment?

No, you're right, this is a bug.

I originally had it in my mind that hugetlbfs wouldn't need to do this
any more because it can just use the folio interfaces and never try to
find the subpage.

But I don't understand all the cases well enough to be sure that
they're all gone, and they certainly don't all go as part of this
patch series.  So I think I need to reintroduce the check-for-hugetlb
to folio_file_page() and we can look at removing it later once we're
sure that nobody is using the interfaces that return pages from the page
cache any more.  Or we convert hugetlbfs to use the page cache the same
way as every other filesystem ;-)

Thanks for spotting that.