Skip to contentSkip to conversation

lock block groups when initializing

PATCH In review Eric Sandeen 3 participants 5 messages Jul 18, 2008 – Jul 21, 2008 ext4 file system

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

PATCH 2/2 lock block groups when initializing

I noticed when filling a 1T filesystem with 4 threads using the fs_mark benchmark:

fs_mark -d /mnt/test -D 256 -n 100000 -t 4 -s 20480 -F -S 0

that I occasionally got checksum mismatch errors:

EXT4-fs error (device sdb): ext4_init_inode_bitmap: Checksum bad for group 6935

etc. I'd reliably get 4-5 of them during the run.

It appears that the problem is likely a race to init the bg's when the uninit_bg feature is enabled.

With the patch below, which adds sb_bgl_locking around initialization, I was able to complete several runs with no errors or warnings.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/balloc.c +3 -0
Index: linux-2.6/fs/ext4/balloc.c
===================================================================
@@ -321,12 +321,15 @@ ext4_read_block_bitmap(struct super_bloc
 	if (bh_uptodate_or_lock(bh))
 		return bh;
 
+	spin_lock(sb_bgl_lock(EXT4_SB(sb), block_group));
 	if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
 		ext4_init_block_bitmap(sb, bh, block_group, desc);
 		set_buffer_uptodate(bh);
 		unlock_buffer(bh);
+		spin_unlock(sb_bgl_lock(EXT4_SB(sb), block_group));
 		return bh;
 	}
+	spin_unlock(sb_bgl_lock(EXT4_SB(sb), block_group));
 	if (bh_submit_read(bh) < 0) {
 		put_bh(bh);
 		ext4_error(sb, __func__,
fs/ext4/ialloc.c +4 -1
Index: linux-2.6/fs/ext4/ialloc.c
===================================================================
@@ -118,12 +118,15 @@ ext4_read_inode_bitmap(struct super_bloc
 	if (bh_uptodate_or_lock(bh))
 		return bh;
 
+	spin_lock(sb_bgl_lock(EXT4_SB(sb), block_group));
 	if (desc->bg_flags & cpu_to_le16(EXT4_BG_INODE_UNINIT)) {
 		ext4_init_inode_bitmap(sb, bh, block_group, desc);
 		set_buffer_uptodate(bh);
 		unlock_buffer(bh);
+		spin_unlock(sb_bgl_lock(EXT4_SB(sb), block_group));
 		return bh;
 	}
+	spin_unlock(sb_bgl_lock(EXT4_SB(sb), block_group));
 	if (bh_submit_read(bh) < 0) {
 		put_bh(bh);
 		ext4_error(sb, __func__,

@@ -735,7 +738,7 @@ got:
 
 			/* When marking the block group with
 			 * ~EXT4_BG_INODE_UNINIT we don't want to depend
-			 * on the value of bg_itable_unsed even though
+			 * on the value of bg_itable_unused even though
 			 * mke2fs could have initialized the same for us.
 			 * Instead we calculated the value below
 			 */
fs/ext4/mballoc.c +3 -0
Index: linux-2.6/fs/ext4/mballoc.c
===================================================================
@@ -787,13 +787,16 @@ static int ext4_mb_init_cache(struct pag
 		if (bh_uptodate_or_lock(bh[i]))
 			continue;
 
+		spin_lock(sb_bgl_lock(EXT4_SB(sb), first_group + i));
 		if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
 			ext4_init_block_bitmap(sb, bh[i],
 						first_group + i, desc);
 			set_buffer_uptodate(bh[i]);
 			unlock_buffer(bh[i]);
+			spin_unlock(sb_bgl_lock(EXT4_SB(sb), first_group + i));
 			continue;
 		}
+		spin_unlock(sb_bgl_lock(EXT4_SB(sb), first_group + i));
 		get_bh(bh[i]);
 		bh[i]->b_end_io = end_buffer_read_sync;
 		submit_bh(READ, bh[i]);Index: linux-2.6/fs/ext4/super.c

===================================================================

fs/ext4/super.c +2 -0
@@ -1621,6 +1621,7 @@ static int ext4_check_descriptors(struct
 			       "(block %llu)!", i, inode_table);
 			return 0;
 		}
+		spin_lock(sb_bgl_lock(sbi, i));
 		if (!ext4_group_desc_csum_verify(sbi, i, gdp)) {
 			printk(KERN_ERR "EXT4-fs: ext4_check_descriptors: "
 			       "Checksum for group %lu failed (%u!=%u)\n",

@@ -1628,6 +1629,7 @@ static int ext4_check_descriptors(struct
 			       gdp)), le16_to_cpu(gdp->bg_checksum));
 			return 0;
 		}
+		spin_unlock(sb_bgl_lock(sbi, i));
 		if (!flexbg_flag)
 			first_block += EXT4_BLOCKS_PER_GROUP(sb);
 	}
maintainer
#1 lore raw

I've added both of these patches to the ext4 patch queue for testing.

- Ted

author
#2 lore raw
Quoted 3 lines · Theodore Tso

I've added both of these patches to the ext4 patch queue for testing.

- Ted

Thanks, I'm hesitant to add my own patches w/o some review first :)

-Eric

maintainer
#3 lore raw
Quoted 7 lines · Eric Sandeen
Quoted 3 lines · Theodore Tso

I've added both of these patches to the ext4 patch queue for testing.

- Ted

Thanks, I'm hesitant to add my own patches w/o some review first :)

I haven't done a full review yet; just a "quick eyeball" of the patch, and then I put it on my laptop and noted that it hadn't destroyed my home directory. :-)

I'll try to do a full review in the few days; I'll move it into the stable queue and push it and a few other patches to the mainline (although it appears that Linus went on vacation before pushing out -rc1; Alan mentioned in another message that Linus might be gone for "a week or two", which surprises me if he really went on vacation for that length of time without pushing out -rc1 first.)

- Ted

#4 lore raw
In reply to Eric Sandeen
Quoted 8 lines · Eric Sandeen

I noticed when filling a 1T filesystem with 4 threads using the fs_mark benchmark that I occasionally got checksum mismatch errors:

It appears that the problem is likely a race to init the bg's when the uninit_bg feature is enabled.

With the patch below, which adds sb_bgl_locking around initialization, I was able to complete several runs with no errors or warnings.

Thanks for finding this.

Quoted 1 line
Signed-off-by Eric Sandeen <[email protected]>

You can add Acked-by: Andreas Dilger <[email protected]>

fs/ext4/balloc.c ext4_read_block_bitmap(struct super_bloc · line 331

Quoted 12 lines
fs/ext4/balloc.c +2 -0
@@ -321,12 +321,15 @@ ext4_read_block_bitmap(struct super_bloc
 	if (bh_uptodate_or_lock(bh))
 		return bh;
 
+	spin_lock(sb_bgl_lock(EXT4_SB(sb), block_group));
 	if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
 		ext4_init_block_bitmap(sb, bh, block_group, desc);
 		set_buffer_uptodate(bh);
 		unlock_buffer(bh);
+		spin_unlock(sb_bgl_lock(EXT4_SB(sb), block_group));
 		return bh;
 	}

Since this flag will only ever change from set to unset, and will in many cases not be set, we should first check if it is set without the spin_lock(), and then if set re-check under the lock. This avoids putting an extra lock in this path. While it may seem that we are already locking and slow because of the locked buffer_head, the sb_bgl_lock() is hashed so many groups will share the same lock and we may cause contention on other CPUs accessing other groups needlessly.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.