Date:	Tue, 24 Feb 2009 10:27:34 -0500
From:	Theodore Tso <tytso@....edu>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH, RFC] ext4: New inode/block allocation algorithms for
	flex_bg filesystems

On Tue, Feb 24, 2009 at 02:29:31PM +0530, Aneesh Kumar K.V wrote:
> >  	/* OK. use inode's group */
> > -	bg_start = (ei->i_block_group * EXT4_BLOCKS_PER_GROUP(inode->i_sb)) +
> > +	block_group = ei->i_block_group;
> > +	if (flex_size >= 4) {
> > +		block_group &= ~(flex_size-1);
> > +		if (S_ISREG(inode->i_mode))
> > +			block_group++;
> > +	}
> 
> 
> Can you explain why we select 4 here ?
> 
> Also add a comment explaining directory/symlink block allocation goes to
> first group of flex_bg and regular files goes to second group and which
> type of workload that would help ? You have the comment in commit message. 

Yeah, I'll add a comment there.

> > +	/* 
> > +	 * If we are doing delayed allocation, we don't need take
> > +	 * colour into account.
> > +	 */
> > +	if (test_opt(inode->i_sb, DELALLOC))
> > +		return bg_start;
> > +
> 
> Again why we don't want to look at colour for delayed allocation ?

If we're doing delayed allocation, we'll be allocating data blocks in
large chunks at a time.  Colour is much more important if you have
multiple processes allocating singleton blocks at a time, so you don't
get interleved allocation (i.e, ABABAB or ABCABBCABC).  But if we're
grabbing chunks of blocks at a time, the benefits largely go away, and
in fact, artificially starting at different location depending on the
process id can actually lead to a greater fragmentation of the free
space.

> >  /*
> > + * Helper function for Orlov's allocator; returns critical information 
> > + * for a particular block group or flex_bg
> > + */
> > +struct orlov_stats {
> > +	__u32 free_inodes;
> > +	__u32 free_blocks;
> > +	__u32 used_dirs;  
> > +};
> > +
> > +void get_orlov_stats(struct super_block *sb, ext4_group_t g, 
> > +		       int flex_size, struct orlov_stats *stats)
> > +{
		...
> > +	g *= flex_size;
> 
> Can you add a comment to the function saying g can be flex group number
> or the actual group number depending on flex_size ?. Without that
> comment the above operation can be confusing.

Sure.

> > +/*
> >   * Orlov's allocator for directories.
> >   *
> 
> You can also remove further description about debt and INODE_COST and
> BLOCK_COST
> 

Yep, good point.


> > +	found_flex_bg:
> > +		if (flex_size == 1) {
> > +			*group = grp;
> > +			return 0;
> > +		}
> > +
> > +		grp *= flex_size;
> > +		for (i = 1; i < flex_size; i++) {
> 
> Why we start  with i = 1 ?
> 

I was putting directories in the second bg of flexgroups; thinking
about it some more, there's really no good reason to do that.  I'll
change that back to be one.

> Can you add a comment saying that we just pick the first group with
> free inode because the goal block for rest of the block allocation
> of the file/directory looks at the flex block group number with
> flex_bg. (more details on ext4_ext_find_goal)

I'll do that.

> > +	/*
> > +	 * If we are doing flex_bg style allocation, try to put
> > +	 * special inodes in the first block group; start files and
> > +	 * directories at the 2nd block group in the flex_bg.
> > +	 */
> 
> Why ? Can you explain whether this placing helps any specific work load
> ? or something where you have observed that this placement helps ?  

This was left over from when I was using the inode number to influence
block allocation.  We're not doing this any more, so this should go
away.   Thanks for asking the question.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists