Re: [PATCH] tmpfs: fix race between umount and writepage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Andrew Morton wrote:
On Tue, 5 Apr 2011 14:34:52 +0400
Konstantin Khlebnikov<khlebnikov@xxxxxxxxxx>  wrote:

shmem_writepage() call igrab() on the inode for the page which is came from
reclaimer to add it later into shmem_swaplist for swap-unuse operation.

This igrab() can race with super-block deactivating process:

shrink_inactive_list()		deactivate_super()
pageout()			tmpfs_fs_type->kill_sb()
shmem_writepage()		kill_litter_super()
				generic_shutdown_super()
				 evict_inodes()
  igrab()
				  atomic_read(&inode->i_count)
				   skip-inode
  iput()
				 if (!list_empty(&sb->s_inodes))
					printk("VFS: Busy inodes after...

Generally, ->writepage implementations shouldn't play with the inode,
for the reasons you've discovered.  A more common race is
writepage-versus-reclaim, where writepage is playing with the inode
when a concurrent reclaim frees the inode (and hence the
address_space).

It is safe to play with the inode while the passed-in page is locked
because nobody will free an inode which has an attached locked page.
But once the page is unlocked, nothing pins the inode.  Typically,
tmpfs goes and breakes this rule.


Question is: why is shmem_writepage() doing the igrab/iput?

Read 1b1b32f2c6f6bb3253 and weep.

That changelog is a little incorrect:

: Ah, I'd never suspected it, but shmem_writepage's swaplist manipulation
: is unsafe: though still hold page lock, which would hold off inode
: deletion if the page were i pagecache, it doesn't hold off once it's in
: swapcache (free_swap_and_cache doesn't wait on locked pages).  Hmm: we
: could put the the inode on swaplist earlier, but then shmem_unuse_inode
: could never prune unswapped inodes.

We don't actually hold the page lock when altering the swaplist:
swap_writepage() unlocks the page.  Doesn't seem to matter.


I think we should get the igrab/iput out of there and come up with a
different way of pinning the inode in ->writepage().

Can we do it in this order?

	mutex_lock(&shmem_swaplist_mutex);
	list_move_tail(&info->swaplist,&shmem_swaplist);
	delete_from_page_cache(page);
	shmem_swp_set(info, entry, swap.val);
	shmem_swp_unmap(entry);
	mutex_unlock(&shmem_swaplist_mutex);
	swap_writepage(page, wbc);									


Yes, we can, but of course without locking shmem_swaplist_mutex if inode already in shmem_swaplist.

I saw that igrab redundancy, but I was confused with lock-nesting and
shmem_swaplist spinlock to mutex conversion.
Seems to shmem_swaplist_mutex is already nested inside PageLock, so all ok.

We can simply revert last hunk from that commit, patch follows.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]