From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1161205AbXCAXb6@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1161205AbXCAXb6 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 18:31:58 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1161213AbXCAXb6
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 18:31:58 -0500
Received: from ns1.coraid.com ([65.14.39.133]:31124 "EHLO coraid.com"
	rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
	id S1161205AbXCAXb4 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 18:31:56 -0500
X-Greylist: delayed 959 seconds by postgrey-1.27 at vger.kernel.org; Thu, 01 Mar 2007 18:31:55 EST
Date: Thu, 1 Mar 2007 18:15:10 -0500
From: "Ed L. Cashin" <ecashin@coraid.com>
To: linux-kernel@vger.kernel.org
Cc: Greg KH <greg@kroah.com>, Andrew Morton <akpm@osdl.org>,
       ecashin@coraid.com
Subject: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070301231510.GC8524@coraid.com>
Reply-To: support@coraid.com
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.5.11+cvs20060126
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

This patch works around a problem discussed here and on the XFS
mailing list in January.

  http://lkml.org/lkml/2007/1/19/56

To summarize the issue: If XFS (or any other creator of bios) gives
the aoe driver a bio with pages that have a zero page _count, and then
the aoe driver hands the page to the network layer in an sk_buff's
frags, and if the network card does not support the scatter gather
feature, then the network layer will eventually try to put_page on the
page, and the kernel winds up panicing.

There is a disconnect between the assumptions of the bio creator (that
pages don't need to have a non-zero _count), and the assumptions of
the network layer (where it's assumed that pages will always have a
positive count).  There was no response, though, to a call in January
for ideas about resolving the disconnect.

So to work around the issue, the simple patch below increments the
page _count before handing it to the network layer and decrements it
after the network layer is done with the page.  This patch eliminates
panics for XFS on aoe users who lack scatter gather support in their
network cards.

It's regrettable that _count is manipulated directly, because Andrew
Morton changed the page "count" member to a _count to prevent exactly
this kind of direct manipulation of the data.  There does not appear
to be a "right" way to increment and decrement the count, however,
inside a driver without unwanted side effects.  The closest candidates
are in mm/internal.h and are presumably intended to be used
exclusively by mm/*.c.

Signed-off-by: "Ed L. Cashin" <ecashin@coraid.com>

diff -upr -X linux-2.6.21-rc1.dontdiff linux-2.6.21-rc1.orig/drivers/block/aoe/aoe.h linux-2.6.21-rc1/drivers/block/aoe/aoe.h
--- linux-2.6.21-rc1.orig/drivers/block/aoe/aoe.h	2007-02-27 14:11:06.249132000 -0500
+++ linux-2.6.21-rc1/drivers/block/aoe/aoe.h	2007-02-27 17:43:22.037069000 -0500
@@ -150,6 +150,7 @@ int aoeblk_init(void);
 void aoeblk_exit(void);
 void aoeblk_gdalloc(void *);
 void aoedisk_rm_sysfs(struct aoedev *d);
+void aoe_bio_done(struct bio *bio, unsigned int bytes_done, int error);
 
 int aoechr_init(void);
 void aoechr_exit(void);
diff -upr -X linux-2.6.21-rc1.dontdiff linux-2.6.21-rc1.orig/drivers/block/aoe/aoeblk.c linux-2.6.21-rc1/drivers/block/aoe/aoeblk.c
--- linux-2.6.21-rc1.orig/drivers/block/aoe/aoeblk.c	2007-02-27 14:11:06.253132000 -0500
+++ linux-2.6.21-rc1/drivers/block/aoe/aoeblk.c	2007-02-27 17:43:22.037069000 -0500
@@ -14,6 +14,29 @@
 
 static struct kmem_cache *buf_pool_cache;
 
+/* workaround for XFS and bios with zero pageref pages in general */
+void
+aoe_bio_done(struct bio *bio, unsigned int bytes_done, int error)
+{
+	struct bio_vec *bv;
+	int i;
+
+	bio_for_each_segment(bv, bio, i)
+		atomic_dec(&bv->bv_page->_count);
+
+	bio_endio(bio, bytes_done, error);
+}
+
+static void
+bio_refpages(struct bio *bio)
+{
+	struct bio_vec *bv;
+	int i;
+
+	bio_for_each_segment(bv, bio, i)
+		atomic_inc(&bv->bv_page->_count);
+}
+
 static ssize_t aoedisk_show_state(struct gendisk * disk, char *page)
 {
 	struct aoedev *d = disk->private_data;
@@ -147,6 +170,7 @@ aoeblk_make_request(request_queue_t *q, 
 	buf->bio = bio;
 	buf->resid = bio->bi_size;
 	buf->sector = bio->bi_sector;
+	bio_refpages(bio);
 	buf->bv = &bio->bi_io_vec[bio->bi_idx];
 	WARN_ON(buf->bv->bv_len == 0);
 	buf->bv_resid = buf->bv->bv_len;
@@ -159,7 +183,7 @@ aoeblk_make_request(request_queue_t *q, 
 			d->aoemajor, d->aoeminor);
 		spin_unlock_irqrestore(&d->lock, flags);
 		mempool_free(buf, d->bufpool);
-		bio_endio(bio, bio->bi_size, -ENXIO);
+		aoe_bio_done(bio, bio->bi_size, -ENXIO);
 		return 0;
 	}
 
diff -upr -X linux-2.6.21-rc1.dontdiff linux-2.6.21-rc1.orig/drivers/block/aoe/aoecmd.c linux-2.6.21-rc1/drivers/block/aoe/aoecmd.c
--- linux-2.6.21-rc1.orig/drivers/block/aoe/aoecmd.c	2007-02-27 14:11:06.253132000 -0500
+++ linux-2.6.21-rc1/drivers/block/aoe/aoecmd.c	2007-02-27 17:43:22.037069000 -0500
@@ -649,7 +649,7 @@ aoecmd_ata_rsp(struct sk_buff *skb)
 			disk_stat_add(disk, sectors[rw], n_sect);
 			disk_stat_add(disk, io_ticks, duration);
 			n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
-			bio_endio(buf->bio, buf->bio->bi_size, n);
+			aoe_bio_done(buf->bio, buf->bio->bi_size, n);
 			mempool_free(buf, d->bufpool);
 		}
 	}
diff -upr -X linux-2.6.21-rc1.dontdiff linux-2.6.21-rc1.orig/drivers/block/aoe/aoedev.c linux-2.6.21-rc1/drivers/block/aoe/aoedev.c
--- linux-2.6.21-rc1.orig/drivers/block/aoe/aoedev.c	2007-02-27 14:11:06.253132000 -0500
+++ linux-2.6.21-rc1/drivers/block/aoe/aoedev.c	2007-02-27 17:43:22.041069250 -0500
@@ -119,7 +119,7 @@ aoedev_downdev(struct aoedev *d)
 		bio = buf->bio;
 		if (--buf->nframesout == 0) {
 			mempool_free(buf, d->bufpool);
-			bio_endio(bio, bio->bi_size, -EIO);
+			aoe_bio_done(bio, bio->bi_size, -EIO);
 		}
 		skb_shinfo(f->skb)->nr_frags = f->skb->data_len = 0;
 	}
@@ -130,7 +130,7 @@ aoedev_downdev(struct aoedev *d)
 		list_del(d->bufq.next);
 		bio = buf->bio;
 		mempool_free(buf, d->bufpool);
-		bio_endio(bio, bio->bi_size, -EIO);
+		aoe_bio_done(bio, bio->bi_size, -EIO);
 	}
 
 	if (d->gd)


-- 
  Ed L Cashin <ecashin@coraid.com>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422709AbXCBBmP@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422709AbXCBBmP (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 20:42:15 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422710AbXCBBmP
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 20:42:15 -0500
Received: from smtp.osdl.org ([65.172.181.24]:34597 "EHLO smtp.osdl.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422709AbXCBBmO (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 20:42:14 -0500
Date: Thu, 1 Mar 2007 17:42:04 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: support@coraid.com
Cc: "Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-Id: <20070301174204.a550dd3a.akpm@linux-foundation.org>
In-Reply-To: <20070301231510.GC8524@coraid.com>
References: <20070301231510.GC8524@coraid.com>
X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.6; i686-pc-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 1 Mar 2007 18:15:10 -0500
"Ed L. Cashin" <ecashin@coraid.com> wrote:

> This patch works around a problem discussed here and on the XFS
> mailing list in January.
> 
>   http://lkml.org/lkml/2007/1/19/56
> 
> To summarize the issue: If XFS (or any other creator of bios) gives
> the aoe driver a bio with pages that have a zero page _count, and then
> the aoe driver hands the page to the network layer in an sk_buff's
> frags, and if the network card does not support the scatter gather
> feature, then the network layer will eventually try to put_page on the
> page, and the kernel winds up panicing.
> 
> There is a disconnect between the assumptions of the bio creator (that
> pages don't need to have a non-zero _count), and the assumptions of
> the network layer (where it's assumed that pages will always have a
> positive count).  There was no response, though, to a call in January
> for ideas about resolving the disconnect.
> 
> So to work around the issue, the simple patch below increments the
> page _count before handing it to the network layer and decrements it
> after the network layer is done with the page.  This patch eliminates
> panics for XFS on aoe users who lack scatter gather support in their
> network cards.

Something funny is going on here.

Generally, one should increment the refcount of a page when it is put into
some container.  That means that the page should get +1 when it is added to
a bio.  (direct-io does this, but the mpage.c pagecache code cheats, and
relies upon PG_locked and PG-writeback protecting the page).

Similarly, the network code (or its caller) should be incrementing the
page's refcount as the page goes into a container (ie: the skb) and
decrementing it as the page is removed.

But someone somewhere is breaking those rules.  Who?

> It's regrettable that _count is manipulated directly, because Andrew
> Morton changed the page "count" member to a _count to prevent exactly
> this kind of direct manipulation of the data.  There does not appear
> to be a "right" way to increment and decrement the count, however,
> inside a driver without unwanted side effects.  The closest candidates
> are in mm/internal.h and are presumably intended to be used
> exclusively by mm/*.c.
> 

Odd.  You _should_ be able to use plain old get_page() and put_page().  If
you use a raw decrement of page->count then there's a risk that this was
the final reference and the page will leak forever.  We do need to use
put_page()'s return-it-to-the-allocator-if-this-was-the-last-use logic.


So.  Who is breaking refcounting protocol here?  Perhaps it is AOE, failing
to increment the refcount on pages as they are added to an skb?

(Do we know which callsite in XFS is adding zero-ref pages to a BIO, btw?)


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422788AbXCBC3Z@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422788AbXCBC3Z (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 21:29:25 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422790AbXCBC3Z
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 21:29:25 -0500
Received: from pentafluge.infradead.org ([213.146.154.40]:37665 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422788AbXCBC3Y (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 21:29:24 -0500
Date: Fri, 2 Mar 2007 02:29:19 +0000
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: support@coraid.com, "Ed L. Cashin" <ecashin@coraid.com>,
       linux-kernel@vger.kernel.org, Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070302022919.GA26285@infradead.org>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>, support@coraid.com,
	"Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
	Greg KH <greg@kroah.com>
References: <20070301231510.GC8524@coraid.com> <20070301174204.a550dd3a.akpm@linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070301174204.a550dd3a.akpm@linux-foundation.org>
User-Agent: Mutt/1.4.2.2i
X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by pentafluge.infradead.org
	See http://www.infradead.org/rpr.html
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 01, 2007 at 05:42:04PM -0800, Andrew Morton wrote:
> Something funny is going on here.

Not so funny for those who've tried to sort out the issue over
the past years and just got ignored..

> Generally, one should increment the refcount of a page when it is put into
> some container.  That means that the page should get +1 when it is added to
> a bio.  (direct-io does this, but the mpage.c pagecache code cheats, and
> relies upon PG_locked and PG-writeback protecting the page).

It's a slab page, and slab pages aren't refcounted (which is a good thing
as you don't own the whole page)

> Similarly, the network code (or its caller) should be incrementing the
> page's refcount as the page goes into a container (ie: the skb) and
> decrementing it as the page is removed.
> 
> But someone somewhere is breaking those rules.  Who?

slab code.  

> So.  Who is breaking refcounting protocol here?  Perhaps it is AOE, failing
> to increment the refcount on pages as they are added to an skb?
> 
> (Do we know which callsite in XFS is adding zero-ref pages to a BIO, btw?)

For example all log I/O is done from kmalloce pages.

Anyway, to rehash what I've been trying to get clarified for ages:


 (1) should we allow to pass slab pages into bios

and

 (2) if yes what's the way lower layers are supposed to handle them
     for any possible refcounting operations like networking or rdma.

There's also a pontial caller in ext3 that can send down kmalloc'ed
buffers: journal_write_metadata_buffer() in need_copy_out && !done_copy_out
case.  But apparently that's an almost dead code path as I've never
seen anyone tripping this one, it's always XFS that people report.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422799AbXCBDXF@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422799AbXCBDXF (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 22:23:05 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422798AbXCBDXF
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 22:23:05 -0500
Received: from smtp.osdl.org ([65.172.181.24]:39078 "EHLO smtp.osdl.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422799AbXCBDXD (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 22:23:03 -0500
Date: Thu, 1 Mar 2007 19:22:45 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: support@coraid.com, "Ed L. Cashin" <ecashin@coraid.com>,
       linux-kernel@vger.kernel.org, Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-Id: <20070301192245.bf034385.akpm@linux-foundation.org>
In-Reply-To: <20070302022919.GA26285@infradead.org>
References: <20070301231510.GC8524@coraid.com>
	<20070301174204.a550dd3a.akpm@linux-foundation.org>
	<20070302022919.GA26285@infradead.org>
X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.17; x86_64-unknown-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 2 Mar 2007 02:29:19 +0000 Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Mar 01, 2007 at 05:42:04PM -0800, Andrew Morton wrote:
> > Something funny is going on here.
> 
> Not so funny for those who've tried to sort out the issue over
> the past years and just got ignored..
> 
> > Generally, one should increment the refcount of a page when it is put into
> > some container.  That means that the page should get +1 when it is added to
> > a bio.  (direct-io does this, but the mpage.c pagecache code cheats, and
> > relies upon PG_locked and PG-writeback protecting the page).
> 
> It's a slab page, and slab pages aren't refcounted (which is a good thing
> as you don't own the whole page)

ah, I see.

> > Similarly, the network code (or its caller) should be incrementing the
> > page's refcount as the page goes into a container (ie: the skb) and
> > decrementing it as the page is removed.
> > 
> > But someone somewhere is breaking those rules.  Who?
> 
> slab code.  

Well I spose slab _could_ take a ref on these pages.

> > So.  Who is breaking refcounting protocol here?  Perhaps it is AOE, failing
> > to increment the refcount on pages as they are added to an skb?
> > 
> > (Do we know which callsite in XFS is adding zero-ref pages to a BIO, btw?)
> 
> For example all log I/O is done from kmalloce pages.
> 
> Anyway, to rehash what I've been trying to get clarified for ages:
> 
> 
>  (1) should we allow to pass slab pages into bios
> 
> and
> 
>  (2) if yes what's the way lower layers are supposed to handle them
>      for any possible refcounting operations like networking or rdma.
> 
> There's also a pontial caller in ext3 that can send down kmalloc'ed
> buffers: journal_write_metadata_buffer() in need_copy_out && !done_copy_out
> case.  But apparently that's an almost dead code path as I've never
> seen anyone tripping this one, it's always XFS that people report.

OK.  Let's go through it.

Networking internally maintains caller memory lifetimes, and it assumes
that the caller allocated memory via __alloc_pages() - because it uses
get_page() and put_page().

BIO, however, does not internally manage caller memory lifetime.  This is
because the caller's ->bi_end_io is always called, so the caller can do it.

So where we've come unstuck is in a module which has gone and fed BIO
memory into networking.  The differing design philosophies are clashing.

I'm surprised this doesn't happen in other places - aren't there any other
drivers which take a BIO and stuff it down the network?

Anyway, where's the bug?

Really, I'd say it's XFS (and ext3).  Even though BIO doesn't presently
manage page lifetimes, it _could_.  After all, the function is called
bio_add_page(), not bio_add_virtual_address().  It's a bit hacky to kmalloc
some memory, run virt_to_page() and to then present that page to BIO even
though the caller (thanks to the slab optimisation) doesn't actually have
control of that page's lifetime.

So we have a few options to look at:

a) kludge things in AOE.  Unpleasing, and might cause memory leaks
   (although it won't, because the caller hasn't run bi_end_io yet).

b) Take a ref on slab pages in slab.  A bit costly, perhaps.

c) teach ext3 and XFS to take a ref on these pages as they are added to
   the BIOs, undo that ref in bi_end_io.

I think c)?


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422856AbXCBEat@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422856AbXCBEat (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 23:30:49 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422875AbXCBEat
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 23:30:49 -0500
Received: from pentafluge.infradead.org ([213.146.154.40]:42511 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422870AbXCBEao (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 23:30:44 -0500
Date: Fri, 2 Mar 2007 04:30:39 +0000
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>, support@coraid.com,
       "Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070302043039.GA18651@infradead.org>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>, support@coraid.com,
	"Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
	Greg KH <greg@kroah.com>
References: <20070301231510.GC8524@coraid.com> <20070301174204.a550dd3a.akpm@linux-foundation.org> <20070302022919.GA26285@infradead.org> <20070301192245.bf034385.akpm@linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070301192245.bf034385.akpm@linux-foundation.org>
User-Agent: Mutt/1.4.2.2i
X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by pentafluge.infradead.org
	See http://www.infradead.org/rpr.html
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 01, 2007 at 07:22:45PM -0800, Andrew Morton wrote:
> Well I spose slab _could_ take a ref on these pages.

What it would need to do is:

 - add a reference for every object touching this page
 - don't give the page back to the page allocator or reuse any
   single object inside it until there are no more reference to the page.

I don't think this is a very good idea, although the netowkring references
tend to be rather short-term once making this not a that bad burden.

> Networking internally maintains caller memory lifetimes, and it assumes
> that the caller allocated memory via __alloc_pages() - because it uses
> get_page() and put_page().
> 
> BIO, however, does not internally manage caller memory lifetime.  This is
> because the caller's ->bi_end_io is always called, so the caller can do it.
> 
> So where we've come unstuck is in a module which has gone and fed BIO
> memory into networking.  The differing design philosophies are clashing.
> 
> I'm surprised this doesn't happen in other places - aren't there any other
> drivers which take a BIO and stuff it down the network?
> 
> Anyway, where's the bug?
> 
> Really, I'd say it's XFS (and ext3).  Even though BIO doesn't presently
> manage page lifetimes, it _could_.  After all, the function is called
> bio_add_page(), not bio_add_virtual_address().  It's a bit hacky to kmalloc
> some memory, run virt_to_page() and to then present that page to BIO even
> though the caller (thanks to the slab optimisation) doesn't actually have
> control of that page's lifetime.

That was the conclusion I came to when this was brought up initially.
Fixing up XFS would be easyish and only waste a tiny amount of memory,
and the same is true for ext3 (I did in fact suggest just using get_free_page
for this case but got shot down for stupid reasons when the slab debug
alignment issues in that area came up)

But in this case we'd really need to enforce this, and add a
BUG_ON(PageSlab(page)) in bio_add_page to trip everyone submit
this kind of pages.

> So we have a few options to look at:
> 
> a) kludge things in AOE.  Unpleasing, and might cause memory leaks
>    (although it won't, because the caller hasn't run bi_end_io yet).
> 
> b) Take a ref on slab pages in slab.  A bit costly, perhaps.
> 
> c) teach ext3 and XFS to take a ref on these pages as they are added to
>    the BIOs, undo that ref in bi_end_io.
> 
> I think c)?

Yes.  I'm perfectly fine with this as long as we document and enforce
this.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422892AbXCBEsT@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422892AbXCBEsT (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 23:48:19 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422891AbXCBEsT
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 23:48:19 -0500
Received: from smtp.osdl.org ([65.172.181.24]:41717 "EHLO smtp.osdl.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422893AbXCBEsQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 23:48:16 -0500
Date: Thu, 1 Mar 2007 20:48:06 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: support@coraid.com, "Ed L. Cashin" <ecashin@coraid.com>,
       linux-kernel@vger.kernel.org, Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-Id: <20070301204806.61225e2b.akpm@linux-foundation.org>
In-Reply-To: <20070302043039.GA18651@infradead.org>
References: <20070301231510.GC8524@coraid.com>
	<20070301174204.a550dd3a.akpm@linux-foundation.org>
	<20070302022919.GA26285@infradead.org>
	<20070301192245.bf034385.akpm@linux-foundation.org>
	<20070302043039.GA18651@infradead.org>
X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.17; x86_64-unknown-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 2 Mar 2007 04:30:39 +0000 Christoph Hellwig <hch@infradead.org> wrote:

> But in this case we'd really need to enforce this, and add a
> BUG_ON(PageSlab(page)) in bio_add_page to trip everyone submit
> this kind of pages.

That would be

	BUG_ON(PageSlab(page) && page_count(page) == 0)?


> > So we have a few options to look at:
> > 
> > a) kludge things in AOE.  Unpleasing, and might cause memory leaks
> >    (although it won't, because the caller hasn't run bi_end_io yet).
> > 
> > b) Take a ref on slab pages in slab.  A bit costly, perhaps.
> > 
> > c) teach ext3 and XFS to take a ref on these pages as they are added to
> >    the BIOs, undo that ref in bi_end_io.
> > 
> > I think c)?
> 
> Yes.  I'm perfectly fine with this as long as we document and enforce
> this.

And write the patch ;)

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422894AbXCBEtQ@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422894AbXCBEtQ (ORCPT <rfc822;w@1wt.eu>);
	Thu, 1 Mar 2007 23:49:16 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422893AbXCBEtQ
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Thu, 1 Mar 2007 23:49:16 -0500
Received: from pentafluge.infradead.org ([213.146.154.40]:36167 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422894AbXCBEtP (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 1 Mar 2007 23:49:15 -0500
Date: Fri, 2 Mar 2007 04:49:10 +0000
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>, support@coraid.com,
       "Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070302044910.GA22209@infradead.org>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>, support@coraid.com,
	"Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
	Greg KH <greg@kroah.com>
References: <20070301231510.GC8524@coraid.com> <20070301174204.a550dd3a.akpm@linux-foundation.org> <20070302022919.GA26285@infradead.org> <20070301192245.bf034385.akpm@linux-foundation.org> <20070302043039.GA18651@infradead.org> <20070301204806.61225e2b.akpm@linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070301204806.61225e2b.akpm@linux-foundation.org>
User-Agent: Mutt/1.4.2.2i
X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by pentafluge.infradead.org
	See http://www.infradead.org/rpr.html
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 01, 2007 at 08:48:06PM -0800, Andrew Morton wrote:
> On Fri, 2 Mar 2007 04:30:39 +0000 Christoph Hellwig <hch@infradead.org> wrote:
> 
> > But in this case we'd really need to enforce this, and add a
> > BUG_ON(PageSlab(page)) in bio_add_page to trip everyone submit
> > this kind of pages.
> 
> That would be
> 
> 	BUG_ON(PageSlab(page) && page_count(page) == 0)?

No, all slab pages.  Currently they all have a reference count of
zero, but we generally don't want people to pass in pages that
come from a non-refcounted allocator.


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1161178AbXCBFAy@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1161178AbXCBFAy (ORCPT <rfc822;w@1wt.eu>);
	Fri, 2 Mar 2007 00:00:54 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S964863AbXCBFAy
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Fri, 2 Mar 2007 00:00:54 -0500
Received: from smtp.osdl.org ([65.172.181.24]:42069 "EHLO smtp.osdl.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S964800AbXCBFAx (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 2 Mar 2007 00:00:53 -0500
Date: Thu, 1 Mar 2007 21:00:44 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: support@coraid.com, "Ed L. Cashin" <ecashin@coraid.com>,
       linux-kernel@vger.kernel.org, Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-Id: <20070301210044.d95bcd70.akpm@linux-foundation.org>
In-Reply-To: <20070302044910.GA22209@infradead.org>
References: <20070301231510.GC8524@coraid.com>
	<20070301174204.a550dd3a.akpm@linux-foundation.org>
	<20070302022919.GA26285@infradead.org>
	<20070301192245.bf034385.akpm@linux-foundation.org>
	<20070302043039.GA18651@infradead.org>
	<20070301204806.61225e2b.akpm@linux-foundation.org>
	<20070302044910.GA22209@infradead.org>
X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.17; x86_64-unknown-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 2 Mar 2007 04:49:10 +0000 Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Mar 01, 2007 at 08:48:06PM -0800, Andrew Morton wrote:
> > On Fri, 2 Mar 2007 04:30:39 +0000 Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > > But in this case we'd really need to enforce this, and add a
> > > BUG_ON(PageSlab(page)) in bio_add_page to trip everyone submit
> > > this kind of pages.
> > 
> > That would be
> > 
> > 	BUG_ON(PageSlab(page) && page_count(page) == 0)?
> 
> No, all slab pages.  Currently they all have a reference count of
> zero, but we generally don't want people to pass in pages that
> come from a non-refcounted allocator.

I that case we're talking about different things.

I thought the proposal was to continue to use slab pages, but to take a ref
on them as they're added to the bio, drop that ref in bi_end_io()?


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S964800AbXCBFEA@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S964800AbXCBFEA (ORCPT <rfc822;w@1wt.eu>);
	Fri, 2 Mar 2007 00:04:00 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S964863AbXCBFD7
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Fri, 2 Mar 2007 00:03:59 -0500
Received: from pentafluge.infradead.org ([213.146.154.40]:48219 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S964800AbXCBFD7 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 2 Mar 2007 00:03:59 -0500
Date: Fri, 2 Mar 2007 05:03:51 +0000
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>, support@coraid.com,
       "Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070302050351.GA24705@infradead.org>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>, support@coraid.com,
	"Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
	Greg KH <greg@kroah.com>
References: <20070301231510.GC8524@coraid.com> <20070301174204.a550dd3a.akpm@linux-foundation.org> <20070302022919.GA26285@infradead.org> <20070301192245.bf034385.akpm@linux-foundation.org> <20070302043039.GA18651@infradead.org> <20070301204806.61225e2b.akpm@linux-foundation.org> <20070302044910.GA22209@infradead.org> <20070301210044.d95bcd70.akpm@linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070301210044.d95bcd70.akpm@linux-foundation.org>
User-Agent: Mutt/1.4.2.2i
X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by pentafluge.infradead.org
	See http://www.infradead.org/rpr.html
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 01, 2007 at 09:00:44PM -0800, Andrew Morton wrote:
> I that case we're talking about different things.
> 
> I thought the proposal was to continue to use slab pages, but to take a ref
> on them as they're added to the bio, drop that ref in bi_end_io()?

That would give you silent memory corruption in case the networking code
hold a reference after the memory gets returned to slab and reused.

We need to either stop allowing to pass slab memory to the block layer,
or document that drivers need to handle it specially and give them a
way to find out about them. (Or do the horrible slab refcounting hack
I wrote up above)


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422784AbXCBFJw@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422784AbXCBFJw (ORCPT <rfc822;w@1wt.eu>);
	Fri, 2 Mar 2007 00:09:52 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422801AbXCBFJw
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Fri, 2 Mar 2007 00:09:52 -0500
Received: from smtp.osdl.org ([65.172.181.24]:42529 "EHLO smtp.osdl.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1422784AbXCBFJv (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 2 Mar 2007 00:09:51 -0500
Date: Thu, 1 Mar 2007 21:09:42 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: support@coraid.com, "Ed L. Cashin" <ecashin@coraid.com>,
       linux-kernel@vger.kernel.org, Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-Id: <20070301210942.ebf40993.akpm@linux-foundation.org>
In-Reply-To: <20070302050351.GA24705@infradead.org>
References: <20070301231510.GC8524@coraid.com>
	<20070301174204.a550dd3a.akpm@linux-foundation.org>
	<20070302022919.GA26285@infradead.org>
	<20070301192245.bf034385.akpm@linux-foundation.org>
	<20070302043039.GA18651@infradead.org>
	<20070301204806.61225e2b.akpm@linux-foundation.org>
	<20070302044910.GA22209@infradead.org>
	<20070301210044.d95bcd70.akpm@linux-foundation.org>
	<20070302050351.GA24705@infradead.org>
X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.17; x86_64-unknown-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 2 Mar 2007 05:03:51 +0000 Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Mar 01, 2007 at 09:00:44PM -0800, Andrew Morton wrote:
> > I that case we're talking about different things.
> > 
> > I thought the proposal was to continue to use slab pages, but to take a ref
> > on them as they're added to the bio, drop that ref in bi_end_io()?
> 
> That would give you silent memory corruption in case the networking code
> hold a reference after the memory gets returned to slab and reused.

Well, given that bi_end_io() is called after the "io" has completed, I'm
assuming that networking has completely finished with the memory by the
time bi_end_io() gets called.

I guess one can envisage situations where that might not happen, but they'd
be terribly buggy ones, surely.

> We need to either stop allowing to pass slab memory to the block layer,
> or document that drivers need to handle it specially and give them a
> way to find out about them. (Or do the horrible slab refcounting hack
> I wrote up above)

OK.  So you're proposing that XFS and ext3 simply stop sing slab for this
memory?


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S1422812AbXCBFP2@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1422812AbXCBFP2 (ORCPT <rfc822;w@1wt.eu>);
	Fri, 2 Mar 2007 00:15:28 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1422811AbXCBFP2
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Fri, 2 Mar 2007 00:15:28 -0500
Received: from pentafluge.infradead.org ([213.146.154.40]:57141 "EHLO
	pentafluge.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422812AbXCBFP2 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 2 Mar 2007 00:15:28 -0500
Date: Fri, 2 Mar 2007 05:15:24 +0000
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>, support@coraid.com,
       "Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
       Greg KH <greg@kroah.com>
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
Message-ID: <20070302051524.GA26803@infradead.org>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>, support@coraid.com,
	"Ed L. Cashin" <ecashin@coraid.com>, linux-kernel@vger.kernel.org,
	Greg KH <greg@kroah.com>
References: <20070301231510.GC8524@coraid.com> <20070301174204.a550dd3a.akpm@linux-foundation.org> <20070302022919.GA26285@infradead.org> <20070301192245.bf034385.akpm@linux-foundation.org> <20070302043039.GA18651@infradead.org> <20070301204806.61225e2b.akpm@linux-foundation.org> <20070302044910.GA22209@infradead.org> <20070301210044.d95bcd70.akpm@linux-foundation.org> <20070302050351.GA24705@infradead.org> <20070301210942.ebf40993.akpm@linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070301210942.ebf40993.akpm@linux-foundation.org>
User-Agent: Mutt/1.4.2.2i
X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by pentafluge.infradead.org
	See http://www.infradead.org/rpr.html
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 01, 2007 at 09:09:42PM -0800, Andrew Morton wrote:
> > or document that drivers need to handle it specially and give them a
> > way to find out about them. (Or do the horrible slab refcounting hack
> > I wrote up above)
> 
> OK.  So you're proposing that XFS and ext3 simply stop sing slab for this
> memory?

Yes.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner+w=401wt.eu-S2992567AbXCBQMI@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S2992567AbXCBQMI (ORCPT <rfc822;w@1wt.eu>);
	Fri, 2 Mar 2007 11:12:08 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org id S2992569AbXCBQMI
	(ORCPT <rfc822;linux-kernel-outgoing>);
	Fri, 2 Mar 2007 11:12:08 -0500
Received: from ns1.coraid.com ([65.14.39.133]:8595 "EHLO coraid.com"
	rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP
	id S2992567AbXCBQMH (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 2 Mar 2007 11:12:07 -0500
X-Greylist: delayed 1199 seconds by postgrey-1.27 at vger.kernel.org; Fri, 02 Mar 2007 11:12:07 EST
Message-ID: <8d855010270590ff0cad9e99e7b95863@coraid.com>
To: akpm@linux-foundation.org, hch@infradead.org
CC: support@coraid.com, ecashin@coraid.com, linux-kernel@vger.kernel.org,
       greg@kroah.com
Subject: Re: PATCH 2.6.21-rc1 aoe: handle zero _count pages in bios
From: Sam Hopkins <sah@coraid.com>
Date: Fri, 2 Mar 2007 10:51:00 -0500
In-Reply-To: <20070301210942.ebf40993.akpm@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org

> Well, given that bi_end_io() is called after the "io" has completed, I'm
> assuming that networking has completely finished with the memory by the
> time bi_end_io() gets called.
> 
> I guess one can envisage situations where that might not happen, but they'd
> be terribly buggy ones, surely.

This is actually quite common when using broadcom chipsets that take a
long time to clean out the tx ring.  We send a command skb out to
write some data, get the response some tens of ms later and the
command skb (with the pages) still sits in the tx ring.  I've gone to
some lengths to limit the skb memory used in aoe to help with the
OOM/swap issue and this has given me headaches.

Sam


