Re: [PATCH] Properly init address_space->writeback_index

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 14 Aug 2008 14:13:40 -0400
Chris Mason <chris.mason@oracle.com> wrote:

> write_cache_pages uses i_mapping->writeback_index to pick up where it
> left off the last time a given inode was found by pdflush or
> balance_dirty_pages (or anyone else who sets wbc->range_cyclic)
> 
> alloc_inode should set it to a sane value so that writeback doesn't start
> in the middle of a file.  It is somewhat difficult to notice the bug since
> write_cache_pages will loop around to the start of the file and the
> elevator helps hide the resulting seeks.
> 
> For whatever reason, Btrfs hits this often.  Unpatched, untarring 30 copies of
> the linux kernel in series runs at 47MB/s on a single sata drive.  With
> this fix, it jumps to 62MB/s.
> 
> Signed-off-by: Chris Mason <chris.mason@oracle.com>
> 
> diff -r 67160ae21a58 fs/inode.c
> --- a/fs/inode.c	Wed Aug 06 19:26:20 2008 -0700
> +++ b/fs/inode.c	Thu Aug 14 10:15:49 2008 -0400
> @@ -166,6 +166,7 @@ static struct inode *alloc_inode(struct 
>  		mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
>  		mapping->assoc_mapping = NULL;
>  		mapping->backing_dev_info = &default_backing_dev_info;
> +		mapping->writeback_index = 0;
>  
>  		/*
>  		 * If the block_device provides a backing_dev_info for client
> 

inode_cachep's ctor memsets the whole inode* to zero.

Does btrfs have an ->alloc_inode handler?  If so, perhaps it is btrfs
which needs to zap that field.

Formally, the way slab works (which is arguably silly from a CPU cache
efficiency POV) is that the object should be returned to slab
(kmem_cache_free()) in a "constructed" manner.  Which means that this
zeroing should happen in destroy_inode().

otoh, the fs/inode.c inode creation/destruction code isn't designed
that way - it chose to initialise everything synchronously in
alloc_inode(), which is arguably wrong.

One wonders if that big memset in inode_init_once() actually does
anything useful.  

Ho hum, so what to do?  I'm suspecting that we should just zap the
kmem_cachep constructor altogether, use a synchronous call to
inode_init_once() to initialise all inode fields and then remove all
those open-coded zeroings.


Something like this?

From: Andrew Morton <akpm@linux-foundation.org>

Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/inode.c |   40 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff -puN fs/inode.c~a fs/inode.c
--- a/fs/inode.c~a
+++ a/fs/inode.c
@@ -115,34 +115,24 @@ static struct inode *alloc_inode(struct 
 	static const struct file_operations empty_fops;
 	struct inode *inode;
 
-	if (sb->s_op->alloc_inode)
+	if (sb->s_op->alloc_inode) {
 		inode = sb->s_op->alloc_inode(sb);
-	else
-		inode = (struct inode *) kmem_cache_alloc(inode_cachep, GFP_KERNEL);
+	} else {
+		inode = kmem_cache_alloc(inode_cachep, GFP_KERNEL);
+		if (inode)
+			inode_init_once(inode);
+	}
 
 	if (inode) {
 		struct address_space * const mapping = &inode->i_data;
 
 		inode->i_sb = sb;
 		inode->i_blkbits = sb->s_blocksize_bits;
-		inode->i_flags = 0;
 		atomic_set(&inode->i_count, 1);
 		inode->i_op = &empty_iops;
 		inode->i_fop = &empty_fops;
 		inode->i_nlink = 1;
 		atomic_set(&inode->i_writecount, 0);
-		inode->i_size = 0;
-		inode->i_blocks = 0;
-		inode->i_bytes = 0;
-		inode->i_generation = 0;
-#ifdef CONFIG_QUOTA
-		memset(&inode->i_dquot, 0, sizeof(inode->i_dquot));
-#endif
-		inode->i_pipe = NULL;
-		inode->i_bdev = NULL;
-		inode->i_cdev = NULL;
-		inode->i_rdev = 0;
-		inode->dirtied_when = 0;
 		if (security_inode_alloc(inode)) {
 			if (inode->i_sb->s_op->destroy_inode)
 				inode->i_sb->s_op->destroy_inode(inode);
@@ -158,15 +148,13 @@ static struct inode *alloc_inode(struct 
 		lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);
 
 		init_rwsem(&inode->i_alloc_sem);
-		lockdep_set_class(&inode->i_alloc_sem, &sb->s_type->i_alloc_sem_key);
+		lockdep_set_class(&inode->i_alloc_sem,
+					&sb->s_type->i_alloc_sem_key);
 
 		mapping->a_ops = &empty_aops;
  		mapping->host = inode;
-		mapping->flags = 0;
 		mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
-		mapping->assoc_mapping = NULL;
 		mapping->backing_dev_info = &default_backing_dev_info;
-		mapping->writeback_index = 0;
 
 		/*
 		 * If the block_device provides a backing_dev_info for client
@@ -181,7 +169,6 @@ static struct inode *alloc_inode(struct 
 				bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
 			mapping->backing_dev_info = bdi;
 		}
-		inode->i_private = NULL;
 		inode->i_mapping = mapping;
 	}
 	return inode;
@@ -197,7 +184,6 @@ void destroy_inode(struct inode *inode) 
 		kmem_cache_free(inode_cachep, (inode));
 }
 
-
 /*
  * These are initializations that only need to be done
  * once, because the fields are idempotent across use
@@ -222,16 +208,8 @@ void inode_init_once(struct inode *inode
 	mutex_init(&inode->inotify_mutex);
 #endif
 }
-
 EXPORT_SYMBOL(inode_init_once);
 
-static void init_once(void *foo)
-{
-	struct inode * inode = (struct inode *) foo;
-
-	inode_init_once(inode);
-}
-
 /*
  * inode_lock must be held
  */
@@ -1400,7 +1378,7 @@ void __init inode_init(void)
 					 0,
 					 (SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
 					 SLAB_MEM_SPREAD),
-					 init_once);
+					 NULL);
 	register_shrinker(&icache_shrinker);
 
 	/* Hash may have been set up in inode_init_early */
_


Note that this assumes that a filesystem which implements
->alloc_inode() will call inode_init_once() within its ->alloc_inode().

Which means, I think, that we can just remove inode_init_once()
altogether and move its initialisations into alloc_inode() along with
all the existing ones.

What do you think?


I suppose that's all a fun project but doesn't preclude merging your
patchlet.  I'll do that for now ;)