On Wed, 19 Sep 2001, Andrea Arcangeli wrote:
> On Wed, Sep 19, 2001 at 08:42:39PM +0100, Hugh Dickins wrote:
> > --- 2.4.10-pre12/mm/swap_state.c Wed Sep 19 14:05:54 2001
> > +++ linux/mm/swap_state.c Mon Sep 17 06:30:26 2001
> > @@ -23,6 +23,17 @@
> > */
> > static int swap_writepage(struct page *page)
> > {
> > + /* One for the page cache, one for this user, one for page->buffers */
> > + if (page_count(page) > 2 + !!page->buffers)
>
> this is racy, you have to spin_lock(&pagecache_lock) before you can
> expect the page_count() stays constant. then after you checked the page
> has count == 1, you must atomically drop it from the pagecache so it's
> not visible anymore to the swapin lookups.
Locking on pagecache_lock is no way to stabilize page count, but this
doesn't need it stabilized: it's just checking there's nothing but us
which can be interested in the page. Okay, we now know that
read_swap_cache_async can get "interested" in surprising pages (when
the swap entry it asks for has meanwhile been replaced), but I don't
see how that endangers the logic here - it could briefly bump count
too high to pass the "don't write" test, but that errs on the safe side.
If you think this racy, how come you still allow "exclusive_swap_page"
use elsewhere? Actually, I think these tests would be better replaced
by use of "exclusive_swap_page", wouldn't they? But perhaps I'll find
I'm off-by-one when I try it tomorrow.
> Another way to fix the race is to change lookup_swap_cache to do
> find_lock_page instead of find_get_page, and then check the page is
> still a swapcachepage after you got it locked (that was the old way,
> somebody changed it and introduced the race, I like lookup_swap_cache to
> use find_get_page so I dropped such check to fix it, it was a minor
> optimization but yes probably worthwhile to reintroduce after addressing
> this race in one of the two ways described).
Well, I certainly agree the system can survive without this optimization,
and I wouldn't want to restore it if it were buggy. I just don't see
the scenario you're afraid of, and nobody had questioned it before.
> It is also buggy, if something it should be "page_count(page) != 1" (not
> != 2).
I don't think so: as the comment says, one for the page cache,
one for the caller of writepage, one (perhaps) for page->buffers.
Hugh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Sep 23 2001 - 21:00:34 EST