[PATCH] umount versus iprune

[PATCH] umount versus iprune

[Hugh Dickins]

When prune_icache coincides with unmounting, invalidate_inodes notices
the inode it's working on as busy but doesn't wait: Self-destruct in 5
seconds message, and later iput oopses on freed super_block.

Neither end is a fast path, so patch below just adds iprune_sem for
exclusion: if you've got a neater solution, great - I shied away from
games with i_state, and extending use of shrinker_sem felt like abuse.

Hugh

--- 2.5.62/fs/inode.c	Mon Feb 10 20:12:50 2003
+++ linux/fs/inode.c	Thu Feb 20 13:01:58 2003
@@ -81,6 +81,11 @@
 spinlock_t inode_lock = SPIN_LOCK_UNLOCKED;
 
 /*
+ * A semaphore to delay invalidate_inodes while prune_icache is busy.
+ */
+static DECLARE_MUTEX(iprune_sem);
+
+/*
  * Statistics gathering..
  */
 struct inodes_stat_t inodes_stat;
@@ -320,12 +325,14 @@
 	int busy;
 	LIST_HEAD(throw_away);
 
+	down(&iprune_sem);
 	spin_lock(&inode_lock);
 	busy = invalidate_list(&inode_in_use, sb, &throw_away);
 	busy |= invalidate_list(&inode_unused, sb, &throw_away);
 	busy |= invalidate_list(&sb->s_dirty, sb, &throw_away);
 	busy |= invalidate_list(&sb->s_io, sb, &throw_away);
 	spin_unlock(&inode_lock);
+	up(&iprune_sem);
 
 	dispose_list(&throw_away);
 
@@ -395,6 +402,7 @@
 	int nr_scanned;
 	unsigned long reap = 0;
 
+	down(&iprune_sem);
 	spin_lock(&inode_lock);
 	for (nr_scanned = 0; nr_scanned < nr_to_scan; nr_scanned++) {
 		struct inode *inode;
@@ -429,6 +437,7 @@
 	}
 	inodes_stat.nr_unused -= nr_pruned;
 	spin_unlock(&inode_lock);
+	up(&iprune_sem);
 	dispose_list(&freeable);
 	if (current_is_kswapd)
 		mod_page_state(kswapd_inodesteal, reap);

Re: [PATCH] umount versus iprune

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> When prune_icache coincides with unmounting, invalidate_inodes notices
> the inode it's working on as busy but doesn't wait: Self-destruct in 5
> seconds message, and later iput oopses on freed super_block.
> 
> Neither end is a fast path, so patch below just adds iprune_sem for
> exclusion: if you've got a neater solution, great - I shied away from
> games with i_state, and extending use of shrinker_sem felt like abuse.
> 

Sounds reasonable.  The semaphore will block kswapd, but the reason kswapd
is being blocked is so that someone else can free tons of memory, so shrug.

Is 2.4 affected?

Re: [PATCH] umount versus iprune

[Hugh Dickins]

On Thu, 20 Feb 2003, Andrew Morton wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> >
> > When prune_icache coincides with unmounting, invalidate_inodes notices
> > the inode it's working on as busy but doesn't wait: Self-destruct in 5
> > seconds message, and later iput oopses on freed super_block.
> 
> Is 2.4 affected?

Good question.

I had thought obviously not, 2.4 prune_icache doesn't __iget and
drop inode_lock as 2.5 does (while invalidating buffers and pages).
Looks like 2.4 leaves that work to the subsequent dispose_list's
truncate_inode_pages.

But that raises the doubt: maybe 2.4 won't get any Self-destruct
message, but when prune_icache calls dispose_list calls clear_inode
and destroy_inode, there could be a reference to freed super_block?

Perhaps there's some other reason why not,
I'll check it out more carefully later.

Hugh

Re: [PATCH] umount versus iprune

[Hugh Dickins]

On Thu, 20 Feb 2003, Hugh Dickins wrote:
> 
> But that raises the doubt: maybe 2.4 won't get any Self-destruct
> message, but when prune_icache calls dispose_list calls clear_inode
> and destroy_inode, there could be a reference to freed super_block?

Which triggers the realization that my original patch was wrong for
this very reason: 2.5 prune_icache must hold iprune_sem until _after_
its dispose_list, and invalidate_inodes might as well do the same:

--- 2.5.62/fs/inode.c	Mon Feb 10 20:12:50 2003
+++ linux/fs/inode.c	Thu Feb 20 20:13:30 2003
@@ -81,6 +81,11 @@
 spinlock_t inode_lock = SPIN_LOCK_UNLOCKED;
 
 /*
+ * A semaphore to delay invalidate_inodes while prune_icache is busy.
+ */
+static DECLARE_MUTEX(iprune_sem);
+
+/*
  * Statistics gathering..
  */
 struct inodes_stat_t inodes_stat;
@@ -320,6 +325,7 @@
 	int busy;
 	LIST_HEAD(throw_away);
 
+	down(&iprune_sem);
 	spin_lock(&inode_lock);
 	busy = invalidate_list(&inode_in_use, sb, &throw_away);
 	busy |= invalidate_list(&inode_unused, sb, &throw_away);
@@ -328,6 +334,7 @@
 	spin_unlock(&inode_lock);
 
 	dispose_list(&throw_away);
+	up(&iprune_sem);
 
 	return busy;
 }
@@ -395,6 +402,7 @@
 	int nr_scanned;
 	unsigned long reap = 0;
 
+	down(&iprune_sem);
 	spin_lock(&inode_lock);
 	for (nr_scanned = 0; nr_scanned < nr_to_scan; nr_scanned++) {
 		struct inode *inode;
@@ -429,7 +437,10 @@
 	}
 	inodes_stat.nr_unused -= nr_pruned;
 	spin_unlock(&inode_lock);
+
 	dispose_list(&freeable);
+	up(&iprune_sem);
+
 	if (current_is_kswapd)
 		mod_page_state(kswapd_inodesteal, reap);
 	else