find_get_swapcache_page() question

find_get_swapcache_page() question

[Marcelo Tosatti]

Ingo,

Looking at find_get_swapcache_page(), I can't see _how_ we can find a
page on the swapper pagecache table that is not a swapcache page.

How that can happen ?

        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);

Re: find_get_swapcache_page() question

[Hugh Dickins]

On Mon, 27 Aug 2001, Marcelo Tosatti wrote:
> 
> Looking at find_get_swapcache_page(), I can't see _how_ we can find a
> page on the swapper pagecache table that is not a swapcache page.

I had a look at this a couple of weeks back.  If I remember rightly,
the page found by __find_page_nolock() cannot be other than a swapcache
page, and will always have PageSwapCache bit set... until pagecache_lock
is dropped on return to its only caller lookup_swap_cache().  In which the 
		if (!PageSwapCache(found))
			BUG();
		if (found->mapping != &swapper_space)
			BUG();
are not safe, since there may a concurrent remove_from_swap_cache(),
either from try_to_unuse() or from Rik's new vm_swap_full() deletion.
Those tests would be safe if the page were locked, but it's not.

I say find_get_swapcache_page() serves no purpose, should be deleted,
and find_get_page() used instead.  That was one of various things in
the swapoff patch I posted to linux-mm on 16 Aug, which I need to
finish off, cut into pieces and submit to Linus.

Hugh

Re: find_get_swapcache_page() question

[Rik van Riel]

On Tue, 28 Aug 2001, Hugh Dickins wrote:

> 		if (!PageSwapCache(found))
> 			BUG();
> 		if (found->mapping != &swapper_space)
> 			BUG();
> are not safe, since there may a concurrent remove_from_swap_cache(),
> either from try_to_unuse() or from Rik's new vm_swap_full() deletion.
> Those tests would be safe if the page were locked, but it's not.

vm_swap_full() is called with the page locked,
remove_from_swap_cache() also seems to want the
page locked...

Rik
-- 
IA64: a worthy successor to i860.

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

Send all your spam to aardvark@nl.linux.org (spam digging piggy)

Re: find_get_swapcache_page() question

[Hugh Dickins]

On Tue, 28 Aug 2001, Rik van Riel wrote:
> On Tue, 28 Aug 2001, Hugh Dickins wrote:
> 
> > 		if (!PageSwapCache(found))
> > 			BUG();
> > 		if (found->mapping != &swapper_space)
> > 			BUG();
> > are not safe, since there may a concurrent remove_from_swap_cache(),
> > either from try_to_unuse() or from Rik's new vm_swap_full() deletion.
> > Those tests would be safe if the page were locked, but it's not.
> 
> vm_swap_full() is called with the page locked,
> remove_from_swap_cache() also seems to want the
> page locked...

Certainly (I seem to recall composing a comment to that effect
recently :-)!  Sorry for being unclear, I meant that those tests
in lookup_swap_cache() are made without the page being locked
by lookup_swap_cache() or its caller (and I'm not proposing
that the page should be locked, but the unsafe tests removed).

Hugh