Re: [PATCH] concurrent inode allocation for ext2 against 2.5.64

[Andrew Morton]

Alex Tomas <[e-mail]> wrote:
>
> 
> hi!
> 
> here is the patch for ext2 concurrent inode allocation. should be applied
> on top of previous concurrent-balloc patch. tested on dual p3 for several
> hours of stress-test + fsck. hope someone test it on big iron ;)
> 

> ...
> +void ext2_reserve_inode (struct super_block * sb, int group, int dir)
> +{

This can have static scope.  And, please, no spaces after the function name,
nor after the `*' thingy.  ext2 is all over the place in this regard and I'm
slowly trying to get it consistent.

I'm not sure that skipping setting s_dirt is desirable.  Sure, we haven't
actually altered the superblock.  But we sort-of "virtually dirtied" it.  The
superblock is now out-of-date and we should sync it.

It could be that not writing the superblock for a week is an OK thing to do. 
inode and block allocation counts are something which fsck can trivially fix
up.  But at the cost of a single sector write per five seconds I think it's
best to keep the superblock more up-to-date.

I'll make the same change to the block allocator patches.

>  struct ext2_bg_info {
>  	u8 debts;
>  	spinlock_t balloc_lock;
> +	spinlock_t ialloc_lock;
>  	unsigned int reserved;
>  } ____cacheline_aligned_in_smp;
> 

hm, I wonder if this should be in a separate cacheline.  We may as well use a
single lock if they're this close together.  Bill, can you test that
sometime?

diff -puN fs/ext2/ialloc.c~ext2-ialloc-no-lock_super-fixes fs/ext2/ialloc.c
--- 25/fs/ext2/ialloc.c~ext2-ialloc-no-lock_super-fixes	2003-03-15 13:36:14.000000000 -0800
+++ 25-akpm/fs/ext2/ialloc.c	2003-03-15 13:40:43.000000000 -0800
@@ -63,7 +63,17 @@ error_out:
 	return bh;
 }
 
-void ext2_reserve_inode (struct super_block * sb, int group, int dir)
+/*
+ * Speculatively reserve an inode in a blockgroup which used to have some
+ * spare ones.  Later, when we come to actually claim the inode in the bitmap
+ * it may be that it was taken.  In that case the allocator will undo this
+ * reservation and try again.
+ *
+ * The inode allocator does not physically alter the superblock.  But we still
+ * set sb->s_dirt, because the superblock was "logically" altered - we need to
+ * go and add up the free inodes counts again and flush out the superblock.
+ */
+static void ext2_reserve_inode(struct super_block *sb, int group, int dir)
 {
 	struct ext2_group_desc * desc;
 	struct buffer_head *bh;
@@ -72,7 +82,7 @@ void ext2_reserve_inode (struct super_bl
 	if (!desc) {
 		ext2_error(sb, "ext2_reserve_inode",
 			"can't get descriptor for group %d", group);
-	return;
+		return;
 	}
 
 	spin_lock(&EXT2_SB(sb)->s_bgi[group].ialloc_lock);
@@ -82,11 +92,11 @@ void ext2_reserve_inode (struct super_bl
 		desc->bg_used_dirs_count =
 			cpu_to_le16(le16_to_cpu(desc->bg_used_dirs_count) + 1);
 	spin_unlock(&EXT2_SB(sb)->s_bgi[group].ialloc_lock);
-
+	sb->s_dirt = 1;
 	mark_buffer_dirty(bh);
 }
 
-void ext2_release_inode (struct super_block * sb, int group, int dir)
+static void ext2_release_inode(struct super_block *sb, int group, int dir)
 {
 	struct ext2_group_desc * desc;
 	struct buffer_head *bh;
@@ -105,7 +115,7 @@ void ext2_release_inode (struct super_bl
 		desc->bg_used_dirs_count =
 			cpu_to_le16(le16_to_cpu(desc->bg_used_dirs_count) - 1);
 	spin_unlock(&EXT2_SB(sb)->s_bgi[group].ialloc_lock);
-
+	sb->s_dirt = 1;
 	mark_buffer_dirty(bh);
 }
 

_