[PATCH] [XFS] Free buffer pages array unconditionally

[PATCH] [XFS] Free buffer pages array unconditionally

[Dave Chinner]

The code in xfs_free_buf() only attempts to free the b_pages array if the
buffer is a page cache backed or page allocated buffer. The extra log buffer
that is used when the log wraps uses pages that are allocated to a different
log buffer, but it still has a b_pages array allocated when those pages
are associated to with the extra buffer in xfs_buf_associate_memory.

Hence we need to always attempt to free the b_pages array when tearing
down a buffer, not just on buffers that are explicitly marked as page bearing
buffers. This fixes a leak detected by the kernel memory leak code.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_buf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
index 4ddc973..4b84bbc 100644
--- a/fs/xfs/linux-2.6/xfs_buf.c
+++ b/fs/xfs/linux-2.6/xfs_buf.c
@@ -316,7 +316,7 @@ STATIC void
 _xfs_buf_free_pages(
 	xfs_buf_t	*bp)
 {
-	if (bp->b_pages != bp->b_page_array) {
+	if (bp->b_pages && bp->b_pages != bp->b_page_array) {
 		kmem_free(bp->b_pages);
 	}
 }
@@ -349,9 +349,9 @@ xfs_buf_free(
 				ASSERT(!PagePrivate(page));
 			page_cache_release(page);
 		}
-		_xfs_buf_free_pages(bp);
 	}
 
+	_xfs_buf_free_pages(bp);
 	xfs_buf_deallocate(bp);
 }
 
-- 
1.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [XFS] Free buffer pages array unconditionally

[Christoph Hellwig]

On Wed, Dec 02, 2009 at 05:12:13PM +1100, Dave Chinner wrote:
> The code in xfs_free_buf() only attempts to free the b_pages array if the
> buffer is a page cache backed or page allocated buffer. The extra log buffer
> that is used when the log wraps uses pages that are allocated to a different
> log buffer, but it still has a b_pages array allocated when those pages
> are associated to with the extra buffer in xfs_buf_associate_memory.
> 
> Hence we need to always attempt to free the b_pages array when tearing
> down a buffer, not just on buffers that are explicitly marked as page bearing
> buffers. This fixes a leak detected by the kernel memory leak code.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/linux-2.6/xfs_buf.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
> index 4ddc973..4b84bbc 100644
> --- a/fs/xfs/linux-2.6/xfs_buf.c
> +++ b/fs/xfs/linux-2.6/xfs_buf.c
> @@ -316,7 +316,7 @@ STATIC void
>  _xfs_buf_free_pages(
>  	xfs_buf_t	*bp)
>  {
> -	if (bp->b_pages != bp->b_page_array) {
> +	if (bp->b_pages && bp->b_pages != bp->b_page_array) {
>  		kmem_free(bp->b_pages);

kmem_free happily takes a NULL pointer, so this is unessecary.

> @@ -349,9 +349,9 @@ xfs_buf_free(
>  				ASSERT(!PagePrivate(page));
>  			page_cache_release(page);
>  		}
> -		_xfs_buf_free_pages(bp);
>  	}
>  
> +	_xfs_buf_free_pages(bp);

This part looks correct, good catch.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [XFS] Free buffer pages array unconditionally

[Dave Chinner]

On Wed, Dec 02, 2009 at 10:17:42AM -0500, Christoph Hellwig wrote:
> On Wed, Dec 02, 2009 at 05:12:13PM +1100, Dave Chinner wrote:
> > The code in xfs_free_buf() only attempts to free the b_pages array if the
> > buffer is a page cache backed or page allocated buffer. The extra log buffer
> > that is used when the log wraps uses pages that are allocated to a different
> > log buffer, but it still has a b_pages array allocated when those pages
> > are associated to with the extra buffer in xfs_buf_associate_memory.
> > 
> > Hence we need to always attempt to free the b_pages array when tearing
> > down a buffer, not just on buffers that are explicitly marked as page bearing
> > buffers. This fixes a leak detected by the kernel memory leak code.
> > 
> > Signed-off-by: Dave Chinner <david@fromorbit.com>
> > ---
> >  fs/xfs/linux-2.6/xfs_buf.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
> > index 4ddc973..4b84bbc 100644
> > --- a/fs/xfs/linux-2.6/xfs_buf.c
> > +++ b/fs/xfs/linux-2.6/xfs_buf.c
> > @@ -316,7 +316,7 @@ STATIC void
> >  _xfs_buf_free_pages(
> >  	xfs_buf_t	*bp)
> >  {
> > -	if (bp->b_pages != bp->b_page_array) {
> > +	if (bp->b_pages && bp->b_pages != bp->b_page_array) {
> >  		kmem_free(bp->b_pages);
> 
> kmem_free happily takes a NULL pointer, so this is unessecary.

Yes, it does, but I wanted to make sure that b_pages had been
assigned before doing the comparison because this is now
called unconditionally. I´ll remove the check and retest.

Hmmm - I suspect that this function needs to NULL b_pages
in case it _xfs_buf_free_pages() is called prior to calling
xfs_buf_free()...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [XFS] Free buffer pages array unconditionally

[Christoph Hellwig]

On Thu, Dec 03, 2009 at 09:22:28AM +1100, Dave Chinner wrote:
> > kmem_free happily takes a NULL pointer, so this is unessecary.
> 
> Yes, it does, but I wanted to make sure that b_pages had been
> assigned before doing the comparison because this is now
> called unconditionally. I?ll remove the check and retest.
> 
> Hmmm - I suspect that this function needs to NULL b_pages
> in case it _xfs_buf_free_pages() is called prior to calling
> xfs_buf_free()...

Yes, at least for now.  Long term we should stop messing with the page array
on a live buffer.  The only place where we currently do that is
xfs_buf_associate_memory, which has two callers, one for the log wrap
reserver buffer, and the other in various places in log recovery for
aligned v2 logs.

For the first one we know the max number of pages we might have to deal
with so we can pass it when allocating the buffer, and for the second
one the number of pages can't be bigger than the original number of
pages for the buffer every so there's no need to do the reallocation
game at all.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] XFS: Free buffer pages array unconditionally

[Dave Chinner]

The code in xfs_free_buf() only attempts to free the b_pages array if the
buffer is a page cache backed or page allocated buffer. The extra log buffer
that is used when the log wraps uses pages that are allocated to a different
log buffer, but it still has a b_pages array allocated when those pages
are associated to with the extra buffer in xfs_buf_associate_memory.

Hence we need to always attempt to free the b_pages array when tearing
down a buffer, not just on buffers that are explicitly marked as page bearing
buffers. This fixes a leak detected by the kernel memory leak code.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_buf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
index 4ddc973..529d6a6 100644
--- a/fs/xfs/linux-2.6/xfs_buf.c
+++ b/fs/xfs/linux-2.6/xfs_buf.c
@@ -318,6 +318,7 @@ _xfs_buf_free_pages(
 {
 	if (bp->b_pages != bp->b_page_array) {
 		kmem_free(bp->b_pages);
+		bp->b_pages = NULL;
 	}
 }
 
@@ -349,9 +350,8 @@ xfs_buf_free(
 				ASSERT(!PagePrivate(page));
 			page_cache_release(page);
 		}
-		_xfs_buf_free_pages(bp);
 	}
-
+	_xfs_buf_free_pages(bp);
 	xfs_buf_deallocate(bp);
 }
 
-- 
1.6.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

RE: [PATCH] XFS: Free buffer pages array unconditionally

[Alex Elder]

Dave Chinner wrote:
> The code in xfs_free_buf() only attempts to free the b_pages array if the
> buffer is a page cache backed or page allocated buffer. The extra log buffer
> that is used when the log wraps uses pages that are allocated to a different
> log buffer, but it still has a b_pages array allocated when those pages
> are associated to with the extra buffer in xfs_buf_associate_memory.
> 
> Hence we need to always attempt to free the b_pages array when tearing
> down a buffer, not just on buffers that are explicitly marked as page bearing
> buffers. This fixes a leak detected by the kernel memory leak code.

Three places call xfs_buf_get_pages();
- _xfs_buf_lookup_pages(), which sets the _XBF_PAGE_CACHE flag in the
  buffer after the call
- xfs_buf_associate_memory(), which sets no flag bit
- xfs_buf_get_noaddr(), which sets the _XBF_PAGES flag.

The only place that checks for _XBF_PAGES is xfs_buf_free().

Given that, I have two comments:
- You could just as easily have set the _XBF_PAGES flag in
  xfs_buf_associate_memory, thereby making that flag indicate
  consistently that the buffer has allocated pages
- Or, since you are proposing unconditionally freeing the
  pages, we can perhaps drop the _XBF_PAGES flag altogether
  since it no longer serves much purpose.  (I prefer this.)

					-Alex

> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/linux-2.6/xfs_buf.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
> index 4ddc973..529d6a6 100644
> --- a/fs/xfs/linux-2.6/xfs_buf.c
> +++ b/fs/xfs/linux-2.6/xfs_buf.c
> @@ -318,6 +318,7 @@ _xfs_buf_free_pages(
>  {
>  	if (bp->b_pages != bp->b_page_array) {
>  		kmem_free(bp->b_pages);
> +		bp->b_pages = NULL;
>  	}
>  }
> 
> @@ -349,9 +350,8 @@ xfs_buf_free(
>  				ASSERT(!PagePrivate(page));
>  			page_cache_release(page);
>  		}
> -		_xfs_buf_free_pages(bp);
>  	}
> -
> +	_xfs_buf_free_pages(bp);
>  	xfs_buf_deallocate(bp);
>  }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] XFS: Free buffer pages array unconditionally

[Dave Chinner]

On Tue, Dec 15, 2009 at 01:21:36PM -0600, Alex Elder wrote:
> Dave Chinner wrote:
> > The code in xfs_free_buf() only attempts to free the b_pages array if the
> > buffer is a page cache backed or page allocated buffer. The extra log buffer
> > that is used when the log wraps uses pages that are allocated to a different
> > log buffer, but it still has a b_pages array allocated when those pages
> > are associated to with the extra buffer in xfs_buf_associate_memory.
> > 
> > Hence we need to always attempt to free the b_pages array when tearing
> > down a buffer, not just on buffers that are explicitly marked as page bearing
> > buffers. This fixes a leak detected by the kernel memory leak code.
> 
> Three places call xfs_buf_get_pages();
> - _xfs_buf_lookup_pages(), which sets the _XBF_PAGE_CACHE flag in the
>   buffer after the call
> - xfs_buf_associate_memory(), which sets no flag bit
> - xfs_buf_get_noaddr(), which sets the _XBF_PAGES flag.
> 
> The only place that checks for _XBF_PAGES is xfs_buf_free().
> 
> Given that, I have two comments:
> - You could just as easily have set the _XBF_PAGES flag in
>   xfs_buf_associate_memory, thereby making that flag indicate
>   consistently that the buffer has allocated pages

_XBF_PAGES is used to indicate that the buffer owns the
pages and must free them when the buffer is freed.

_XBF_PAGE_CACHE indicates pages are allocated out of the buftarg
page cache and need to be released when the buffer is freed.

In both cases, xfs_buf_free() has to free the pages
associated with these buffers.

In the case of xfs_buf_associate_memory(), the memory attached to
the buffer is owned by an external entity and hence we are not
allowed to free it when we free the buffer. Therefore we can't set
_XBF_PAGES on the buffer.

> - Or, since you are proposing unconditionally freeing the
>   pages,

This change doesn't unconditionally free the pages attached
to the buffer - it unconditionally frees the array used to
index the pages on the buffer when the buffer is torn down.
i.e:

   +------+         +------+
   |  bp  |         |      |-------> page
   |      | b_pages |      |-------> page
   |      |-------->|      |.............
   |      |         |      |-------> page
   |      |         |      |-------> page
   +------+         +------+
                     ^^^^^^
		This array is what we are
		freeing unconditionally.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

RE: [PATCH] XFS: Free buffer pages array unconditionally

[Alex Elder]

Dave Chinner wrote:
> On Tue, Dec 15, 2009 at 01:21:36PM -0600, Alex Elder wrote:
>> Dave Chinner wrote:
>>> The code in xfs_free_buf() only attempts to free the b_pages array if the
>>> buffer is a page cache backed or page allocated buffer. The extra log buffer
>>> that is used when the log wraps uses pages that are allocated to a different
>>> log buffer, but it still has a b_pages array allocated when those pages
>>> are associated to with the extra buffer in xfs_buf_associate_memory.

I get it now.  You're right, I was confusing the array
with the pages themselves.  And in hindsight the title
of your patch and the explanation is abundantly clear
on what's being freed...  Thank you for carefully
explaining it to me (again).

The patch looks good.

					-Alex

Reviewed-by: Alex Elder <aelder@sgi.com>
 
>>> Hence we need to always attempt to free the b_pages array when tearing
>>> down a buffer, not just on buffers that are explicitly marked as page bearing
>>> buffers. This fixes a leak detected by the kernel memory leak code.
>> 
>> Three places call xfs_buf_get_pages();
>> - _xfs_buf_lookup_pages(), which sets the _XBF_PAGE_CACHE flag in the   buffer after the call
>> - xfs_buf_associate_memory(), which sets no flag bit
>> - xfs_buf_get_noaddr(), which sets the _XBF_PAGES flag.
>> 
>> The only place that checks for _XBF_PAGES is xfs_buf_free().
>> 
>> Given that, I have two comments:
>> - You could just as easily have set the _XBF_PAGES flag in
>>   xfs_buf_associate_memory, thereby making that flag indicate
>>   consistently that the buffer has allocated pages
> 
> _XBF_PAGES is used to indicate that the buffer owns the
> pages and must free them when the buffer is freed.
> 
> _XBF_PAGE_CACHE indicates pages are allocated out of the buftarg
> page cache and need to be released when the buffer is freed.
> 
> In both cases, xfs_buf_free() has to free the pages
> associated with these buffers.
> 
> In the case of xfs_buf_associate_memory(), the memory attached to
> the buffer is owned by an external entity and hence we are not
> allowed to free it when we free the buffer. Therefore we can't set
> _XBF_PAGES on the buffer.
> 
>> - Or, since you are proposing unconditionally freeing the
>>   pages,
> 
> This change doesn't unconditionally free the pages attached
> to the buffer - it unconditionally frees the array used to
> index the pages on the buffer when the buffer is torn down.
> i.e:
> 
>    +------+         +------+
>    |  bp  |         |      |-------> page
>    |      | b_pages |      |-------> page
>    |      |-------->|      |.............
>    |      |         |      |-------> page
>    |      |         |      |-------> page
>    +------+         +------+
>                      ^^^^^^
> 		This array is what we are
> 		freeing unconditionally.
> 
> Cheers,
> 
> Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs