Re: Unnecessary barrier in sync_page()?

[Andrew Morton <akpm@osdl.org>]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> On Wed, Jul 07, 2004 at 04:57:04PM -0400, Chris Mason wrote:
> > I wasn't worried about the locked bit when I added the barrier, my goal
> > was to order things with people that set page->mapping to null.
> 
> page->mapping cannot change from NULL to non-NULL there.
> 
> it can only change from non-NULL to NULL, and there's no way to
> serialize with the truncate without taking the page lock.

And we cannot lock the page because, err, we need to run sync_page() for
that.

> The one extremely important fix you did around the same time, has been
> to "cache" the value of "mapping" in the kernel stack, so that it
> remains the same during the while function (so that it cannot start
> non-NULL an finish NULL).

But the page can come unlocked and truncate or page reclaim can remove the
page from the mapping and memory reclaim can reclaim the inode:

	int block_sync_page(struct page *page)
	{
		struct address_space *mapping;

		smp_mb();
		mapping = page_mapping(page);
-> right here
		if (mapping)
-> go boom here		blk_run_backing_dev(mapping->backing_dev_info, page);
		return 0;
	}

But I cannot think of any callers of sync_page() who don't have a ref on
the inode, so...