[PATCH] __wait_on_freeing_inode fix

[PATCH] __wait_on_freeing_inode fix

[Miklos Szeredi]

Andrew,

This patch fixes queer behavior in __wait_on_freeing_inode().

If I_LOCK was not set it called yield(), effectively busy waiting for
the removal of the inode from the hash.  This change was introduced
within "[PATCH] eliminate inode waitqueue hashtable" Changeset
1.1938.166.16 last october by wli.

The solution is to restore the old behavior, of unconditionally
waiting on the waitqueue.  It doesn't matter if I_LOCK is not set
initally, the task will go to sleep, and wake up when wake_up_inode()
is called from generic_delete_inode() after removing the inode from
the hash chain.

Comment is also updated to better reflect current behavior.

Compile tested only.  This condition is very hard to trigger normally
(simultaneous clear_inode() with iget()) so probably only heavy stress
testing can reveal any change of behavior.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

--- inode.c~	2005-05-02 11:24:49.000000000 +0200
+++ inode.c	2005-05-06 13:25:12.000000000 +0200
@@ -1253,29 +1253,21 @@
 }
 
 /*
- * If we try to find an inode in the inode hash while it is being deleted, we
- * have to wait until the filesystem completes its deletion before reporting
- * that it isn't found.  This is because iget will immediately call
- * ->read_inode, and we want to be sure that evidence of the deletion is found
- * by ->read_inode.
+ * If we try to find an inode in the inode hash while it is being
+ * deleted, we have to wait until the filesystem completes its
+ * deletion before reporting that it isn't found.  This function waits
+ * until the deletion _might_ have completed.  Callers are responsible
+ * to recheck inode state.
+ * 
+ * It doesn't matter if I_LOCK is not set initially, a call to
+ * wake_up_inode() after removing from the hash list will DTRT.
+ *
  * This is called with inode_lock held.
  */
 static void __wait_on_freeing_inode(struct inode *inode)
 {
 	wait_queue_head_t *wq;
 	DEFINE_WAIT_BIT(wait, &inode->i_state, __I_LOCK);
-
-	/*
-	 * I_FREEING and I_CLEAR are cleared in process context under
-	 * inode_lock, so we have to give the tasks who would clear them
-	 * a chance to run and acquire inode_lock.
-	 */
-	if (!(inode->i_state & I_LOCK)) {
-		spin_unlock(&inode_lock);
-		yield();
-		spin_lock(&inode_lock);
-		return;
-	}
 	wq = bit_waitqueue(&inode->i_state, __I_LOCK);
 	prepare_to_wait(wq, &wait.wait, TASK_UNINTERRUPTIBLE);
 	spin_unlock(&inode_lock);

Re: [PATCH] __wait_on_freeing_inode fix

[Artem B. Bityuckiy]

В Птн, 06/05/2005 в 13:46 +0200, Miklos Szeredi пишет:
> This patch fixes queer behavior in __wait_on_freeing_inode().
Although the patch looks sane & simple, I gonna test your patch today.
I'll report the results. 

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH] __wait_on_freeing_inode fix

[Miklos Szeredi]

> > This patch fixes queer behavior in __wait_on_freeing_inode().
> Although the patch looks sane & simple, I gonna test your patch today.
> I'll report the results. 

Thanks!

Miklos

Re: [PATCH] __wait_on_freeing_inode fix

[David Woodhouse]

On Fri, 2005-05-06 at 13:46 +0200, Miklos Szeredi wrote:
> The solution is to restore the old behavior, of unconditionally
> waiting on the waitqueue.  It doesn't matter if I_LOCK is not set
> initally, the task will go to sleep, and wake up when wake_up_inode()
> is called from generic_delete_inode() after removing the inode from
> the hash chain.

That's all well and good if it's actually generic_delete_inode() which
removes the inode from the hash chain. But if it's prune_icache() which
does that, you don't get the wakeup.

Applying Artem's patch will fix that.

-- 
dwmw2

Re: [PATCH] __wait_on_freeing_inode fix

[Miklos Szeredi]

> That's all well and good if it's actually generic_delete_inode() which
> removes the inode from the hash chain. But if it's prune_icache() which
> does that, you don't get the wakeup.
> 
> Applying Artem's patch will fix that.

I think it should work without Artem's patch too, since prune_icache()
removes the inode from the hash chain at the same time (under
inode_lock) as changing it's state to I_FREEING.  So the pruned inode
will never be seen by iget().

Miklos

Re: [PATCH] __wait_on_freeing_inode fix

[Artem B. Bityuckiy]

> I think it should work without Artem's patch too, since prune_icache()
> removes the inode from the hash chain at the same time (under
> inode_lock) as changing it's state to I_FREEING.  So the pruned inode
> will never be seen by iget().
> 
I suppose this doesn't mean that your patch fixes my problem (it mustn't
I believe) ?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH] __wait_on_freeing_inode fix

[Miklos Szeredi]

> > I think it should work without Artem's patch too, since prune_icache()
> > removes the inode from the hash chain at the same time (under
> > inode_lock) as changing it's state to I_FREEING.  So the pruned inode
> > will never be seen by iget().
> > 
> I suppose this doesn't mean that your patch fixes my problem (it mustn't
> I believe) ?

You are right.  They are completely orthogonal, except the fact that
with your patch, the iget() clear_inode() collision will be more
frequent, and thus the yield() hack would trigger more often.

Miklos

Re: [PATCH] __wait_on_freeing_inode fix

[David Woodhouse]

On Fri, 2005-05-06 at 15:38 +0200, Miklos Szeredi wrote:
> I think it should work without Artem's patch too, since prune_icache()
> removes the inode from the hash chain at the same time (under
> inode_lock) as changing it's state to I_FREEING.  So the pruned inode
> will never be seen by iget().

Doh. Yes, of course -- it had temporarily escaped my notice that the
whole _point_ of Artem's patch is that prune_icache() is currently
broken in precisely the way you describe.

-- 
dwmw2

Re: [PATCH] __wait_on_freeing_inode fix

[Artem B. Bityuckiy]

> Compile tested only.  This condition is very hard to trigger normally
> (simultaneous clear_inode() with iget()) so probably only heavy stress
> testing can reveal any change of behavior.

Well, my stress test works fine with your patch. I've tested it on JFFS2
FS which works on top of RAM-emulated flash. I tried it together with my
patch since otherwise the stress test crashes due to the race that my
patch fixes.

On vanilla linux-2.6.11.5 the stress test usually crashes in about 5
minutes, but linux-2.6.11.5 + the 2 patches (as well as + only one my
patch) it is stable for 2 hours already.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH] __wait_on_freeing_inode fix

[William Lee Irwin III]

On Fri, May 06, 2005 at 01:46:38PM +0200, Miklos Szeredi wrote:
> If I_LOCK was not set it called yield(), effectively busy waiting for
> the removal of the inode from the hash.  This change was introduced
> within "[PATCH] eliminate inode waitqueue hashtable" Changeset
> 1.1938.166.16 last october by wli.
> The solution is to restore the old behavior, of unconditionally
> waiting on the waitqueue.  It doesn't matter if I_LOCK is not set
> initally, the task will go to sleep, and wake up when wake_up_inode()
> is called from generic_delete_inode() after removing the inode from
> the hash chain.

I was trying to preserve some (possibly misinterpreted) behavior but I
can't remember what anymore. Anyway, since it's misbehaving, nuke it.


-- wli