Re: [PATCH] remove_inode_page rewrite.

[Steve Dodd <steved@loth.demon.co.uk>]

On Tue, May 09, 2000 at 09:14:08PM +0100, Dave Jones wrote:

> I believe that while after CPU0 drops the pagecache_lock, and starts
> removing one page, CPU1 fails to lock the same page (as CPU0 grabbed it 
> with the trylock) and moves to the next page in the list, succeeds,
> removes it, and then rescans from the top.
> 
> With the current locking I believe it's then possible for CPU1 to
> lock that page

Which page? CPU1 should never find the page CPU0 is freeing because it will
either be locked, or not on the list at all. By the time CPU0 unlocks the
page, it's removed it from the list (and it grabs the spinlock while messing
with the list structure).

> (again in the TryLockPage(page) call) just before CPU0
> calls page_cache_release(page)
> 
> This patch probably kills us latency-wise, but looks a lot more
> sane in my eyes.

Now that invalidate_inode_page isn't calling sync_page, there seems to be
no reason to drop and retake the spinlock, I agree.

[..]
> - repeat:
> -	head = &inode->i_mapping->pages;
>  	spin_lock(&pagecache_lock);
> +
> +	head = &inode->i_mapping->pages;

That shouldn't be necessary - nobody is likely to change the address of
inode->i_mapping->pages under us :)
--
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.eu.org/Linux-MM/