invalidate_inode_buffers call in invalidate_list?

invalidate_inode_buffers call in invalidate_list?

[Christoph Hellwig]

Does anyone remember why we call invalidate_inode_buffers in
invalidate_list?  These days we basically call invalidate_inode_buffers
only in ->evict_inode, which we will call from invalidate_list in
case of a zero refcount.

For calls to invalidate_inode_buffers it's hamrless, but for other
callers it means we lose track of which buffers were dirtied for
an inode, and thus can write them out during fsync.

Something like the untested patch below should fix it.  While we're
at it - any reaso not to consider I_NEW inodes as busy in
invalidate_list?


Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2010-10-17 16:33:34.826254213 -0700
+++ linux-2.6/fs/inode.c	2010-10-17 16:34:40.407003969 -0700
@@ -570,18 +570,16 @@ static int invalidate_list(struct super_
 			spin_unlock(&inode->i_lock);
 			continue;
 		}
-		invalidate_inode_buffers(inode);
-		if (!inode->i_ref) {
-			WARN_ON(inode->i_state & I_NEW);
-			inode->i_state |= I_FREEING;
+		if (inode->i_ref) {
 			spin_unlock(&inode->i_lock);
-
-			/* save a lock round trip by removing the inode here. */
-			list_move(&inode->i_sb_list, dispose);
+			busy = 1;
 			continue;
 		}
+		inode->i_state |= I_FREEING;
 		spin_unlock(&inode->i_lock);
-		busy = 1;
+
+		/* save a lock round trip by removing the inode here. */
+		list_move(&inode->i_sb_list, dispose);
 	}
 	return busy;
 }

Re: invalidate_inode_buffers call in invalidate_list?

[Dave Chinner]

On Sun, Oct 17, 2010 at 07:39:00PM -0400, Christoph Hellwig wrote:
> Does anyone remember why we call invalidate_inode_buffers in
> invalidate_list?  These days we basically call invalidate_inode_buffers
> only in ->evict_inode, which we will call from invalidate_list in
> case of a zero refcount.
> 
> For calls to invalidate_inode_buffers it's hamrless, but for other
> callers it means we lose track of which buffers were dirtied for
> an inode, and thus can write them out during fsync.
> 
> Something like the untested patch below should fix it.

Seems reasonable to me. I can't see any obvious reason why the call
in evict_inode in not sufficient to ensure the inode is correctly
cleaned up, and none of the inode buffers take a reference to the
inode so they shouldn't need to be invalidated to get the reference
count to zero....

> While we're
> at it - any reaso not to consider I_NEW inodes as busy in
> invalidate_list?

Not that I can think of. I'd probably leave the WARN_ON() condition
there, though, so that if we do ever encounter them we hear about
it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: invalidate_inode_buffers call in invalidate_list?

[Christoph Hellwig]

On Mon, Oct 18, 2010 at 12:11:50PM +1100, Dave Chinner wrote:
> > While we're
> > at it - any reaso not to consider I_NEW inodes as busy in
> > invalidate_list?
> 
> Not that I can think of. I'd probably leave the WARN_ON() condition
> there, though, so that if we do ever encounter them we hear about
> it...

The WARN_ON is already unreachable - we check for I_NEW a couple
of lines above and never drop it until after we reach the WARN_ON.