[PATCH 1/4] fs: do not drop inode_lock in dispose_list

[PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Christoph Hellwig]

Despite the comment above it we can not safely drop the lock here.
invalidate_list is called from many other places that just umount.
Also switch to proper list macros now that we never drop the lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2010-10-24 13:12:18.873003757 +0200
+++ linux-2.6/fs/inode.c	2010-10-24 14:35:29.924253651 +0200
@@ -482,26 +482,10 @@ static void dispose_list(struct list_hea
  */
 static int invalidate_list(struct list_head *head, struct list_head *dispose)
 {
-	struct list_head *next;
+	struct inode *inode, *next;
 	int busy = 0;
 
-	next = head->next;
-	for (;;) {
-		struct list_head *tmp = next;
-		struct inode *inode;
-
-		/*
-		 * We can reschedule here without worrying about the list's
-		 * consistency because the per-sb list of inodes must not
-		 * change during umount anymore, and because iprune_sem keeps
-		 * shrink_icache_memory() away.
-		 */
-		cond_resched_lock(&inode_lock);
-
-		next = next->next;
-		if (tmp == head)
-			break;
-		inode = list_entry(tmp, struct inode, i_sb_list);
+	list_for_each_entry_safe(inode, next, head, i_sb_list) {
 		if (inode->i_state & I_NEW)
 			continue;
 		if (atomic_read(&inode->i_count)) {

[PATCH 2/4] fs: fold invalidate_list into invalidate_inodes

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2010-10-24 16:31:49.000000000 +0200
+++ linux-2.6/fs/inode.c	2010-10-24 16:32:20.609254420 +0200
@@ -477,15 +477,24 @@ static void dispose_list(struct list_hea
 	}
 }
 
-/*
- * Invalidate all inodes for a device.
+/**
+ * invalidate_inodes	- attempt to free all inodes on a superblock
+ * @sb:		superblock to operate on
+ *
+ * Attempts to free all inodes for a given superblock.  If there were any
+ * busy inodes return a non-zero value, else zero.
  */
-static int invalidate_list(struct list_head *head, struct list_head *dispose)
+int invalidate_inodes(struct super_block *sb)
 {
-	struct inode *inode, *next;
 	int busy = 0;
+	struct inode *inode, *next;
+	LIST_HEAD(dispose);
 
-	list_for_each_entry_safe(inode, next, head, i_sb_list) {
+	down_write(&iprune_sem);
+
+	spin_lock(&inode_lock);
+	fsnotify_unmount_inodes(&sb->s_inodes);
+	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
 		if (inode->i_state & I_NEW)
 			continue;
 		if (atomic_read(&inode->i_count)) {
@@ -499,34 +508,14 @@ static int invalidate_list(struct list_h
 		 * Move the inode off the IO lists and LRU once I_FREEING is
 		 * set so that it won't get moved back on there if it is dirty.
 		 */
-		list_move(&inode->i_lru, dispose);
+		list_move(&inode->i_lru, &dispose);
 		list_del_init(&inode->i_wb_list);
 		if (!(inode->i_state & (I_DIRTY | I_SYNC)))
 			percpu_counter_dec(&nr_inodes_unused);
 	}
-	return busy;
-}
-
-/**
- *	invalidate_inodes	- discard the inodes on a device
- *	@sb: superblock
- *
- *	Discard all of the inodes for a given superblock. If the discard
- *	fails because there are busy inodes then a non zero value is returned.
- *	If the discard is successful all the inodes have been discarded.
- */
-int invalidate_inodes(struct super_block *sb)
-{
-	int busy;
-	LIST_HEAD(throw_away);
-
-	down_write(&iprune_sem);
-	spin_lock(&inode_lock);
-	fsnotify_unmount_inodes(&sb->s_inodes);
-	busy = invalidate_list(&sb->s_inodes, &throw_away);
 	spin_unlock(&inode_lock);
 
-	dispose_list(&throw_away);
+	dispose_list(&dispose);
 	up_write(&iprune_sem);
 
 	return busy;

Re: [PATCH 2/4] fs: fold invalidate_list into invalidate_inodes

[Christian Stroetmann]

  Aloha;

Just only a typo, maybe

On the 24.10.2010 19:40, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c	2010-10-24 16:31:49.000000000 +0200
> +++ linux-2.6/fs/inode.c	2010-10-24 16:32:20.609254420 +0200
> @@ -477,15 +477,24 @@ static void dispose_list(struct list_hea
>   	}
>   }
>
> -/*
> - * Invalidate all inodes for a device.
> +/**
> + * invalidate_inodes	- attempt to free all inodes on a superblock
> + * @sb:		superblock to operate on
> + *
> + * Attempts to free all inodes for a given superblock.  If there were any
> + * busy inodes return a non-zero value, else zero.
maybe:
, then return a

[...]

Have fun
Christian Stroetmann

[PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes

[Christoph Hellwig]

Skip I_FREEING inodes just like I_WILL_FREE and I_NEW when walking the
writeback lists.  Currenly this can't happen, but once we move from
inode_lock to more fine grained locking we can have an inode that's
still on the writeback lists but has I_FREEING set, and we absolutely
need to skip it here, just like we do for all other inode list walks.

Based on a patch from Dave Chinner.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c	2010-10-24 13:21:18.282010741 +0200
+++ linux-2.6/fs/fs-writeback.c	2010-10-24 13:21:35.456004735 +0200
@@ -487,10 +487,16 @@ static int writeback_sb_inodes(struct su
 			return 0;
 		}
 
-		if (inode->i_state & (I_NEW | I_WILL_FREE)) {
+		/*
+		 * Don't bother with new inodes or inodes beeing freed, first
+		 * kind does not need peridic writeout yet, and for the latter
+		 * kind writeout is handled by the freer.
+		 */
+		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
 			requeue_io(inode);
 			continue;
 		}
+
 		/*
 		 * Was this inode dirtied after sync_sb_inodes was called?
 		 * This keeps sync from extra jobs and livelock.
@@ -498,7 +504,6 @@ static int writeback_sb_inodes(struct su
 		if (inode_dirtied_after(inode, wbc->wb_start))
 			return 1;
 
-		BUG_ON(inode->i_state & I_FREEING);
 		__iget(inode);
 		pages_skipped = wbc->pages_skipped;
 		writeback_single_inode(inode, wbc);

Re: [PATCH 3/4] fs: skip I_FREEING inodes in writeback_sb_inodes

[Christian Stroetmann]

  little typos

On the 24.10.2010 19:40, Christoph Hellwig wrote:
> Skip I_FREEING inodes just like I_WILL_FREE and I_NEW when walking the
> writeback lists.  Currenly this can't happen, but once we move from
> inode_lock to more fine grained locking we can have an inode that's
> still on the writeback lists but has I_FREEING set, and we absolutely
> need to skip it here, just like we do for all other inode list walks.
>
> Based on a patch from Dave Chinner.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c	2010-10-24 13:21:18.282010741 +0200
> +++ linux-2.6/fs/fs-writeback.c	2010-10-24 13:21:35.456004735 +0200
> @@ -487,10 +487,16 @@ static int writeback_sb_inodes(struct su
>   			return 0;
>   		}
>
> -		if (inode->i_state&  (I_NEW | I_WILL_FREE)) {
> +		/*
> +		 * Don't bother with new inodes or inodes beeing freed, first
being
> +		 * kind does not need peridic writeout yet, and for the latter
> +		 * kind writeout is handled by the freer.
don't know:
freer inode
or
releaser

[...]

[PATCH 4/4] fs: kill I_WILL_FREE

[Christoph Hellwig]

The I_WILL_FREE is currently set for inodes that we write out during
umount after dropping their last reference.  It is handled equally
to I_FREEING in most places.  The two execptions are:

 - writeback_single_inode skips all list manipulations for I_FREEING,
   but not for I_WILL_FREE.  We don't care about which list an
   I_WILL_FREE inode is on, because we will remove it from the list
   a little bit later.
 - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
   inodes.  This only matters for filesystem that re-dirty the inode
   during writeback and then use the I_DIRTY flags inside ->evict_inode.
   The formers is done by XFS, but it uses it's internal state to flush
   the inode.  I could not find any filesystem that looks at I_DIRTY
   inside ->evict_inode either.

Besides cleaning up the code removing I_WILL_FREE will allow us to
avoid one i_lock roundtrip once inode_lock is split and keep iput_final
more logic.  This includes removing the __remove_inode_hash call in
iput_final, given that we never drop the protection from lookups now
that I_FREEING is set earlier.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/btrfs/inode.c
===================================================================
--- linux-2.6.orig/fs/btrfs/inode.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/btrfs/inode.c	2010-10-24 16:32:43.647253791 +0200
@@ -3862,8 +3862,7 @@ again:
 		else if (inode->i_ino > entry->vfs_inode.i_ino)
 			p = &parent->rb_right;
 		else {
-			WARN_ON(!(entry->vfs_inode.i_state &
-				  (I_WILL_FREE | I_FREEING)));
+			WARN_ON(!(entry->vfs_inode.i_state & I_FREEING));
 			rb_erase(parent, &root->inode_tree);
 			RB_CLEAR_NODE(parent);
 			spin_unlock(&root->inode_lock);
Index: linux-2.6/fs/drop_caches.c
===================================================================
--- linux-2.6.orig/fs/drop_caches.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/drop_caches.c	2010-10-24 16:32:43.647253791 +0200
@@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup
 
 	spin_lock(&inode_lock);
 	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
-		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW))
+		if (inode->i_state & (I_FREEING | I_NEW))
 			continue;
 		if (inode->i_mapping->nrpages == 0)
 			continue;
Index: linux-2.6/fs/gfs2/inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/inode.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/gfs2/inode.c	2010-10-24 16:32:43.650253651 +0200
@@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
 	struct gfs2_skip_data *data = opaque;
 
 	if (ip->i_no_addr == data->no_addr) {
-		if (inode->i_state & (I_FREEING|I_WILL_FREE)){
+		if (inode->i_state & I_FREEING){
 			data->skipped = 1;
 			return 0;
 		}
Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c	2010-10-24 16:32:39.000000000 +0200
+++ linux-2.6/fs/fs-writeback.c	2010-10-24 16:32:43.654254629 +0200
@@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
 
 /*
  * Write out an inode's dirty pages.  Called under inode_lock.  Either the
- * caller has ref on the inode (either via __iget or via syscall against an fd)
- * or the inode has I_WILL_FREE set (via generic_forget_inode)
+ * caller has ref on the inode or the inode is beeing freed.
  *
  * If `wait' is set, wait on the writeout.
  *
@@ -320,9 +319,7 @@ writeback_single_inode(struct inode *ino
 	int ret;
 
 	if (!atomic_read(&inode->i_count))
-		WARN_ON(!(inode->i_state & (I_WILL_FREE|I_FREEING)));
-	else
-		WARN_ON(inode->i_state & I_WILL_FREE);
+		WARN_ON(!(inode->i_state & I_FREEING));
 
 	if (inode->i_state & I_SYNC) {
 		/*
@@ -492,7 +489,7 @@ static int writeback_sb_inodes(struct su
 		 * kind does not need peridic writeout yet, and for the latter
 		 * kind writeout is handled by the freer.
 		 */
-		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
+		if (inode->i_state & (I_NEW | I_FREEING)) {
 			requeue_io(inode);
 			continue;
 		}
@@ -1043,7 +1040,7 @@ static void wait_sb_inodes(struct super_
 	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
 		struct address_space *mapping;
 
-		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW))
+		if (inode->i_state & (I_FREEING | I_NEW))
 			continue;
 		mapping = inode->i_mapping;
 		if (mapping->nrpages == 0)
@@ -1154,7 +1151,7 @@ EXPORT_SYMBOL(sync_inodes_sb);
  * This function commits an inode to disk immediately if it is dirty. This is
  * primarily needed by knfsd.
  *
- * The caller must either have a ref on the inode or must have set I_WILL_FREE.
+ * The caller must either have a ref on the inode or must have set I_FREEING.
  */
 int write_inode_now(struct inode *inode, int sync)
 {
Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2010-10-24 16:32:20.000000000 +0200
+++ linux-2.6/fs/inode.c	2010-10-24 16:33:46.912003547 +0200
@@ -667,7 +667,7 @@ repeat:
 			continue;
 		if (!test(inode, data))
 			continue;
-		if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
+		if (inode->i_state & I_FREEING) {
 			__wait_on_freeing_inode(inode);
 			goto repeat;
 		}
@@ -693,7 +693,7 @@ repeat:
 			continue;
 		if (inode->i_sb != sb)
 			continue;
-		if (inode->i_state & (I_FREEING|I_WILL_FREE)) {
+		if (inode->i_state & I_FREEING) {
 			__wait_on_freeing_inode(inode);
 			goto repeat;
 		}
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(iunique);
 struct inode *igrab(struct inode *inode)
 {
 	spin_lock(&inode_lock);
-	if (!(inode->i_state & (I_FREEING|I_WILL_FREE)))
+	if (!(inode->i_state & I_FREEING))
 		__iget(inode);
 	else
 		/*
@@ -1211,7 +1211,7 @@ int insert_inode_locked(struct inode *in
 				continue;
 			if (old->i_sb != sb)
 				continue;
-			if (old->i_state & (I_FREEING|I_WILL_FREE))
+			if (old->i_state & I_FREEING)
 				continue;
 			break;
 		}
@@ -1250,7 +1250,7 @@ int insert_inode_locked4(struct inode *i
 				continue;
 			if (!test(old, data))
 				continue;
-			if (old->i_state & (I_FREEING|I_WILL_FREE))
+			if (old->i_state & I_FREEING)
 				continue;
 			break;
 		}
@@ -1305,6 +1305,8 @@ static void iput_final(struct inode *ino
 	const struct super_operations *op = inode->i_sb->s_op;
 	int drop;
 
+	WARN_ON(inode->i_state & I_NEW);
+
 	if (op && op->drop_inode)
 		drop = op->drop_inode(inode);
 	else
@@ -1313,25 +1315,19 @@ static void iput_final(struct inode *ino
 	if (!drop) {
 		if (sb->s_flags & MS_ACTIVE) {
 			inode->i_state |= I_REFERENCED;
-			if (!(inode->i_state & (I_DIRTY|I_SYNC))) {
+			if (!(inode->i_state & (I_DIRTY|I_SYNC)))
 				inode_lru_list_add(inode);
-			}
 			spin_unlock(&inode_lock);
 			return;
 		}
-		WARN_ON(inode->i_state & I_NEW);
-		inode->i_state |= I_WILL_FREE;
+		inode->i_state |= I_FREEING;
 		spin_unlock(&inode_lock);
 		write_inode_now(inode, 1);
 		spin_lock(&inode_lock);
-		WARN_ON(inode->i_state & I_NEW);
-		inode->i_state &= ~I_WILL_FREE;
-		__remove_inode_hash(inode);
+	} else {
+		inode->i_state |= I_FREEING;
 	}
 
-	WARN_ON(inode->i_state & I_NEW);
-	inode->i_state |= I_FREEING;
-
 	/*
 	 * Move the inode off the IO lists and LRU once I_FREEING is
 	 * set so that it won't get moved back on there if it is dirty.
Index: linux-2.6/fs/notify/inode_mark.c
===================================================================
--- linux-2.6.orig/fs/notify/inode_mark.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/notify/inode_mark.c	2010-10-24 16:32:43.661255957 +0200
@@ -244,11 +244,11 @@ void fsnotify_unmount_inodes(struct list
 		struct inode *need_iput_tmp;
 
 		/*
-		 * We cannot __iget() an inode in state I_FREEING,
-		 * I_WILL_FREE, or I_NEW which is fine because by that point
-		 * the inode cannot have any associated watches.
+		 * We cannot __iget() an inode in state I_FREEING or I_NEW,
+		 * which is fine because by that point the inode cannot
+		 * have any associated watches.
 		 */
-		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW))
+		if (inode->i_state & (I_FREEING | I_NEW))
 			continue;
 
 		/*
@@ -272,7 +272,7 @@ void fsnotify_unmount_inodes(struct list
 		/* In case the dropping of a reference would nuke next_i. */
 		if ((&next_i->i_sb_list != list) &&
 		    atomic_read(&next_i->i_count) &&
-		    !(next_i->i_state & (I_FREEING | I_WILL_FREE))) {
+		    !(next_i->i_state & I_FREEING)) {
 			__iget(next_i);
 			need_iput = next_i;
 		}
Index: linux-2.6/fs/quota/dquot.c
===================================================================
--- linux-2.6.orig/fs/quota/dquot.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/quota/dquot.c	2010-10-24 16:32:43.665253722 +0200
@@ -898,7 +898,7 @@ static void add_dquot_ref(struct super_b
 
 	spin_lock(&inode_lock);
 	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
-		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW))
+		if (inode->i_state & (I_FREEING | I_NEW))
 			continue;
 #ifdef CONFIG_QUOTA_DEBUG
 		if (unlikely(inode_get_rsv_space(inode) > 0))
Index: linux-2.6/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_iops.c	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_iops.c	2010-10-24 16:32:43.665253722 +0200
@@ -80,7 +80,7 @@ xfs_mark_inode_dirty_sync(
 {
 	struct inode	*inode = VFS_I(ip);
 
-	if (!(inode->i_state & (I_WILL_FREE|I_FREEING)))
+	if (!(inode->i_state & I_FREEING))
 		mark_inode_dirty_sync(inode);
 }
 
@@ -90,7 +90,7 @@ xfs_mark_inode_dirty(
 {
 	struct inode	*inode = VFS_I(ip);
 
-	if (!(inode->i_state & (I_WILL_FREE|I_FREEING)))
+	if (!(inode->i_state & I_FREEING))
 		mark_inode_dirty(inode);
 }
 
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2010-10-24 16:30:05.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2010-10-24 16:32:43.673253652 +0200
@@ -1601,8 +1601,8 @@ struct super_operations {
  * I_DIRTY_DATASYNC and I_DIRTY_PAGES.
  *
  * Four bits define the lifetime of an inode.  Initially, inodes are I_NEW,
- * until that flag is cleared.  I_WILL_FREE, I_FREEING and I_CLEAR are set at
- * various stages of removing an inode.
+ * until that flag is cleared.  I_FREEING and I_CLEAR are set at various
+ * stages of removing an inode.
  *
  * Two bits are used for locking and completion notification, I_NEW and I_SYNC.
  *
@@ -1613,46 +1613,39 @@ struct super_operations {
  *			don't have to write inode on fdatasync() when only
  *			mtime has changed in it.
  * I_DIRTY_PAGES	Inode has dirty pages.  Inode itself may be clean.
+ * I_FREEING		Set when inode is about to be freed but still has dirty
+ *			pages or buffers attached or the inode itself is still
+ *			dirty.
+ * I_CLEAR		Added by end_writeback().  In this state the inode is
+ *			clean and can be destroyed.  Inode keeps I_FREEING.
+ * I_REFERENCED		Inode was recently referenced on the LRU and got another
+ *			pass before eviction.
  * I_NEW		Serves as both a mutex and completion notification.
  *			New inodes set I_NEW.  If two processes both create
  *			the same inode, one of them will release its inode and
  *			wait for I_NEW to be released before returning.
- *			Inodes in I_WILL_FREE, I_FREEING or I_CLEAR state can
- *			also cause waiting on I_NEW, without I_NEW actually
- *			being set.  find_inode() uses this to prevent returning
- *			nearly-dead inodes.
- * I_WILL_FREE		Must be set when calling write_inode_now() if i_count
- *			is zero.  I_FREEING must be set when I_WILL_FREE is
- *			cleared.
- * I_FREEING		Set when inode is about to be freed but still has dirty
- *			pages or buffers attached or the inode itself is still
- *			dirty.
- * I_CLEAR		Added by end_writeback().  In this state the inode is clean
- *			and can be destroyed.  Inode keeps I_FREEING.
- *
- *			Inodes that are I_WILL_FREE, I_FREEING or I_CLEAR are
- *			prohibited for many purposes.  iget() must wait for
- *			the inode to be completely released, then create it
- *			anew.  Other functions will just ignore such inodes,
- *			if appropriate.  I_NEW is used for waiting.
- *
+ *			Inodes in I_FREEING state can also cause waiting on
+ *			I_NEW, without I_NEW actually being set.  find_inode()
+ *			uses this to prevent returning nearly-dead inodes.
  * I_SYNC		Synchonized write of dirty inode data.  The bits is
  *			set during data writeback, and cleared with a wakeup
  *			on the bit address once it is done.
  *
- * Q: What is the difference between I_WILL_FREE and I_FREEING?
+ * Inodes that have I_FREEING set are prohibited for many purposes.
+ * iget*_locked() must wait for the inode to be completely released, then
+ * create it anew.  Other functions will just ignore such inodes, if
+ * appropriate.  I_NEW is used for waiting.
  */
 #define I_DIRTY_SYNC		(1 << 0)
 #define I_DIRTY_DATASYNC	(1 << 1)
 #define I_DIRTY_PAGES		(1 << 2)
-#define __I_NEW			3
+#define I_FREEING		(1 << 3)
+#define I_CLEAR			(1 << 4)
+#define I_REFERENCED		(1 << 5)
+#define __I_NEW			6
 #define I_NEW			(1 << __I_NEW)
-#define I_WILL_FREE		(1 << 4)
-#define I_FREEING		(1 << 5)
-#define I_CLEAR			(1 << 6)
 #define __I_SYNC		7
 #define I_SYNC			(1 << __I_SYNC)
-#define I_REFERENCED		(1 << 8)
 
 #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
 

Re: [PATCH 4/4] fs: kill I_WILL_FREE

[Christian Stroetmann]

  little typos

On the 24.10.2010 19:40, Christoph Hellwig wrote:
> The I_WILL_FREE is currently set for inodes that we write out during
> umount after dropping their last reference.  It is handled equally
> to I_FREEING in most places.  The two execptions are:
>
>   - writeback_single_inode skips all list manipulations for I_FREEING,
>     but not for I_WILL_FREE.  We don't care about which list an
>     I_WILL_FREE inode is on, because we will remove it from the list
>     a little bit later.
>   - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
>     inodes.  This only matters for filesystem that re-dirty the inode
>     during writeback and then use the I_DIRTY flags inside ->evict_inode.
>     The formers is done by XFS, but it uses it's internal state to flush
>     the inode.  I could not find any filesystem that looks at I_DIRTY
>     inside ->evict_inode either.
>
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic.  This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: linux-2.6/fs/btrfs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/btrfs/inode.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/btrfs/inode.c	2010-10-24 16:32:43.647253791 +0200
> @@ -3862,8 +3862,7 @@ again:
>   		else if (inode->i_ino>  entry->vfs_inode.i_ino)
>   			p =&parent->rb_right;
>   		else {
> -			WARN_ON(!(entry->vfs_inode.i_state&
> -				  (I_WILL_FREE | I_FREEING)));
> +			WARN_ON(!(entry->vfs_inode.i_state&  I_FREEING));
>   			rb_erase(parent,&root->inode_tree);
>   			RB_CLEAR_NODE(parent);
>   			spin_unlock(&root->inode_lock);
> Index: linux-2.6/fs/drop_caches.c
> ===================================================================
> --- linux-2.6.orig/fs/drop_caches.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/drop_caches.c	2010-10-24 16:32:43.647253791 +0200
> @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup
>
>   	spin_lock(&inode_lock);
>   	list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE|I_NEW))
> +		if (inode->i_state&  (I_FREEING | I_NEW))
>   			continue;
>   		if (inode->i_mapping->nrpages == 0)
>   			continue;
> Index: linux-2.6/fs/gfs2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/inode.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/gfs2/inode.c	2010-10-24 16:32:43.650253651 +0200
> @@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
>   	struct gfs2_skip_data *data = opaque;
>
>   	if (ip->i_no_addr == data->no_addr) {
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE)){
> +		if (inode->i_state&  I_FREEING){
>   			data->skipped = 1;
>   			return 0;
>   		}
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c	2010-10-24 16:32:39.000000000 +0200
> +++ linux-2.6/fs/fs-writeback.c	2010-10-24 16:32:43.654254629 +0200
> @@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
>
>   /*
>    * Write out an inode's dirty pages.  Called under inode_lock.  Either the
> - * caller has ref on the inode (either via __iget or via syscall against an fd)
> - * or the inode has I_WILL_FREE set (via generic_forget_inode)
> + * caller has ref on the inode or the inode is beeing freed.
>    *
>    * If `wait' is set, wait on the writeout.
>    *
> @@ -320,9 +319,7 @@ writeback_single_inode(struct inode *ino
>   	int ret;
>
>   	if (!atomic_read(&inode->i_count))
> -		WARN_ON(!(inode->i_state&  (I_WILL_FREE|I_FREEING)));
> -	else
> -		WARN_ON(inode->i_state&  I_WILL_FREE);
> +		WARN_ON(!(inode->i_state&  I_FREEING));
>
>   	if (inode->i_state&  I_SYNC) {
>   		/*
> @@ -492,7 +489,7 @@ static int writeback_sb_inodes(struct su
>   		 * kind does not need peridic writeout yet, and for the latter
>   		 * kind writeout is handled by the freer.
don't know again:
freer inode
or
releaser
>   		*/
> -		if (inode->i_state&  (I_NEW | I_FREEING | I_WILL_FREE)) {
> +		if (inode->i_state&  (I_NEW | I_FREEING)) {
>   			requeue_io(inode);
>   			continue;
>   		}
> @@ -1043,7 +1040,7 @@ static void wait_sb_inodes(struct super_
>   	list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
>   		struct address_space *mapping;
>
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE|I_NEW))
> +		if (inode->i_state&  (I_FREEING | I_NEW))
>   			continue;
>   		mapping = inode->i_mapping;
>   		if (mapping->nrpages == 0)
> @@ -1154,7 +1151,7 @@ EXPORT_SYMBOL(sync_inodes_sb);
>    * This function commits an inode to disk immediately if it is dirty. This is
>    * primarily needed by knfsd.
>    *
> - * The caller must either have a ref on the inode or must have set I_WILL_FREE.
> + * The caller must either have a ref on the inode or must have set I_FREEING.
>    */
>   int write_inode_now(struct inode *inode, int sync)
>   {
> Index: linux-2.6/fs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/inode.c	2010-10-24 16:32:20.000000000 +0200
> +++ linux-2.6/fs/inode.c	2010-10-24 16:33:46.912003547 +0200
> @@ -667,7 +667,7 @@ repeat:
>   			continue;
>   		if (!test(inode, data))
>   			continue;
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE)) {
> +		if (inode->i_state&  I_FREEING) {
>   			__wait_on_freeing_inode(inode);
>   			goto repeat;
>   		}
> @@ -693,7 +693,7 @@ repeat:
>   			continue;
>   		if (inode->i_sb != sb)
>   			continue;
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE)) {
> +		if (inode->i_state&  I_FREEING) {
>   			__wait_on_freeing_inode(inode);
>   			goto repeat;
>   		}
> @@ -964,7 +964,7 @@ EXPORT_SYMBOL(iunique);
>   struct inode *igrab(struct inode *inode)
>   {
>   	spin_lock(&inode_lock);
> -	if (!(inode->i_state&  (I_FREEING|I_WILL_FREE)))
> +	if (!(inode->i_state&  I_FREEING))
>   		__iget(inode);
>   	else
>   		/*
> @@ -1211,7 +1211,7 @@ int insert_inode_locked(struct inode *in
>   				continue;
>   			if (old->i_sb != sb)
>   				continue;
> -			if (old->i_state&  (I_FREEING|I_WILL_FREE))
> +			if (old->i_state&  I_FREEING)
>   				continue;
>   			break;
>   		}
> @@ -1250,7 +1250,7 @@ int insert_inode_locked4(struct inode *i
>   				continue;
>   			if (!test(old, data))
>   				continue;
> -			if (old->i_state&  (I_FREEING|I_WILL_FREE))
> +			if (old->i_state&  I_FREEING)
>   				continue;
>   			break;
>   		}
> @@ -1305,6 +1305,8 @@ static void iput_final(struct inode *ino
>   	const struct super_operations *op = inode->i_sb->s_op;
>   	int drop;
>
> +	WARN_ON(inode->i_state&  I_NEW);
> +
>   	if (op&&  op->drop_inode)
>   		drop = op->drop_inode(inode);
>   	else
> @@ -1313,25 +1315,19 @@ static void iput_final(struct inode *ino
>   	if (!drop) {
>   		if (sb->s_flags&  MS_ACTIVE) {
>   			inode->i_state |= I_REFERENCED;
> -			if (!(inode->i_state&  (I_DIRTY|I_SYNC))) {
> +			if (!(inode->i_state&  (I_DIRTY|I_SYNC)))
>   				inode_lru_list_add(inode);
> -			}
>   			spin_unlock(&inode_lock);
>   			return;
>   		}
> -		WARN_ON(inode->i_state&  I_NEW);
> -		inode->i_state |= I_WILL_FREE;
> +		inode->i_state |= I_FREEING;
>   		spin_unlock(&inode_lock);
>   		write_inode_now(inode, 1);
>   		spin_lock(&inode_lock);
> -		WARN_ON(inode->i_state&  I_NEW);
> -		inode->i_state&= ~I_WILL_FREE;
> -		__remove_inode_hash(inode);
> +	} else {
> +		inode->i_state |= I_FREEING;
>   	}
>
> -	WARN_ON(inode->i_state&  I_NEW);
> -	inode->i_state |= I_FREEING;
> -
>   	/*
>   	 * Move the inode off the IO lists and LRU once I_FREEING is
>   	 * set so that it won't get moved back on there if it is dirty.
> Index: linux-2.6/fs/notify/inode_mark.c
> ===================================================================
> --- linux-2.6.orig/fs/notify/inode_mark.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/notify/inode_mark.c	2010-10-24 16:32:43.661255957 +0200
> @@ -244,11 +244,11 @@ void fsnotify_unmount_inodes(struct list
>   		struct inode *need_iput_tmp;
>
>   		/*
> -		 * We cannot __iget() an inode in state I_FREEING,
> -		 * I_WILL_FREE, or I_NEW which is fine because by that point
> -		 * the inode cannot have any associated watches.
> +		 * We cannot __iget() an inode in state I_FREEING or I_NEW,
> +		 * which is fine because by that point the inode cannot
> +		 * have any associated watches.
>   		 */
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE|I_NEW))
> +		if (inode->i_state&  (I_FREEING | I_NEW))
>   			continue;
>
>   		/*
> @@ -272,7 +272,7 @@ void fsnotify_unmount_inodes(struct list
>   		/* In case the dropping of a reference would nuke next_i. */
>   		if ((&next_i->i_sb_list != list)&&
>   		atomic_read(&next_i->i_count)&&
> -		    !(next_i->i_state&  (I_FREEING | I_WILL_FREE))) {
> +		    !(next_i->i_state&  I_FREEING)) {
>   			__iget(next_i);
>   			need_iput = next_i;
>   		}
> Index: linux-2.6/fs/quota/dquot.c
> ===================================================================
> --- linux-2.6.orig/fs/quota/dquot.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/quota/dquot.c	2010-10-24 16:32:43.665253722 +0200
> @@ -898,7 +898,7 @@ static void add_dquot_ref(struct super_b
>
>   	spin_lock(&inode_lock);
>   	list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
> -		if (inode->i_state&  (I_FREEING|I_WILL_FREE|I_NEW))
> +		if (inode->i_state&  (I_FREEING | I_NEW))
>   			continue;
>   #ifdef CONFIG_QUOTA_DEBUG
>   		if (unlikely(inode_get_rsv_space(inode)>  0))
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_iops.c	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_iops.c	2010-10-24 16:32:43.665253722 +0200
> @@ -80,7 +80,7 @@ xfs_mark_inode_dirty_sync(
>   {
>   	struct inode	*inode = VFS_I(ip);
>
> -	if (!(inode->i_state&  (I_WILL_FREE|I_FREEING)))
> +	if (!(inode->i_state&  I_FREEING))
>   		mark_inode_dirty_sync(inode);
>   }
>
> @@ -90,7 +90,7 @@ xfs_mark_inode_dirty(
>   {
>   	struct inode	*inode = VFS_I(ip);
>
> -	if (!(inode->i_state&  (I_WILL_FREE|I_FREEING)))
> +	if (!(inode->i_state&  I_FREEING))
>   		mark_inode_dirty(inode);
>   }
>
> Index: linux-2.6/include/linux/fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h	2010-10-24 16:30:05.000000000 +0200
> +++ linux-2.6/include/linux/fs.h	2010-10-24 16:32:43.673253652 +0200
> @@ -1601,8 +1601,8 @@ struct super_operations {
>    * I_DIRTY_DATASYNC and I_DIRTY_PAGES.
>    *
>    * Four bits define the lifetime of an inode.  Initially, inodes are I_NEW,
comma too much?!
> - * until that flag is cleared.  I_WILL_FREE, I_FREEING and I_CLEAR are set at
> - * various stages of removing an inode.
> + * until that flag is cleared.  I_FREEING and I_CLEAR are set at various
> + * stages of removing an inode.
>    *
>    * Two bits are used for locking and completion notification, I_NEW and I_SYNC.
>    *
> @@ -1613,46 +1613,39 @@ struct super_operations {
>    *			don't have to write inode on fdatasync() when only
>    *			mtime has changed in it.
>    * I_DIRTY_PAGES	Inode has dirty pages.  Inode itself may be clean.
> + * I_FREEING		Set when inode is about to be freed but still has dirty
> + *			pages or buffers attached or the inode itself is still
maybe a comma:
, or the inode itself is
> + *			dirty.
> + * I_CLEAR		Added by end_writeback().  In this state the inode is
> + *			clean and can be destroyed.  Inode keeps I_FREEING.
> + * I_REFERENCED		Inode was recently referenced on the LRU and got another
> + *			pass before eviction.
>    * I_NEW		Serves as both a mutex and completion notification.
maybe:
Serves as both: a mutex and a completion notification.
>    *			New inodes set I_NEW.  If two processes both create
>    *			the same inode, one of them will release its inode and
, then one of them
>    *			wait for I_NEW to be released before returning.
> - *			Inodes in I_WILL_FREE, I_FREEING or I_CLEAR state can
> - *			also cause waiting on I_NEW, without I_NEW actually
> - *			being set.  find_inode() uses this to prevent returning
> - *			nearly-dead inodes.
> - * I_WILL_FREE		Must be set when calling write_inode_now() if i_count
> - *			is zero.  I_FREEING must be set when I_WILL_FREE is
> - *			cleared.
> - * I_FREEING		Set when inode is about to be freed but still has dirty
> - *			pages or buffers attached or the inode itself is still
> - *			dirty.
> - * I_CLEAR		Added by end_writeback().  In this state the inode is clean
> - *			and can be destroyed.  Inode keeps I_FREEING.
> - *
> - *			Inodes that are I_WILL_FREE, I_FREEING or I_CLEAR are
> - *			prohibited for many purposes.  iget() must wait for
> - *			the inode to be completely released, then create it
> - *			anew.  Other functions will just ignore such inodes,
> - *			if appropriate.  I_NEW is used for waiting.
> - *
> + *			Inodes in I_FREEING state can also cause waiting on
> + *			I_NEW, without I_NEW actually being set.  find_inode()
> + *			uses this to prevent returning nearly-dead inodes.
>    * I_SYNC		Synchonized write of dirty inode data.  The bits is
The bit is
>    *			set during data writeback, and cleared with a wakeup
>    *			on the bit address once it is done.
>    *
> - * Q: What is the difference between I_WILL_FREE and I_FREEING?
> + * Inodes that have I_FREEING set are prohibited for many purposes.
> + * iget*_locked() must wait for the inode to be completely released, then
> + * create it anew.  Other functions will just ignore such inodes, if
> + * appropriate.  I_NEW is used for waiting.
appropriated
>    */
>   #define I_DIRTY_SYNC		(1<<  0)
>   #define I_DIRTY_DATASYNC	(1<<  1)
>   #define I_DIRTY_PAGES		(1<<  2)
> -#define __I_NEW			3
> +#define I_FREEING		(1<<  3)
> +#define I_CLEAR			(1<<  4)
> +#define I_REFERENCED		(1<<  5)
> +#define __I_NEW			6
>   #define I_NEW			(1<<  __I_NEW)
> -#define I_WILL_FREE		(1<<  4)
> -#define I_FREEING		(1<<  5)
> -#define I_CLEAR			(1<<  6)
>   #define __I_SYNC		7
>   #define I_SYNC			(1<<  __I_SYNC)
> -#define I_REFERENCED		(1<<  8)
>
>   #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
>

Re: [PATCH 4/4] fs: kill I_WILL_FREE

[Christian Stroetmann]

  and one again
On the 24.10.2010 23:46, Christian Stroetmann wrote:
>  little typos
>
> On the 24.10.2010 19:40, Christoph Hellwig wrote:
>> The I_WILL_FREE is currently set for inodes that we write out during
>> umount after dropping their last reference.  It is handled equally
>> to I_FREEING in most places.  The two execptions are:
>>
>>   - writeback_single_inode skips all list manipulations for I_FREEING,
>>     but not for I_WILL_FREE.  We don't care about which list an
>>     I_WILL_FREE inode is on, because we will remove it from the list
>>     a little bit later.
>>   - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
>>     inodes.  This only matters for filesystem that re-dirty the inode
>>     during writeback and then use the I_DIRTY flags inside 
>> ->evict_inode.
>>     The formers is done by XFS, but it uses it's internal state to flush
>>     the inode.  I could not find any filesystem that looks at I_DIRTY
>>     inside ->evict_inode either.
>>
>> Besides cleaning up the code removing I_WILL_FREE will allow us to
>> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
>> more logic.  This includes removing the __remove_inode_hash call in
>> iput_final, given that we never drop the protection from lookups now
>> that I_FREEING is set earlier.
>>
>> Signed-off-by: Christoph Hellwig<hch@lst.de>
>>
>> Index: linux-2.6/fs/btrfs/inode.c
>> ===================================================================
>> --- linux-2.6.orig/fs/btrfs/inode.c    2010-10-24 16:30:05.000000000 
>> +0200
>> +++ linux-2.6/fs/btrfs/inode.c    2010-10-24 16:32:43.647253791 +0200
>> @@ -3862,8 +3862,7 @@ again:
>>           else if (inode->i_ino>  entry->vfs_inode.i_ino)
>>               p =&parent->rb_right;
>>           else {
>> -            WARN_ON(!(entry->vfs_inode.i_state&
>> -                  (I_WILL_FREE | I_FREEING)));
>> +            WARN_ON(!(entry->vfs_inode.i_state&  I_FREEING));
>>               rb_erase(parent,&root->inode_tree);
>>               RB_CLEAR_NODE(parent);
>>               spin_unlock(&root->inode_lock);
>> Index: linux-2.6/fs/drop_caches.c
>> ===================================================================
>> --- linux-2.6.orig/fs/drop_caches.c    2010-10-24 16:30:05.000000000 
>> +0200
>> +++ linux-2.6/fs/drop_caches.c    2010-10-24 16:32:43.647253791 +0200
>> @@ -18,7 +18,7 @@ static void drop_pagecache_sb(struct sup
>>
>>       spin_lock(&inode_lock);
>>       list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
>> -        if (inode->i_state&  (I_FREEING|I_WILL_FREE|I_NEW))
>> +        if (inode->i_state&  (I_FREEING | I_NEW))
>>               continue;
>>           if (inode->i_mapping->nrpages == 0)
>>               continue;
>> Index: linux-2.6/fs/gfs2/inode.c
>> ===================================================================
>> --- linux-2.6.orig/fs/gfs2/inode.c    2010-10-24 16:30:05.000000000 
>> +0200
>> +++ linux-2.6/fs/gfs2/inode.c    2010-10-24 16:32:43.650253651 +0200
>> @@ -84,7 +84,7 @@ static int iget_skip_test(struct inode *
>>       struct gfs2_skip_data *data = opaque;
>>
>>       if (ip->i_no_addr == data->no_addr) {
>> -        if (inode->i_state&  (I_FREEING|I_WILL_FREE)){
>> +        if (inode->i_state&  I_FREEING){
>>               data->skipped = 1;
>>               return 0;
>>           }
>> Index: linux-2.6/fs/fs-writeback.c
>> ===================================================================
>> --- linux-2.6.orig/fs/fs-writeback.c    2010-10-24 16:32:39.000000000 
>> +0200
>> +++ linux-2.6/fs/fs-writeback.c    2010-10-24 16:32:43.654254629 +0200
>> @@ -301,8 +301,7 @@ static void inode_wait_for_writeback(str
>>
>>   /*
>>    * Write out an inode's dirty pages.  Called under inode_lock.  
>> Either the
>> - * caller has ref on the inode (either via __iget or via syscall 
>> against an fd)
>> - * or the inode has I_WILL_FREE set (via generic_forget_inode)
>> + * caller has ref on the inode or the inode is beeing freed.
being
  [...]

Re: [PATCH 4/4] fs: kill I_WILL_FREE

[Al Viro]

On Sun, Oct 24, 2010 at 07:40:58PM +0200, Christoph Hellwig wrote:
> The I_WILL_FREE is currently set for inodes that we write out during
> umount after dropping their last reference.  It is handled equally
> to I_FREEING in most places.  The two execptions are:
> 
>  - writeback_single_inode skips all list manipulations for I_FREEING,
>    but not for I_WILL_FREE.  We don't care about which list an
>    I_WILL_FREE inode is on, because we will remove it from the list
>    a little bit later.
>  - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
>    inodes.  This only matters for filesystem that re-dirty the inode
>    during writeback and then use the I_DIRTY flags inside ->evict_inode.
>    The formers is done by XFS, but it uses it's internal state to flush
>    the inode.  I could not find any filesystem that looks at I_DIRTY
>    inside ->evict_inode either.
> 
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic.  This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.

All except that one applied, exofs patch dropped, fixes folded, etc.
Branch pushed in the same place...

Re: [PATCH 4/4] fs: kill I_WILL_FREE

[Al Viro]

On Sun, Oct 24, 2010 at 07:40:58PM +0200, Christoph Hellwig wrote:
>  - writeback_single_inode skips all list manipulations for I_FREEING,
>    but not for I_WILL_FREE.  We don't care about which list an
>    I_WILL_FREE inode is on, because we will remove it from the list
>    a little bit later.
>  - __mark_inode_dirty skips I_FREEING inodes but not I_WILL_FREE
>    inodes.  This only matters for filesystem that re-dirty the inode
>    during writeback and then use the I_DIRTY flags inside ->evict_inode.
>    The formers is done by XFS, but it uses it's internal state to flush
>    the inode.  I could not find any filesystem that looks at I_DIRTY
>    inside ->evict_inode either.
> 
> Besides cleaning up the code removing I_WILL_FREE will allow us to
> avoid one i_lock roundtrip once inode_lock is split and keep iput_final
> more logic.  This includes removing the __remove_inode_hash call in
> iput_final, given that we never drop the protection from lookups now
> that I_FREEING is set earlier.

The thing is, the code is really brittle in that area.  What we rely upon
is
	* due to sync_filesystem() done just before there won't be anything
dirty during invalidate_inodes() (OK, evict_inodes() now) run.
	* for final iput done during ->put_super() we won't redirty the
inode.  Note that we used to loop in write_one_inode() until the sucker
got clean.  Not anymore (since 2002 or so).
	* XFS does redirty, but then it does explicit write of those guys
itself (from ->put_super()).

Let's leave that alone for now; I remember quite well how much PITA has
that area caused us in the past (my fuckup in 2.4.15, etc.) and I'd rather
not mix revisiting that fun place with other non-trivial work.

Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Dave Chinner]

On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
> 
> Despite the comment above it we can not safely drop the lock here.
> invalidate_list is called from many other places that just umount.
> Also switch to proper list macros now that we never drop the lock.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

What tree are these patches against - a couple of them don't quite
apply to Al's merge-stem tree...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Dave Chinner]

On Mon, Oct 25, 2010 at 04:33:53PM +1100, Dave Chinner wrote:
> On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
> > 
> > Despite the comment above it we can not safely drop the lock here.
> > invalidate_list is called from many other places that just umount.
> > Also switch to proper list macros now that we never drop the lock.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> What tree are these patches against - a couple of them don't quite
> apply to Al's merge-stem tree...

Not sure whether it's my merge, something in Al's tree series or
something brought in from the post-2.6.36 tree, but it goes splat
pretty quickly (xfstest 013) with:

[  302.233778] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[  302.235129] IP: [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[  302.235997] PGD 7a98a067 PUD 7a98b067 PMD 0
[  302.236024] Oops: 0002 [#1] SMP
[  302.236024] last sysfs file: /sys/devices/virtio-pci/virtio1/block/vda/removable
[  302.236024] CPU 1
[  302.236024] Modules linked in:
[  302.236024]
[  302.236024] Pid: 2523, comm: flush-253:0 Not tainted 2.6.36-dgc+ #577 /Bochs
[  302.236024] RIP: 0010:[<ffffffff8114cc3a>]  [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[  302.236024] RSP: 0018:ffff88007b6a3c60  EFLAGS: 00010246
[  302.236024] RAX: ffff88007cf3c820 RBX: ffff88007c3e1000 RCX: ffff88007b6a3c60
[  302.236024] RDX: 0000000000000000 RSI: ffff88007b1f5220 RDI: ffff88007c3e1000
[  302.236024] RBP: ffff88007b6a3c70 R08: ffff88007b6a3c60 R09: ffff88007c1eb800
[  302.236024] R10: ffff88007b6a3c60 R11: 0000000000000001 R12: ffff88007c199778
[  302.236024] R13: ffff88007619b1d0 R14: ffff88007c199808 R15: ffffffff81d427e0
[  302.236024] FS:  0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
[  302.236024] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  302.236024] CR2: 0000000000000008 CR3: 000000007a989000 CR4: 00000000000006e0
[  302.236024] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  302.236024] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  302.236024] Process flush-253:0 (pid: 2523, threadinfo ffff88007b6a2000, task ffff88007c91b040)
[  302.236024] Stack:
[  302.236024]  ffff88007b6a3c70 ffff88007c3e1000 ffff88007b6a3c90 ffffffff8114d135
[  302.236024] <0> ffff88007b6a3c90 ffff88007c3e1000 ffff88007b6a3ce0 ffffffff8116e010
[  302.236024] <0> ffff88007b6a3d30 ffff88007c3e1068 0000000000000000 ffff88007b461ec0
[  302.236024] Call Trace:
[  302.236024]  [<ffffffff8114d135>] put_super+0x25/0x40
[  302.236024]  [<ffffffff8116e010>] writeback_inodes_wb+0x130/0x180
[  302.236024]  [<ffffffff8116e2fb>] wb_writeback+0x29b/0x410
[  302.236024]  [<ffffffff8105fb89>] ? default_spin_lock_flags+0x9/0x10
[  302.236024]  [<ffffffff817f6a4f>] ? _raw_spin_lock_irqsave+0x2f/0x40
[  302.236024]  [<ffffffff8116e507>] wb_do_writeback+0x97/0x1e0
[  302.236024]  [<ffffffff8116e702>] bdi_writeback_thread+0xb2/0x270
[  302.236024]  [<ffffffff8116e650>] ? bdi_writeback_thread+0x0/0x270
[  302.236024]  [<ffffffff8109f976>] kthread+0x96/0xa0
[  302.236024]  [<ffffffff81036de4>] kernel_thread_helper+0x4/0x10
[  302.236024]  [<ffffffff8109f8e0>] ? kthread+0x0/0xa0
[  302.236024]  [<ffffffff81036de0>] ? kernel_thread_helper+0x0/0x10
[  302.236024] Code: 90 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 8b 87 a8 00 00 00 48 89 fb ff c8 89 87 a8 00 00 00 85 c0 75
[  302.236024] RIP  [<ffffffff8114cc3a>] __put_super+0x2a/0x80
[  302.236024]  RSP <ffff88007b6a3c60>
[  302.236024] CR2: 0000000000000008
[  302.265338] ---[ end trace 64118e3d92267d36 ]---

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Dave Chinner]

On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> On Mon, Oct 25, 2010 at 04:33:53PM +1100, Dave Chinner wrote:
> > On Sun, Oct 24, 2010 at 07:40:24PM +0200, Christoph Hellwig wrote:
> > > 
> > > Despite the comment above it we can not safely drop the lock here.
> > > invalidate_list is called from many other places that just umount.
> > > Also switch to proper list macros now that we never drop the lock.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > What tree are these patches against - a couple of them don't quite
> > apply to Al's merge-stem tree...
> 
> Not sure whether it's my merge, something in Al's tree series or
> something brought in from the post-2.6.36 tree, but it goes splat
> pretty quickly (xfstest 013) with:
> 
> [  302.233778] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [  302.235129] IP: [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [  302.235997] PGD 7a98a067 PUD 7a98b067 PMD 0
> [  302.236024] Oops: 0002 [#1] SMP
> [  302.236024] last sysfs file: /sys/devices/virtio-pci/virtio1/block/vda/removable
> [  302.236024] CPU 1
> [  302.236024] Modules linked in:
> [  302.236024]
> [  302.236024] Pid: 2523, comm: flush-253:0 Not tainted 2.6.36-dgc+ #577 /Bochs
> [  302.236024] RIP: 0010:[<ffffffff8114cc3a>]  [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [  302.236024] RSP: 0018:ffff88007b6a3c60  EFLAGS: 00010246
> [  302.236024] RAX: ffff88007cf3c820 RBX: ffff88007c3e1000 RCX: ffff88007b6a3c60
> [  302.236024] RDX: 0000000000000000 RSI: ffff88007b1f5220 RDI: ffff88007c3e1000
> [  302.236024] RBP: ffff88007b6a3c70 R08: ffff88007b6a3c60 R09: ffff88007c1eb800
> [  302.236024] R10: ffff88007b6a3c60 R11: 0000000000000001 R12: ffff88007c199778
> [  302.236024] R13: ffff88007619b1d0 R14: ffff88007c199808 R15: ffffffff81d427e0
> [  302.236024] FS:  0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000
> [  302.236024] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  302.236024] CR2: 0000000000000008 CR3: 000000007a989000 CR4: 00000000000006e0
> [  302.236024] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  302.236024] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [  302.236024] Process flush-253:0 (pid: 2523, threadinfo ffff88007b6a2000, task ffff88007c91b040)
> [  302.236024] Stack:
> [  302.236024]  ffff88007b6a3c70 ffff88007c3e1000 ffff88007b6a3c90 ffffffff8114d135
> [  302.236024] <0> ffff88007b6a3c90 ffff88007c3e1000 ffff88007b6a3ce0 ffffffff8116e010
> [  302.236024] <0> ffff88007b6a3d30 ffff88007c3e1068 0000000000000000 ffff88007b461ec0
> [  302.236024] Call Trace:
> [  302.236024]  [<ffffffff8114d135>] put_super+0x25/0x40
> [  302.236024]  [<ffffffff8116e010>] writeback_inodes_wb+0x130/0x180
> [  302.236024]  [<ffffffff8116e2fb>] wb_writeback+0x29b/0x410
> [  302.236024]  [<ffffffff8105fb89>] ? default_spin_lock_flags+0x9/0x10
> [  302.236024]  [<ffffffff817f6a4f>] ? _raw_spin_lock_irqsave+0x2f/0x40
> [  302.236024]  [<ffffffff8116e507>] wb_do_writeback+0x97/0x1e0
> [  302.236024]  [<ffffffff8116e702>] bdi_writeback_thread+0xb2/0x270
> [  302.236024]  [<ffffffff8116e650>] ? bdi_writeback_thread+0x0/0x270
> [  302.236024]  [<ffffffff8109f976>] kthread+0x96/0xa0
> [  302.236024]  [<ffffffff81036de4>] kernel_thread_helper+0x4/0x10
> [  302.236024]  [<ffffffff8109f8e0>] ? kthread+0x0/0xa0
> [  302.236024]  [<ffffffff81036de0>] ? kernel_thread_helper+0x0/0x10
> [  302.236024] Code: 90 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 8b 87 a8 00 00 00 48 89 fb ff c8 89 87 a8 00 00 00 85 c0 75
> [  302.236024] RIP  [<ffffffff8114cc3a>] __put_super+0x2a/0x80
> [  302.236024]  RSP <ffff88007b6a3c60>
> [  302.236024] CR2: 0000000000000008
> [  302.265338] ---[ end trace 64118e3d92267d36 ]---

This is coming form Al's tree, so it's not these specific patches
that are triggering it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Christoph Hellwig]

On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> Not sure whether it's my merge, something in Al's tree series or
> something brought in from the post-2.6.36 tree, but it goes splat
> pretty quickly (xfstest 013) with:

That's because the version of the separate wb and lru lists patch that
Al took from your tree was missing various deletions from the wb list.

Below is the fix I sent him, not sure why it didn't make it to the list:

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2010-10-24 16:30:07.592253652 +0200
+++ linux-2.6/fs/inode.c	2010-10-24 16:31:31.132003756 +0200
@@ -509,8 +509,14 @@ static int invalidate_list(struct list_h
 			continue;
 		}
 
-		list_move(&inode->i_lru, dispose);
 		inode->i_state |= I_FREEING;
+
+		/*
+		 * Move the inode off the IO lists and LRU once I_FREEING is
+		 * set so that it won't get moved back on there if it is dirty.
+		 */
+		list_move(&inode->i_lru, dispose);
+		list_del_init(&inode->i_wb_list);
 		if (!(inode->i_state & (I_DIRTY | I_SYNC)))
 			percpu_counter_dec(&nr_inodes_unused);
 	}
@@ -620,9 +626,15 @@ static void prune_icache(int nr_to_scan)
 			if (!can_unuse(inode))
 				continue;
 		}
-		list_move(&inode->i_lru, &freeable);
 		WARN_ON(inode->i_state & I_NEW);
 		inode->i_state |= I_FREEING;
+
+		/*
+		 * Move the inode off the IO lists and LRU once I_FREEING is
+		 * set so that it won't get moved back on there if it is dirty.
+		 */
+		list_move(&inode->i_lru, &freeable);
+		list_del_init(&inode->i_wb_list);
 		percpu_counter_dec(&nr_inodes_unused);
 	}
 	if (current_is_kswapd())
@@ -1343,16 +1355,16 @@ static void iput_final(struct inode *ino
 		inode->i_state &= ~I_WILL_FREE;
 		__remove_inode_hash(inode);
 	}
-	list_del_init(&inode->i_wb_list);
+
 	WARN_ON(inode->i_state & I_NEW);
 	inode->i_state |= I_FREEING;
 
 	/*
-	 * After we delete the inode from the LRU here, we avoid moving dirty
-	 * inodes back onto the LRU now because I_FREEING is set and hence
-	 * writeback_single_inode() won't move the inode around.
+	 * Move the inode off the IO lists and LRU once I_FREEING is
+	 * set so that it won't get moved back on there if it is dirty.
 	 */
 	inode_lru_list_del(inode);
+	list_del_init(&inode->i_wb_list);
 
 	__inode_sb_list_del(inode);
 	spin_unlock(&inode_lock);

Re: [PATCH 1/4] fs: do not drop inode_lock in dispose_list

[Dave Chinner]

On Mon, Oct 25, 2010 at 12:07:58PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 25, 2010 at 04:46:11PM +1100, Dave Chinner wrote:
> > Not sure whether it's my merge, something in Al's tree series or
> > something brought in from the post-2.6.36 tree, but it goes splat
> > pretty quickly (xfstest 013) with:
> 
> That's because the version of the separate wb and lru lists patch that
> Al took from your tree was missing various deletions from the wb list.
> 
> Below is the fix I sent him, not sure why it didn't make it to the list:

Fix works just fine. Thanks.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com