Andrew Morton <akpm@xxxxxxxx> wrote: > > SetPageLocked(page); > > if (TestSetPagLocked(page)) > BUG(); > > would make me more comfortable.. That shouldn't be necessary if add_to_page_cache() also doesn't do that, but if you wish, I can do that - it's the error handling path, so it doesn't matter too much performancewise. > > > For the second hunk, is it not possible to do this cleanup in the callback > > > function? > > > > Which callback function? > > I was referring to the filler_t thingy. Is it not possible to get control > of that? Well, the filler_t thing is generally a_ops->readpage from the caller fs, but we don't want to call that if add_to_page_cache() failed, and we don't want to call it if we're just discarding a bunch of pages we've now no intention of actually reading. I suppose we could add another callback for ditching pages we don't want to keep. This has the potential to be called quite a lot because of the way readahead works on Linux. > So please, can we have some comments in there which describe the new > behaviour in a manner sufficient for a maintainer to follow so people don't > break your stuff? Okay... I'll add more comments. I should probably also extend the documentation on releasepage(). It won't be till Monday though. > > Out of interest, why do we need PG_private to say there's something in > > page->private? Can't it just be assumed either that if page->private is > > non-zero or that if a_ops->releasepage() is non-NULL, then we need to > > "release" the page? > > page->private is an unsigned long, not a pointer. The core kernel hence > cannot determine from its value whether or not it is live. For example, the > fs might choose to treat it as a bitmap of which-blocks-are-uptodate and > which-blocks-are-dirty. Then the second option is still possible (calling releasepage() unconditionally). David
