Re: [PATCH] aoe: adjust ref of head for compound page tails

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 7 Aug 2013 20:50:09 -0400 Ed Cashin <ecashin@coraid.com> wrote:

> > But we shouldn't get that far:
> > 
> > static inline void get_page(struct page *page)
> > {
> > 	if (unlikely(PageTail(page)))
> > 		if (likely(__get_page_tail(page)))
> > 			return;
> > 	/*
> > 	 * Getting a normal page or the head of a compound page
> > 	 * requires to already have an elevated page->_count.
> > 	 */
> > 	VM_BUG_ON(atomic_read(&page->_count) <= 0);
> > 	atomic_inc(&page->_count);
> > }
> > 
> > This is a tail page, so we should be using __get_page_tail().
> 
> 
> When the workaround was created, it was with the assumption that the zero-count pages are not always tail pages, and that seemed to be the case in 2007, but as I said, I don't have a mechanism for detecting that now, so I cannot say whether it really happens with today's kernel.

It sounds we should pull out all that code and retest.  It shouldn't be
needed - if this results in some failure then I suspect core MM will
need changes.

Why don't you have a "mechanism for detecting that"?  It's a matter of
pointing AOE at some hugetlb pages?

> If it is never correct for normal pages or compound page heads to have a zero count while they are associated with a bio, then yes, I think get_page is a great solution.  The VM_BUG_ON in get_page would identify any parts of the kernel that are supplying bios that have pages without references.
> 
> Just a note in response to "we shouldn't get that far":  I believe the VM_BUG_ON line in get_page does get executed for tail pages when the __get_page_tail returns false because the compound page head has a zero count:
> 
>   get_page -> __get_page_tail -> get_page_unless_zero returns false, so
>     __get_page_tail returns "got", which is still false, so
>       get_page executes the VM_BUG_ON, where the _count will be zero for the tail page.

Yup.  If we hit a zero-ref huge page here then we want that BUG_ON to trigger.