Re: [PATCH] mm: Introduce page_size()

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jan 01, 2019 at 03:41:00PM +0530, Aneesh Kumar K.V wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> > On Tue, Jan 01, 2019 at 08:57:53AM +0530, Aneesh Kumar K.V wrote:
> >> Matthew Wilcox <willy@infradead.org> writes:
> >> > +/* Returns the number of bytes in this potentially compound page. */
> >> > +static inline unsigned long page_size(struct page *page)
> >> > +{
> >> > +	return (unsigned long)PAGE_SIZE << compound_order(page);
> >> > +}
> >> > +
> >> 
> >> How about compound_page_size() to make it clear this is for
> >> compound_pages? Should we make it work with Tail pages by doing
> >> compound_head(page)?
> >
> > I think that's a terrible idea.  Actually, I think the whole way we handle
> > compound pages is terrible; we should only ever see head pages.  Doing
> > page cache lookups should only give us head pages.  Calling pfn_to_page()
> > should give us the head page.  We should only put head pages into SG lists.
> > Everywhere you see a struct page should only be a head page.
> >
> > I know we're far from that today, and there's lots of work to be done
> > to get there.  But the current state of handling compound pages is awful
> > and confusing.
> >
> > Also, page_size() isn't just for compound pages.  It works for regular
> > pages too.  I'd be open to putting a VM_BUG_ON(PageTail(page)) in it
> > to catch people who misuse it.
> 
> Adding VM_BUG_ON is a good idea.

I'm no longer sure about that.  If someone has a tail page and asks for
page_size(page), I think they want to get PAGE_SIZE back.  Just look at the current users in that patch; they all process page_size() number of bytes, then
move on to the next struct page.

If they somehow happen to have a tail page, then we want them to process
PAGE_SIZE bytes at a time, then move onto the next page, until they hit
a head page.  If calling page_size() on a tail page returned the size
of the entire compound page, then it would process some bytes from pages
which weren't part of this compound page.

So I think the current definition of page_size() is right.