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

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

[Andrew Morton]

On Wed, 7 Aug 2013 17:12:36 -0400 Ed Cashin <ecashin@coraid.com> wrote:

> 
> On Aug 7, 2013, at 4:58 PM, Andrew Morton wrote:
> 
> > On Thu, 1 Aug 2013 21:29:59 -0400 Ed Cashin <ecashin@coraid.com> wrote:
> > 
> >> As discussed previously,
> > 
> > I think I missed that.
> > 
> >> the fact that some users of the block
> >> layer provide bios that point to pages with a zero _count means
> >> that it is not OK for the network layer to do a put_page on the
> >> skb frags during an skb_linearize, so the aoe driver gets a
> >> reference to pages in bios and puts the reference before ending
> >> the bio.  And because it cannot use get_page on a page with a
> >> zero _count, it manipulates the value directly.
> > 
> > Eh?  What code is putting count==0 pages into bios?  That sounds very
> > weird and broken.
> 
> I thought so in 2007 but couldn't solicit a clear "this is wrong" consensus from the discussion.
> 
>   http://article.gmane.org/gmane.linux.kernel/499197
>   https://lkml.org/lkml/2007/1/19/56
>   https://lkml.org/lkml/2006/12/18/230
> 
> We were seeing zero-count pages in bios from XFS, but Christoph Hellwig pointed out that kmalloced pages can also come from ext3 when it's doing log recovery, and they'll have zero page counts.

aiiee!

It is (I suppose) reasonable to put kmalloced memory into a BIO's page
array.  And it is perfectly reasonable for a user of that bio to do a
get_page/put_page against that page.  It is utterly unreasonable for
the damn page to get freed as a result!

I'd claim that slab is broken.  The page is in use, so it should have an
elevated refcount, full stop.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

On Wed, 7 Aug 2013 14:18:35 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 7 Aug 2013 17:12:36 -0400 Ed Cashin <ecashin@coraid.com> wrote:
> 
> > 
> > On Aug 7, 2013, at 4:58 PM, Andrew Morton wrote:
> > 
> > > On Thu, 1 Aug 2013 21:29:59 -0400 Ed Cashin <ecashin@coraid.com> wrote:
> > > 
> > >> As discussed previously,
> > > 
> > > I think I missed that.
> > > 
> > >> the fact that some users of the block
> > >> layer provide bios that point to pages with a zero _count means
> > >> that it is not OK for the network layer to do a put_page on the
> > >> skb frags during an skb_linearize, so the aoe driver gets a
> > >> reference to pages in bios and puts the reference before ending
> > >> the bio.  And because it cannot use get_page on a page with a
> > >> zero _count, it manipulates the value directly.
> > > 
> > > Eh?  What code is putting count==0 pages into bios?  That sounds very
> > > weird and broken.
> > 
> > I thought so in 2007 but couldn't solicit a clear "this is wrong" consensus from the discussion.
> > 
> >   http://article.gmane.org/gmane.linux.kernel/499197
> >   https://lkml.org/lkml/2007/1/19/56
> >   https://lkml.org/lkml/2006/12/18/230
> > 
> > We were seeing zero-count pages in bios from XFS, but Christoph Hellwig pointed out that kmalloced pages can also come from ext3 when it's doing log recovery, and they'll have zero page counts.
> 
> aiiee!
> 
> It is (I suppose) reasonable to put kmalloced memory into a BIO's page
> array.  And it is perfectly reasonable for a user of that bio to do a
> get_page/put_page against that page.  It is utterly unreasonable for
> the damn page to get freed as a result!
> 
> I'd claim that slab is broken.  The page is in use, so it should have an
> elevated refcount, full stop.
> 

err, no.  slab.c uses alloc_pages(), so the underlying page indeed has
a proper refcount.  I'm still not understanding how this situation comes
about.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Ed Cashin]

On Aug 7, 2013, at 5:27 PM, Andrew Morton wrote:

> On Wed, 7 Aug 2013 14:18:35 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Wed, 7 Aug 2013 17:12:36 -0400 Ed Cashin <ecashin@coraid.com> wrote:
>> 
>>> 
>>> On Aug 7, 2013, at 4:58 PM, Andrew Morton wrote:
>>> 
>>>> On Thu, 1 Aug 2013 21:29:59 -0400 Ed Cashin <ecashin@coraid.com> wrote:
>>>> 
>>>>> As discussed previously,
>>>> 
>>>> I think I missed that.
>>>> 
>>>>> the fact that some users of the block
>>>>> layer provide bios that point to pages with a zero _count means
>>>>> that it is not OK for the network layer to do a put_page on the
>>>>> skb frags during an skb_linearize, so the aoe driver gets a
>>>>> reference to pages in bios and puts the reference before ending
>>>>> the bio.  And because it cannot use get_page on a page with a
>>>>> zero _count, it manipulates the value directly.
>>>> 
>>>> Eh?  What code is putting count==0 pages into bios?  That sounds very
>>>> weird and broken.
>>> 
>>> I thought so in 2007 but couldn't solicit a clear "this is wrong" consensus from the discussion.
>>> 
>>>  http://article.gmane.org/gmane.linux.kernel/499197
>>>  https://lkml.org/lkml/2007/1/19/56
>>>  https://lkml.org/lkml/2006/12/18/230
>>> 
>>> We were seeing zero-count pages in bios from XFS, but Christoph Hellwig pointed out that kmalloced pages can also come from ext3 when it's doing log recovery, and they'll have zero page counts.
>> 
>> aiiee!
>> 
>> It is (I suppose) reasonable to put kmalloced memory into a BIO's page
>> array.  And it is perfectly reasonable for a user of that bio to do a
>> get_page/put_page against that page.  It is utterly unreasonable for
>> the damn page to get freed as a result!
>> 
>> I'd claim that slab is broken.  The page is in use, so it should have an
>> elevated refcount, full stop.
>> 
> 
> err, no.  slab.c uses alloc_pages(), so the underlying page indeed has
> a proper refcount.  I'm still not understanding how this situation comes
> about.

It sounds like it's wrong to give block pages with a zero count, so why not just have aoe BUG_ON(compound_trans_head(bv->page->_count) == 0) until we're sure nobody does that anymore?

If that idea makes sense to you, I will submit a new patch to follow the one under discussion.

-- 
  Ed Cashin
  ecashin@coraid.com


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Ed Cashin]

On Aug 7, 2013, at 7:41 PM, Ed Cashin wrote:

> It sounds like it's wrong to give block pages with a zero count, so why not just have aoe BUG_ON(compound_trans_head(bv->page->_count) == 0) until we're sure nobody does that anymore?

Ugh.  I goofed the parens and such.  I meant,

  BUG_ON(compound_trans_head(bv->bv_page)->_count == 0)

... will catch the cases we think should not be occurring.

> If that idea makes sense to you, I will submit a new patch to follow the one under discussion.

-- 
  Ed Cashin
  ecashin@coraid.com


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

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

> On Aug 7, 2013, at 5:27 PM, Andrew Morton wrote:
> 
> >> elevated refcount, full stop.
> >> 
> > 
> > err, no.  slab.c uses alloc_pages(), so the underlying page indeed has
> > a proper refcount.  I'm still not understanding how this situation comes
> > about.
> 
> It sounds like it's wrong to give block pages with a zero count,

Depends on your definition of "page".  It should be OK to put a
_count==0 tail page into a BIO, because the MM knows that it's a tail
page and that its refcount actually lives in the head page.

> so why not just have aoe BUG_ON(compound_trans_head(bv->page->_count) == 0) until we're sure nobody does that anymore?

AOE shouldn't be touching ->_count at all.  That's why it has the
leading underscore.  If AOE can stick with the usual interfaces such as
page_count(), everything should work?


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>