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@xxxxxxxxxxxxx
|