Flex_BG ialloc awareness V2.

fs/ext4/balloc.c +1 -0

@@ -600,6 +600,7 @@ void ext4_free_blocks_sb(handle_t *handle, struct super_block *sb,
 	struct ext4_sb_info *sbi;
 	int err = 0, ret;
 	ext4_grpblk_t group_freed;
+	ext4_group_t flex_group;
 
 	*pdquot_freed_blocks = 0;
 	sbi = EXT4_SB(sb);

fs/ext4/balloc.c +1 -0

@@ -1610,6 +1619,7 @@ ext4_fsblk_t ext4_new_blocks_old(handle_t *handle, struct inode *inode,
 	unsigned short windowsz = 0;
 	ext4_group_t ngroups;
 	unsigned long num = *count;
+	ext4_group_t flex_group;
 
 	*errp = -ENOSPC;
 	sb = inode->i_sb;

fs/ext4/ialloc.c +1 -0

@@ -158,6 +158,7 @@ void ext4_free_inode (handle_t *handle, struct inode * inode)
 	struct ext4_super_block * es;
 	struct ext4_sb_info *sbi;
 	int fatal = 0, err;
+	ext4_group_t flex_group;
 
 	if (atomic_read(&inode->i_count) > 1) {
 		printk ("ext4_free_inode: inode has count=%d\n",

include/linux/ext4_fs.h +9 -0

+{
+	return block_group >> sbi->s_groups_per_flex_shift;
+}
+
+static inline unsigned int ext4_flex_bg_size(struct ext4_sb_info *sbi)
+{
+	return 1 << sbi->s_groups_per_flex_shift;
+}
+
 #define ext4_std_error(sb, errno)				\
 do {								\
 	if ((errno))						\

lib/ext2fs/alloc_tables.c +1 -0

+	if (EXT2_HAS_INCOMPAT_FEATURE (fs->super, 

Quoted 12 lines · Jose R. Santos

fs/ext4/balloc.c +1 -0

@@ -600,6 +600,7 @@ void ext4_free_blocks_sb(handle_t *handle, struct super_block *sb,
 	struct ext4_sb_info *sbi;
 	int err = 0, ret;
 	ext4_grpblk_t group_freed;
+	ext4_group_t flex_group;
 
 	*pdquot_freed_blocks = 0;
 	sbi = EXT4_SB(sb);

@@ -745,6 +746,14 @@ do_more:
 	spin_unlock(sb_bgl_lock(sbi, block_group));
 	percpu_counter_add(&sbi->s_freeblocks_counter, count);
 

fs/ext4/balloc.c do_more: · line 755

Quoted 7 lines

fs/ext4/balloc.c +7 -0

+	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG) &&
+	    sbi->s_groups_per_flex_shift) {
+		flex_group = ext4_flex_group(sbi, block_group);
+		spin_lock(sb_bgl_lock(sbi, flex_group));
+		sbi->s_flex_groups[flex_group].free_blocks += count;
+		spin_unlock(sb_bgl_lock(sbi, flex_group));
+	}

In general, I prefer to keep variables in as local a scope as possible. In this case, flex_group could be declared inside the "if (EXT4_HAS_INCOMPAT" check.

Quoted 5 lines

fs/ext4/ialloc.c +5 -0

+#define free_block_ratio 10
+
+int find_group_flex(struct super_block *sb, struct inode *parent)
+{
+	n_fbg_groups = (sbi->s_groups_count + flex_size - 1) / flex_size;

fs/ext4/ialloc.c static int find_group_dir(struct super_block *sb, struct inode *parent, · line 323

Quoted 4 lines

fs/ext4/ialloc.c +4 -0

+	best_flex = parent_fbg_group;
+
+find_close_to_parent:
+	flex_freeb_ratio = flex_group[best_flex].free_blocks*100/blocks_per_flex;

There is no particular reason that this ratio needs to be "*100", it could just as easily be a fraction of 256 and make the multiply into a shift. The free_block_ratio would be 26 in that case.

fs/ext4/ialloc.c static int find_group_dir(struct super_block *sb, struct inode *parent, · line 335

Quoted 3 lines

fs/ext4/ialloc.c +3 -0

+	for (i = 0; i < n_fbg_groups; i++) {
+		if (i == parent_fbg_group || i == parent_fbg_group - 1)
+			continue;

It seems this scans flex groups the way we used to scan groups?

fs/ext4/ialloc.c static int find_group_dir(struct super_block *sb, struct inode *parent, · line 354

Quoted 3 lines

fs/ext4/ialloc.c +3 -0

+found_flexbg:
+	for (i = best_flex * flex_size; i < ngroups &&
+		     i < (best_flex + 1) * flex_size; i++) {

And now that we've found a suitable flex group, we need to find which block group therein has some free inodes...

fs/ext4/super.c static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es, · line 1428

Quoted 3 lines

fs/ext4/super.c +2 -0

+static int ext4_fill_flex_info(struct super_block *sb)
+{

It still seems desirable to have a single per-group array instead of

include/linux/ext4_fs.h struct ext4_super_block { · line 634

Quoted 6 lines

include/linux/ext4_fs.h +1 -1

@@ -622,7 +631,9 @@ struct ext4_super_block {
 	__le16  s_mmp_interval;         /* # seconds to wait in MMP checking */
 	__le64  s_mmp_block;            /* Block for multi-mount protection */
 	__le32  s_raid_stripe_width;    /* blocks on all data disks (N*stride)*/
-	__u32   s_reserved[163];        /* Padding to the end of the block */
+	__le16	s_flex_bg_size;		/* FLEX_BG group size */

Shouldn't this be "s_flex_bg_bits"?

Quoted 23 lines

include/linux/ext4_fs.h +9 -0

+{
+	return block_group >> sbi->s_groups_per_flex_shift;
+}
+
+static inline unsigned int ext4_flex_bg_size(struct ext4_sb_info *sbi)
+{
+	return 1 << sbi->s_groups_per_flex_shift;
+}
+
 #define ext4_std_error(sb, errno)				\
 do {								\
 	if ((errno))						\

lib/ext2fs/alloc_tables.c +7 -0

+	if (EXT2_HAS_INCOMPAT_FEATURE (fs->super, 
+				       EXT4_FEATURE_INCOMPAT_FLEX_BG)) 
+		ext2fs_allocate_flex_groups(fs);
+	
+	else {
+		for (i = 0; i < fs->group_desc_count; i++) {
+			retval = ext2fs_allocate_group_table(fs, i, fs->block_map);

lib/ext2fs/alloc_tables.c errcode_t ext2fs_allocate_tables(ext2_filsys fs) · line 238

Quoted 4 lines

lib/ext2fs/alloc_tables.c +3 -0

+			if (retval)
+				return retval;
+		}
 	}

My preference would be to have "if (EXT2_HAS_INCOMPAT...) { ... } else {" (i.e. add { } for the first part) since there are { } on the second part, and it is just easier to read.

lib/ext2fs/alloc_tables.c static void PRS(int argc, char *argv[]) · line 1049

Quoted 6 lines

lib/ext2fs/alloc_tables.c +4 -0

@@ -1045,6 +1046,19 @@ static void PRS(int argc, char *argv[])
+			if ((flex_bg_size & (flex_bg_size-1)) != 0) {
+				com_err(program_name, 0,
+					_("Flex_BG size must be a power of 2"));
+				exit(1);

If flex_bg_size is a power of two then there isn't any need to store anything except __u8 s_flex_bg_bits in the superblock.

lib/ext2fs/alloc_tables.c static void PRS(int argc, char *argv[]) · line 1460

Quoted 4 lines

lib/ext2fs/alloc_tables.c +3 -0

@@ -1444,6 +1458,10 @@ static void PRS(int argc, char *argv[])
+	if(flex_bg_size) {
+		fs_param.s_flex_bg_size = ext2fs_swab16(flex_bg_size);
+	}

Space between if and (, and no need for braces for a single line body.

It would also be nice to get a m_flexbg test case along with this patch that (at minimum) creates a filesystem with flexbg enabled, and then runs e2fsck on it. This was broken for the lazy_bg feature for a long time, so it makes sense to add a test to verify each new feature has some basic functionality. If the f_random_corruption test is in the git tree, it would be good to add the flex_bg option to the list of possible feature combinations to test.

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

Quoted 3 lines · Andreas Dilger

There is no particular reason that this ratio needs to be "*100", it could just as easily be a fraction of 256 and make the multiply into a shift. The free_block_ratio would be 26 in that case.

The idea here is to reserve 10% (free_block_ratio) of free blocks in a flexbg for allocation of new files and expansion of existing one. The "*100" make the math here easy but this still something that need to be tune further. I'm sure we can do this in a series of shifts, just haven't spent the time thinking of a clever way to do this.

Quoted 8 lines

include/linux/ext4_fs.h struct ext4_super_block { · line 634

Quoted 6 lines

include/linux/ext4_fs.h +1 -1

@@ -622,7 +631,9 @@ struct ext4_super_block {
 	__le16  s_mmp_interval;         /* # seconds to wait in MMP checking */
 	__le64  s_mmp_block;            /* Block for multi-mount protection */
 	__le32  s_raid_stripe_width;    /* blocks on all data disks (N*stride)*/
-	__u32   s_reserved[163];        /* Padding to the end of the block */
+	__le16	s_flex_bg_size;		/* FLEX_BG group size */

Shouldn't this be "s_flex_bg_bits"?

I debated whether to store this as the s_flex_bg_size and calculate the bits during the filesystem mount time or just stored the bit in the super block to begging with. The reason I stored the size is that it seemed more in line with the other fields in the super block. I don't mind either way since this is more of a style issue, although saving an extra 8bits in the super block may be good enough reason to change it.

Quoted 3 lines

My preference would be to have "if (EXT2_HAS_INCOMPAT...) { ... } else {" (i.e. add { } for the first part) since there are { } on the second part, and it is just easier to read.

Mine too, but checkpatch complained about this. :)

Quoted 5 lines · Jose R. Santos

Quoted 3 lines · Andreas Dilger

There is no particular reason that this ratio needs to be "*100", it could just as easily be a fraction of 256 and make the multiply into a shift. The free_block_ratio would be 26 in that case.

Quoted 5 lines

The idea here is to reserve 10% (free_block_ratio) of free blocks in a flexbg for allocation of new files and expansion of existing one. The "*100" make the math here easy but this still something that need to be tune further. I'm sure we can do this in a series of shifts, just haven't spent the time thinking of a clever way to do this.

This is a common misconception for code to have 10% mean 10 / 100. It is just as good to have 26/256

Quoted 9 lines

Quoted 8 lines

include/linux/ext4_fs.h struct ext4_super_block { · line 634

Quoted 6 lines

include/linux/ext4_fs.h +1 -1

@@ -622,7 +631,9 @@ struct ext4_super_block {
 	__le16  s_mmp_interval;         /* # seconds to wait in MMP checking */
 	__le64  s_mmp_block;            /* Block for multi-mount protection */
 	__le32  s_raid_stripe_width;    /* blocks on all data disks (N*stride)*/
-	__u32   s_reserved[163];        /* Padding to the end of the block */
+	__le16	s_flex_bg_size;		/* FLEX_BG group size */

Shouldn't this be "s_flex_bg_bits"?

Quoted 6 lines

I debated whether to store this as the s_flex_bg_size and calculate the bits during the filesystem mount time or just stored the bit in the super block to begging with. The reason I stored the size is that it seemed more in line with the other fields in the super block. I don't mind either way since this is more of a style issue, although saving an extra 8bits in the super block may be good enough reason to change it.

I'd think being able to avoid the divide for every inode allocation is more important than 8 bits in the superblock.

Quoted 5 lines

Quoted 3 lines

My preference would be to have "if (EXT2_HAS_INCOMPAT...) { ... } else {" (i.e. add { } for the first part) since there are { } on the second part, and it is just easier to read.

Mine too, but checkpatch complained about this. :)

Time to fix checkpatch it would seem.

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

Quoted 2 lines

I'd think being able to avoid the divide for every inode allocation is more important than 8 bits in the superblock.

We already avoid the divide since what we store in the sbi IS the bits which are calculated at mount time for each fs. Base on the other fields in the super block struct, I decided to put explicit size of the flexbg in the super block. The kernel code can decide how best to use that number which in this case its used to calculate the number of bits in order to avoid doing divides.

So this is really a styling issue in how to record data in the super block. The only technical issue with this is whether it's important to save those extra 8 bits in the super block struct.

Quoted 10 lines · Jose R. Santos

Quoted 2 lines

I'd think being able to avoid the divide for every inode allocation is more important than 8 bits in the superblock.

We already avoid the divide since what we store in the sbi IS the bits which are calculated at mount time for each fs. Base on the other fields in the super block struct, I decided to put explicit size of the flexbg in the super block. The kernel code can decide how best to use that number which in this case its used to calculate the number of bits in order to avoid doing divides.

Quoted 3 lines

So this is really a styling issue in how to record data in the super block. The only technical issue with this is whether it's important to save those extra 8 bits in the super block struct.

Well, if it is stored in the superblock as a non-power-of-two value, then there always exists the possibility that it is set incorrectly (maybe by a version of mke2fs that doesn't verify this) and the code will not do the right thing. Storing it in bits (as is done with e.g. s_log_block_size and s_log_frag_size) ensures there is no possibility of a value that isn't a power-of-two.

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

Now, storing the bits only guaranties that the flexbg size is always a power-of-two and does not guarantee that the super block flexbg size represents the actual meta-data grouping on disk. For this we need to verify that the bitmap offsets match what the super block reports. It may be an unlikely scenario, but it may be worth it to check this as well at mount time.

Quoted 6 lines · Jose R. Santos

Now, storing the bits only guaranties that the flexbg size is always a power-of-two and does not guarantee that the super block flexbg size represents the actual meta-data grouping on disk. For this we need to verify that the bitmap offsets match what the super block reports. It may be an unlikely scenario, but it may be worth it to check this as well at mount time.

I'm not sure what you mean... Isn't the flexbg size just a count of the number of block groups? If it is always a power of two, and the groups per metabg is always a power of two (it is) then they will always be even multiples.

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

... if the value in the super block is corrupted and
does not represent the actual flexbg size, the inode allocation will behave in weird unexpected ways. Just as we check that the bitmaps are within the block group range (when not using flexbg), we should probably sanity check the size of the flexbg as reported in the super block.

Or do you believe the check is unnecessary?

Well, I can imagine in some cases that the flexbg will not be completely contiguous on disk (e.g. after a filesystem resize, if there are bad blocks, etc). As long as the group descriptors themselves are correct (i.e. referencing valid bitmaps/itable) then it shouldn't cause a mount failure if the per-group data isn't strictly aligned according to the superblock flexbg count.

We would need to validate the group descriptor separately though (e.g. group checksums).

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