Re: [PATCH 17/29] ext4: Better estimate credits needed for ext4_da_writepages()

[Zheng Liu <gnehzuil.liu@gmail.com>]

On Mon, Apr 08, 2013 at 11:32:22PM +0200, Jan Kara wrote:
> We limit the number of blocks written in a single loop of
> ext4_da_writepages() to 64 when inode uses indirect blocks. That is
> unnecessary as credit estimates for mapping logically continguous run of
> blocks is rather low even for inode with indirect blocks. So just lift
> this limitation and properly calculate the number of necessary credits.
> 
> This better credit estimate will also later allow us to always write at
> least a single page in one iteration.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

A minor comment below.  Otherwise the patch looks good to me.
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

> ---
>  fs/ext4/ext4.h    |    3 +-
>  fs/ext4/extents.c |   16 ++++++--------
>  fs/ext4/inode.c   |   58 ++++++++++++++++++++--------------------------------
>  3 files changed, 30 insertions(+), 47 deletions(-)
...
>  static int ext4_da_writepages_trans_blocks(struct inode *inode)
>  {
> -	int max_blocks = EXT4_I(inode)->i_reserved_data_blocks;
> -
> -	/*
> -	 * With non-extent format the journal credit needed to
> -	 * insert nrblocks contiguous block is dependent on
> -	 * number of contiguous block. So we will limit
> -	 * number of contiguous block to a sane value
> -	 */
> -	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) &&
> -	    (max_blocks > EXT4_MAX_TRANS_DATA))
> -		max_blocks = EXT4_MAX_TRANS_DATA;
> +	int bpp = ext4_journal_blocks_per_page(inode);
>  
> -	return ext4_chunk_trans_blocks(inode, max_blocks);
> +	return ext4_meta_trans_blocks(inode,
> +				MAX_WRITEPAGES_EXTENT_LEN + bpp - 1, bpp);

FWIW, MAX_WRITEPAGES_EXTENT_LEN is defined at patch 18/29.  So after
applied this patch, we will get a build error.  So that would be great
if MAX_WRITEPAGES_EXTENT_LEN can be define in this commit.

Regards,
                                                - Zheng

>  }
>  
>  /*
> @@ -4396,11 +4389,12 @@ int ext4_getattr(struct vfsmount *mnt, struct dentry *dentry,
>  	return 0;
>  }
>  
> -static int ext4_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
> +static int ext4_index_trans_blocks(struct inode *inode, int lblocks,
> +				   int pextents)
>  {
>  	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
> -		return ext4_ind_trans_blocks(inode, nrblocks);
> -	return ext4_ext_index_trans_blocks(inode, nrblocks, chunk);
> +		return ext4_ind_trans_blocks(inode, lblocks);
> +	return ext4_ext_index_trans_blocks(inode, pextents);
>  }
>  
>  /*
> @@ -4414,7 +4408,8 @@ static int ext4_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
>   *
>   * Also account for superblock, inode, quota and xattr blocks
>   */
> -static int ext4_meta_trans_blocks(struct inode *inode, int nrblocks, int chunk)
> +static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
> +				  int pextents)
>  {
>  	ext4_group_t groups, ngroups = ext4_get_groups_count(inode->i_sb);
>  	int gdpblocks;
> @@ -4422,14 +4417,10 @@ static int ext4_meta_trans_blocks(struct inode *inode, int nrblocks, int chunk)
>  	int ret = 0;
>  
>  	/*
> -	 * How many index blocks need to touch to modify nrblocks?
> -	 * The "Chunk" flag indicating whether the nrblocks is
> -	 * physically contiguous on disk
> -	 *
> -	 * For Direct IO and fallocate, they calls get_block to allocate
> -	 * one single extent at a time, so they could set the "Chunk" flag
> +	 * How many index blocks need to touch to map @lblocks logical blocks
> +	 * to @pextents physical extents?
>  	 */
> -	idxblocks = ext4_index_trans_blocks(inode, nrblocks, chunk);
> +	idxblocks = ext4_index_trans_blocks(inode, lblocks, pextents);
>  
>  	ret = idxblocks;
>  
> @@ -4437,12 +4428,7 @@ static int ext4_meta_trans_blocks(struct inode *inode, int nrblocks, int chunk)
>  	 * Now let's see how many group bitmaps and group descriptors need
>  	 * to account
>  	 */
> -	groups = idxblocks;
> -	if (chunk)
> -		groups += 1;
> -	else
> -		groups += nrblocks;
> -
> +	groups = idxblocks + pextents;
>  	gdpblocks = groups;
>  	if (groups > ngroups)
>  		groups = ngroups;
> @@ -4473,7 +4459,7 @@ int ext4_writepage_trans_blocks(struct inode *inode)
>  	int bpp = ext4_journal_blocks_per_page(inode);
>  	int ret;
>  
> -	ret = ext4_meta_trans_blocks(inode, bpp, 0);
> +	ret = ext4_meta_trans_blocks(inode, bpp, bpp);
>  
>  	/* Account for data blocks for journalled mode */
>  	if (ext4_should_journal_data(inode))
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html