strange locking __find_get_swapcache_page()

strange locking __find_get_swapcache_page()

[Rik van Riel]

Hi,

I've encountered a suspicious piece of code in filemap.c:

struct page * __find_get_swapcache_page( ... )
...
        /*
         * We need the LRU lock to protect against page_launder().
         */

        spin_lock(&pagecache_lock);
        page = __find_page_nolock(mapping, offset, *hash);
        if (page) {
                spin_lock(&pagemap_lru_lock);
                if (PageSwapCache(page))
                        page_cache_get(page);
                else
                        page = NULL;
                spin_unlock(&pagemap_lru_lock);
        }
        spin_unlock(&pagecache_lock);


Question is ... WHY do we need the pagemap_lru_lock ?

Page_launder() never removes the page from the swap
cache, that is only done by reclaim_page(), and done
while holding the pagecache_lock.

The other places where pages are removed from the
swap cache (tmpfs and free_page_and_swap_cache)
also hold the pagecache_lock.

Taking the pagemap_lru_lock seems unneeded to me...

regards,

Rik
--
Executive summary of a recent Microsoft press release:
   "we are concerned about the GNU General Public License (GPL)"


		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com/


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: strange locking __find_get_swapcache_page()

[Linus Torvalds]

On Mon, 30 Jul 2001, Rik van Riel wrote:
>
> I've encountered a suspicious piece of code in filemap.c:
>
> struct page * __find_get_swapcache_page( ... )

Hmm. I thin the whole PageSwapCache() test is bogus - if we found it on
the swapper_space address space, then the page had better be a swap-cache
page, and testing for it explicitly is silly.

Also, it appears that the only caller of this is
find_get_swapcache_page(), which in itself really doesn't even care: it
just uses the lookup as a boolen on whether to add a new page to the swap
cache, and does even _that_ completely wrong. There's a big race there,
see if you can spot it.

The fix, I suspect, is to pretty much get rid of the code altogether, and
make it use add_to_page_cache_unique() or whatever it is called that gets
the duplicate check _right_.

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: strange locking __find_get_swapcache_page()

[Rik van Riel]

On Mon, 30 Jul 2001, Linus Torvalds wrote:
> On Mon, 30 Jul 2001, Rik van Riel wrote:
> >
> > I've encountered a suspicious piece of code in filemap.c:
> >
> > struct page * __find_get_swapcache_page( ... )
>
> Hmm. I thin the whole PageSwapCache() test is bogus - if we
> found it on the swapper_space address space, then the page had
> better be a swap-cache page, and testing for it explicitly is
> silly.

*nod*

> Also, it appears that the only caller of this is
> find_get_swapcache_page(), which in itself really doesn't even

It's even simpler than that.  The only user is lookup_swap_cache(),
which is used only to see if a page is present in the swap cache
or not ...

regards,

Rik
--
Executive summary of a recent Microsoft press release:
   "we are concerned about the GNU General Public License (GPL)"


		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: strange locking __find_get_swapcache_page()

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Mon, 30 Jul 2001, Rik van Riel wrote:
> >
> > I've encountered a suspicious piece of code in filemap.c:
> >
> > struct page * __find_get_swapcache_page( ... )
> 
> Hmm. I thin the whole PageSwapCache() test is bogus - if we found it on
> the swapper_space address space, then the page had better be a swap-cache
> page, and testing for it explicitly is silly.
> 
> Also, it appears that the only caller of this is
> find_get_swapcache_page(), which in itself really doesn't even care: it
> just uses the lookup as a boolen on whether to add a new page to the swap
> cache, and does even _that_ completely wrong. There's a big race there,
> see if you can spot it.

read_swap_cache_async()?  All code paths in that area are
under lock_kernel().

> The fix, I suspect, is to pretty much get rid of the code altogether, and
> make it use add_to_page_cache_unique() or whatever it is called that gets
> the duplicate check _right_.

The whole lot needed spring cleaning 1-2 years ago, but I see no
bugs in there.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: strange locking __find_get_swapcache_page()

[Linus Torvalds]

On Tue, 31 Jul 2001, Andrew Morton wrote:
>
> read_swap_cache_async()?  All code paths in that area are
> under lock_kernel().

Ugh. But yes, that would certainly fix the race.

Let's leave it, and fix it for real in 2.5.x

		Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/