Re: [PATCH 3/3] ext4: Use readahead when reading an inode from the inode table

[Theodore Tso <tytso@mit.edu>]

On Wed, Oct 01, 2008 at 07:17:46PM -0700, Andreas Dilger wrote:
> I'd actually thought that having a tunable in units of "kB" is better
> than blocks, since userspace shouldn't have to know the filesystem
> block size to tune readahead for a device.  Depending on the block size
> this tunable can vary by 64x the amount of readahead (1kB vs. 64kB blocks).

True, but realistically, except for benchmarks and die-hards that want
to get the last bit of performance out, I really doubt that many
people will be tweaking this tunable anyway.  Mainly I'm just feeling
rather lazy and am not sure it's worth it.  :-)

> > +		 * If we need to do any I/O, try to readahead up to 16
> > +		 * blocks from the inode table.
> 
> Comment is out of date.

Good point, thanks, I'll fix it.

> 
> > +		if (EXT4_SB(sb)->s_inode_readahead_blks) {
> > +			/* Make sure s_inode_readahead_blks is a power of 2 */
> > +			while (EXT4_SB(sb)->s_inode_readahead_blks &
> > +			       (EXT4_SB(sb)->s_inode_readahead_blks-1))
> > +				EXT4_SB(sb)->s_inode_readahead_blks = 
> > +				   (EXT4_SB(sb)->s_inode_readahead_blks &
> > +				    (EXT4_SB(sb)->s_inode_readahead_blks-1));
> 
> Is there a good reason why the readahead blocks is a power of 2?  Given
> that the blocks are likely NOT contiguous for a directory, nor are they
> aligned to the underlying LUN offsets, I don't think this is a benefit.

What we are reading ahead here are not directory blocks, but the inode
table.  So these are real, physical block numbers that we are dealing
with here, and so aligning the read requests to powers of two can be very
helpful.

> In any case, any tweaking of s_inode_readahead_blks should probably be
> done at the time it is set instead of each time an inode is read.

The parameter s_inode_readahead_blks can be set via a proc setting, so
it would be a lot more trouble to tweak the parameter at the time when
it is set.  (It could be done, but it means we could no longer use the
generic proc functions.)  As it turns out, if s_inode_readahead_blks
is a power of two, the while loop becomes a single conditional branch
instruction, so it's really not that expensive.

> > +			ext4_error(sb, "ext4_get_inode_loc",
> 
> s/ext4_get_inode_loc/__func__/?

Sure.

> > +		case Opt_inode_readahead_blks:
> > +			if (option < 0 || option > 31)
> > +				return 0;
> > +			sbi->s_inode_readahead_blks = option;
> 
> This would appear to limit the inode_readahead_blks to 31 blocks, yet the
> default is 32?  I suspect this is left over from when it was a shift?

Yup, good point.  I didn't notice becuase it's much more convenient to
use the /proc interface, and that's how I did all of my testing.

    	      		     	    	- Ted