On Aug 7, 2013, at 8:05 PM, Andrew Morton wrote:
> On Wed, 7 Aug 2013 19:54:45 -0400 Ed Cashin <ecas...@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
ecas...@coraid.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
