On Tue, Jan 01, 2019 at 03:41:00PM +0530, Aneesh Kumar K.V wrote:
> Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
> > On Tue, Jan 01, 2019 at 08:57:53AM +0530, Aneesh Kumar K.V wrote:
> >> Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
