xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] [XFS] Free buffer pages array unconditionally
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 3 Dec 2009 09:22:28 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20091202151742.GA1263@xxxxxxxxxxxxx>
References: <1259734333-20581-1-git-send-email-david@xxxxxxxxxxxxx> <20091202151742.GA1263@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>