Re: [PATCH] ext4: optimize ext4_check_dir_entry() with unlikely() annotations

[Eric Sandeen <sandeen@redhat.com>]

On 12/19/10 9:16 PM, Theodore Ts'o wrote:
> This function gets called a lot for large directories, and the answer
> is almost always "no, no, there's no problem".  This means using
> unlikely() is a good thing.

testing bonnie++ file creates on a ramdisk should give a good idea
of how much or if this helps...

-Eric

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/dir.c   |   42 ++++++++++++++++++++++++------------------
>  fs/ext4/ext4.h  |    3 ++-
>  fs/ext4/namei.c |   14 +++++++-------
>  3 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index ece76fb..bd5d74d 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -60,7 +60,11 @@ static unsigned char get_dtype(struct super_block *sb, int filetype)
>  	return (ext4_filetype_table[filetype]);
>  }
>  
> -
> +/*
> + * Return 0 if the directory entry is OK, and 1 if there is a problem
> + *
> + * Note: this is the opposite of what ext2 and ext3 historically returned...
> + */
>  int __ext4_check_dir_entry(const char *function, unsigned int line,
>  			   struct inode *dir,
>  			   struct ext4_dir_entry_2 *de,
> @@ -71,26 +75,28 @@ int __ext4_check_dir_entry(const char *function, unsigned int line,
>  	const int rlen = ext4_rec_len_from_disk(de->rec_len,
>  						dir->i_sb->s_blocksize);
>  
> -	if (rlen < EXT4_DIR_REC_LEN(1))
> +	if (unlikely(rlen < EXT4_DIR_REC_LEN(1)))
>  		error_msg = "rec_len is smaller than minimal";
> -	else if (rlen % 4 != 0)
> +	else if (unlikely(rlen % 4 != 0))
>  		error_msg = "rec_len % 4 != 0";
> -	else if (rlen < EXT4_DIR_REC_LEN(de->name_len))
> +	else if (unlikely(rlen < EXT4_DIR_REC_LEN(de->name_len)))
>  		error_msg = "rec_len is too small for name_len";
> -	else if (((char *) de - bh->b_data) + rlen > dir->i_sb->s_blocksize)
> +	else if (unlikely(((char *) de - bh->b_data) + rlen >
> +			  dir->i_sb->s_blocksize))
>  		error_msg = "directory entry across blocks";
> -	else if (le32_to_cpu(de->inode) >
> -			le32_to_cpu(EXT4_SB(dir->i_sb)->s_es->s_inodes_count))
> +	else if (unlikely(le32_to_cpu(de->inode) >
> +			le32_to_cpu(EXT4_SB(dir->i_sb)->s_es->s_inodes_count)))
>  		error_msg = "inode out of bounds";
> +	else
> +		return 0;
>  
> -	if (error_msg != NULL)
> -		ext4_error_inode(dir, function, line, bh->b_blocknr,
> -			"bad entry in directory: %s - "
> -			"offset=%u(%u), inode=%u, rec_len=%d, name_len=%d",
> -			error_msg, (unsigned) (offset%bh->b_size), offset,
> -			le32_to_cpu(de->inode),
> -			rlen, de->name_len);
> -	return error_msg == NULL ? 1 : 0;
> +	ext4_error_inode(dir, function, line, bh->b_blocknr,
> +			 "bad entry in directory: %s - "
> +			 "offset=%u(%u), inode=%u, rec_len=%d, name_len=%d",
> +			 error_msg, (unsigned) (offset%bh->b_size), offset,
> +			 le32_to_cpu(de->inode),
> +			 rlen, de->name_len);
> +	return 1;
>  }
>  
>  static int ext4_readdir(struct file *filp,
> @@ -194,8 +200,8 @@ revalidate:
>  		while (!error && filp->f_pos < inode->i_size
>  		       && offset < sb->s_blocksize) {
>  			de = (struct ext4_dir_entry_2 *) (bh->b_data + offset);
> -			if (!ext4_check_dir_entry(inode, de,
> -						  bh, offset)) {
> +			if (ext4_check_dir_entry(inode, de,
> +						 bh, offset)) {
>  				/*
>  				 * On error, skip the f_pos to the next block
>  				 */
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 17baecb..49f1cea 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1639,7 +1639,8 @@ extern int __ext4_check_dir_entry(const char *, unsigned int, struct inode *,
>  				  struct ext4_dir_entry_2 *,
>  				  struct buffer_head *, unsigned int);
>  #define ext4_check_dir_entry(dir, de, bh, offset) \
> -	__ext4_check_dir_entry(__func__, __LINE__, (dir), (de), (bh), (offset))
> +	unlikely(__ext4_check_dir_entry(__func__, __LINE__, (dir), (de), \
> +					(bh), (offset)))
>  extern int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
>  				    __u32 minor_hash,
>  				    struct ext4_dir_entry_2 *dirent);
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 2030864..e275464 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -581,9 +581,9 @@ static int htree_dirblock_to_tree(struct file *dir_file,
>  					   dir->i_sb->s_blocksize -
>  					   EXT4_DIR_REC_LEN(0));
>  	for (; de < top; de = ext4_next_entry(de, dir->i_sb->s_blocksize)) {
> -		if (!ext4_check_dir_entry(dir, de, bh,
> -					(block<<EXT4_BLOCK_SIZE_BITS(dir->i_sb))
> -						+((char *)de - bh->b_data))) {
> +		if (ext4_check_dir_entry(dir, de, bh,
> +				(block<<EXT4_BLOCK_SIZE_BITS(dir->i_sb))
> +					 + ((char *)de - bh->b_data))) {
>  			/* On error, skip the f_pos to the next block. */
>  			dir_file->f_pos = (dir_file->f_pos |
>  					(dir->i_sb->s_blocksize - 1)) + 1;
> @@ -820,7 +820,7 @@ static inline int search_dirblock(struct buffer_head *bh,
>  		if ((char *) de + namelen <= dlimit &&
>  		    ext4_match (namelen, name, de)) {
>  			/* found a match - just to be sure, do a full check */
> -			if (!ext4_check_dir_entry(dir, de, bh, offset))
> +			if (ext4_check_dir_entry(dir, de, bh, offset))
>  				return -1;
>  			*res_dir = de;
>  			return 1;
> @@ -1269,7 +1269,7 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry,
>  		de = (struct ext4_dir_entry_2 *)bh->b_data;
>  		top = bh->b_data + blocksize - reclen;
>  		while ((char *) de <= top) {
> -			if (!ext4_check_dir_entry(dir, de, bh, offset))
> +			if (ext4_check_dir_entry(dir, de, bh, offset))
>  				return -EIO;
>  			if (ext4_match(namelen, name, de))
>  				return -EEXIST;
> @@ -1636,7 +1636,7 @@ static int ext4_delete_entry(handle_t *handle,
>  	pde = NULL;
>  	de = (struct ext4_dir_entry_2 *) bh->b_data;
>  	while (i < bh->b_size) {
> -		if (!ext4_check_dir_entry(dir, de, bh, i))
> +		if (ext4_check_dir_entry(dir, de, bh, i))
>  			return -EIO;
>  		if (de == de_del)  {
>  			BUFFER_TRACE(bh, "get_write_access");
> @@ -1919,7 +1919,7 @@ static int empty_dir(struct inode *inode)
>  			}
>  			de = (struct ext4_dir_entry_2 *) bh->b_data;
>  		}
> -		if (!ext4_check_dir_entry(inode, de, bh, offset)) {
> +		if (ext4_check_dir_entry(inode, de, bh, offset)) {
>  			de = (struct ext4_dir_entry_2 *)(bh->b_data +
>  							 sb->s_blocksize);
>  			offset = (offset | (sb->s_blocksize - 1)) + 1;