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

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

On Wed, 7 Aug 2013 19:54:45 -0400 Ed Cashin <ecashin@coraid.com> wrote:

> > +bio_pageinc(struct bio *bio)
> > +{
> > +       struct bio_vec *bv;
> > +       struct page *page;
> > +       int i;
> > +
> > +       bio_for_each_segment(bv, bio, i) {
> > +               page = bv->bv_page;
> > +               /* Non-zero page count for non-head members of
> > +                * compound pages is no longer allowed by the kernel,
> > +                * but this has never been seen here.
> > +                */
> > +               if (unlikely(PageCompound(page)))
> > +                       if (compound_trans_head(page) != page) {
> > +                               pr_crit("page tail used for block I/O\n");
> > +                               BUG();
> > +                       }
> > 
> > But get_page/put_page against a tail page of a compound page should
> > Just Work.  The core MM will hunt down the head page and manipulate its
> > refcount.  
> > 
> > Perhaps the problem here is that AOE is diving under the covers and is
> > using low-level page refcount alterations:
> > 
> > +               atomic_inc(&page->_count);
> > 
> > Why does AOE do this?  It would be better if it were to use the
> > official published MM interfaces.  If those interfaces need
> > alteration/augmentation then we can do that.
> 
> The get_page cannot be used when the _count is 0 (which is exactly when we need to increment it), because it will VM_BUG_ON.
> 
> 	/*
> 	 * 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);
> 

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().