Messages in this thread | | | Subject | Re: [PATCH] aoe: adjust ref of head for compound page tails | From | Ed Cashin <> | Date | Wed, 7 Aug 2013 20:50:09 -0400 |
| |
On Aug 7, 2013, at 8:05 PM, Andrew Morton wrote:
> 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().
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.
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.
-- Ed Cashin ecashin@coraid.com
| |