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>