xfs
[Top] [All Lists]

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

To: "Dave Chinner" <david@xxxxxxxxxxxxx>
Subject: RE: [PATCH] XFS: Free buffer pages array unconditionally
From: "Alex Elder" <aelder@xxxxxxx>
Date: Tue, 15 Dec 2009 13:21:36 -0600
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1260832317-14977-1-git-send-email-david@xxxxxxxxxxxxx>
Thread-index: Acp9FVn69UqjYVZpTKG2yJVGa1mZIAApX5vw
Thread-topic: [PATCH] XFS: Free buffer pages array unconditionally
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@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..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);
>  }

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