[PATCH] fix inode state corruption (2.6.8-rc1-bk1)

[PATCH] fix inode state corruption (2.6.8-rc1-bk1)

[Miklos Szeredi]

Hi Andrew,

This patch fixes a hard-to-trigger condition, where the inode is on
the inode_in_use list while it's state is dirty.  In this state dirty
pages are not written back in sync() or from kupdate, only from direct
page reclaim.  And this causes a livelock in balance_dirty_pages after
a while.

Please apply!

The actual sequence of events required to get into this state is:

thread   function                             inode state         inode list
----------------------------------------------------------------------------
1 __sync_single_inode (background)            I_DIRTY             sb->s_io
1 do_writepages ...                           I_LOCKED
2 __writeback_single_inode (sync) sleeps      I_LOCKED
1 __sync_single_inode (background) finish     0                   inode_in_use
2 __writeback_single_inode (sync) wakeup      0
2 __sync_single_inode (sync)                  0  
2 do_writepages ...                           I_LOCKED
3 __mark_inode_dirty                          I_LOCKED | I_DIRTY
2 __sync_single_inode (sync) finish           I_DIRTY             left on
                                                                  inode_in_use

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

==============================================================================
--- linux-2.6.8-rc1-bk1/fs/fs-writeback.c.orig	2004-07-13 12:59:58.000000000 +0200
+++ linux-2.6.8-rc1-bk1/fs/fs-writeback.c	2004-07-13 14:31:07.000000000 +0200
@@ -213,8 +213,17 @@ __sync_single_inode(struct inode *inode,
 		} else if (inode->i_state & I_DIRTY) {
 			/*
 			 * Someone redirtied the inode while were writing back
-			 * the pages: nothing to do.
+			 * the pages.
 			 */
+			if (wait) {
+				/*
+				 * It is possible that this function is entered
+				 * with the inode on the in_use list, and it
+				 * is dirtied during being locked, in which
+				 * case it must be moved onto the dirty list.
+				 */
+				list_move(&inode->i_list, &sb->s_dirty);
+			}
 		} else if (atomic_read(&inode->i_count)) {
 			/*
 			 * The inode is clean, inuse

Re: [PATCH] fix inode state corruption (2.6.8-rc1-bk1)

[Andrew Morton]

Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> This patch fixes a hard-to-trigger condition, where the inode is on
>  the inode_in_use list while it's state is dirty.  In this state dirty
>  pages are not written back in sync() or from kupdate, only from direct
>  page reclaim.  And this causes a livelock in balance_dirty_pages after
>  a while.

How ghastly.

Why did you make the list movement conditional on non-zero `wait'?

It would be equivalent to remove these lines from __mark_inode_dirty():

		/*
		 * If the inode is locked, just update its dirty state. 
		 * The unlocker will place the inode on the appropriate
		 * superblock list, based upon its state.
		 */
		if (inode->i_state & I_LOCK)
			goto out;

but probably not so good, because that could cause other tasks to come
around and wait on this inode while it is under writeout instead of writing
back different inodes.

Re: [PATCH] fix inode state corruption (2.6.8-rc1-bk1)

[Miklos Szeredi]

Andrew Morton <akpm@osdl.org> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > This patch fixes a hard-to-trigger condition, where the inode is on
> >  the inode_in_use list while it's state is dirty.  In this state dirty
> >  pages are not written back in sync() or from kupdate, only from direct
> >  page reclaim.  And this causes a livelock in balance_dirty_pages after
> >  a while.
> 
> How ghastly.
> 
> Why did you make the list movement conditional on non-zero `wait'?

Because, I think this particular case can only happen in sync
writeback.  Otherwise the inode_lock is not released in
__writeback_single_inode so the inode must be on the s_io list.  I
don't see wheter it makes any difference performance-wise whether the
inode is left on s_io or unconditionally moved to s_dirty, but this is
the smaller change.

Miklos