Re: page->mapping == 0

From: Linus Torvalds (torvalds@transmeta.com)
Date: Sun Oct 29 2000 - 14:12:15 EST


On Sun, 29 Oct 2000, Alexander Viro wrote:
>
> One possible way is to access page->mapping _only_ under the page lock
> and in cases when we call ->i_mapping->a_ops->foo check the ->mapping
> before the method call.

I'm leaning towards this for a 2.4.x solution.

As far as I can tell, page->mapping is _already_ only accessed and
modified with the page lock held. It's just that we don't test it for
NULL, in case an earlier lock holder decided to clear it.

(No, I didn't look through all the users, but at least conceptually it
_should_ be true that we only look at "mapping" with the lock held: it's
mainly used for pagein, and pageout, buth with the lock held for other
reasons already. Certainly all the places where we have had bug-reports
have been of this type).

Making it policy that we have to re-test page->mapping after aquireing the
page lock might be the simplest fix for 2.4.x. It still means that we
might end up allowing people to have a "bad" page in the VM space due to
the "test->insert" race condition, but it woul dmake that event pretty
much a harmless bug (and thus move it to the "beauty wart - to be fixed
later" category).

And the places where we get the page lock and use page->mapping are not
that many, I think.

(And notice how we actually _have_ this approach already in
do_buffer_fdatasync(), for similar reasons - we use the "re-test the
page->buffers" thing there. Of course, there we do it because the clearing
of page->buffers is easier to see, and can happen as a result of memory
pressure, and not just truncate()).

So, for example, in __find_lock_page() we should re-test the mapping after
we aquired the page lock. Which is fairly easy, just add something like

        /* Race: did the mapping go away before we got the page lock? */
        if (page && page->mapping != mapping) {
                page_cache_release(page);
                goto repeat;
        }

to the end of __find_lock_page(). Add similar logic to
do_generic_file_read(), filemap_nopage() and filemap_sync_pte() and
read_cache_page(), and you're pretty much done.

                Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Tue Oct 31 2000 - 21:00:25 EST