Re: [PATCH 0/6 ]Ext4 journal credits reservation fixes

[Mingming Cao <cmm@us.ibm.com>]

在 2008-08-15五的 23:03 +0530，Aneesh Kumar K.V写道：
> On Tue, Aug 12, 2008 at 09:23:10AM -0700, Mingming Cao wrote:
> > This is a rework of journal credits fix patch in the ext4 patch queue.
> > The patch series contains
> > 
> > - patch 1: helper funtions for journal credits calculation and fix the
> > writepage/write_begin on nonextent files
> > - patch 2: journal credit fix wirtepahe/write_begin for extents files,
> > and migration
> > - patch 3: credit fix for dio, fallocate
> > -patch 4: rebase ext4_da_writepages_rework patch
> > - patch 5: credit fix for delalloc writepages
> > -patch 6: credit fix for defrag
> > 
> > 
> 
> I see this patch is pushed to the patch queue. Below is the review in
> patch form.
> 
> commit 679a6fa1de0bc67c0a444b748696a2f2c22428c7
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Date:   Fri Aug 15 22:13:07 2008 +0530
> 
>     cleanup
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 258cd1a..164c988 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1150,7 +1150,8 @@ extern void ext4_set_inode_flags(struct inode *);
>  extern void ext4_get_inode_flags(struct ext4_inode_info *);
>  extern void ext4_set_aops(struct inode *inode);
>  extern int ext4_writepage_trans_blocks(struct inode *);
> -extern int ext4_meta_trans_blocks(struct inode *, int nrblocks, int idxblocks);
> +extern int ext4_meta_trans_blocks(struct inode *, int nrblocks,
> +					int idxblocks, bool chunk);

This might be the right thing to do , but  I don't see other places in
ext3/4 uses "bool" type for a flag.    I think just to keep the code
consistant,  using "int" as a flag varibale type is quit common, not a
big deal anyway.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0b34998..a843cd3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1568,9 +1568,9 @@ static void ext4_da_release_space(struct inode *inode, int to_free)
>  		 * but since this function is called from invalidate
>  		 * page, it's harmless to return without any action
>  		 */
> -		printk(KERN_INFO "ext4 delalloc try to release %d reserved"
> -			    "blocks for inode %lu, but there is no reserved"
> -			    "data blocks\n", inode->i_ino, to_free);
> +		printk(KERN_INFO "ext4 delalloc try to release %d reserved "
> +			    "blocks for inode %lu, but there is no reserved "
> +			    "data blocks\n", to_free, inode->i_ino);
>  		spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
>  		return;
>  	}

The compile warning has already fixed in patch queue. I wan't sure what
do you mean in your last email about adding a space at the end of each
line?

> @@ -2303,13 +2303,13 @@ static int ext4_da_writepage(struct page *page,
>   * get_block is calculated
>   */
> 
> -#define		EXT4_MAX_WRITEPAGES_SIZE	PAGEVEC_SIZE
> -static int ext4_writepages_trans_blocks(struct inode *inode)
> +static int ext4_da_writepages_trans_blocks(struct inode *inode)
>  {
> -	int bpp = ext4_journal_blocks_per_page(inode);
> -	int max_blocks = EXT4_MAX_WRITEPAGES_SIZE * bpp;
> -
> -	if (max_blocks > EXT4_I(inode)->i_reserved_data_blocks)
> +	int max_blocks;
> +	if (EXT4_I(inode)->i_reserved_data_blocks >
> +				EXT4_BLOCKS_PER_GROUP(inode->i_sb))
> +		max_blocks =  EXT4_BLOCKS_PER_GROUP(inode->i_sb);
> +	else
>  		max_blocks =  EXT4_I(inode)->i_reserved_data_blocks;
> 

Hmm. EXT4_BLOCKS_PER_GROUP could still be too big for a single
transaction.  Default EXT4_BLOCKS_PER_GROUP is 32kblocks, that's could
still overflow the max capacity of a single journal log (default is 1/4
journal log size(128M)).

In fact, even if the EXT4_BLOCKS_PER_GROUP is the right value, and you
only limit the credit reservation here, later, we still need to  limit
the logical page extent to flush in mpage_da_writepages() to not exceed
the previously reserved credits.


> @@ -4459,13 +4459,19 @@ int ext4_getattr(struct vfsmount *mnt, struct dentry *dentry,
>   * over different block groups
>   *
>   * Also account for superblock, inode, quota and xattr blocks
> + * This doesn't account for the blocks needed for journaled
> + * data blocks.
>   */
> -int ext4_meta_trans_blocks(struct inode* inode, int nrblocks, int idxblocks)
> +int ext4_meta_trans_blocks(struct inode* inode,
> +				int nrblocks, int idxblocks, bool chunk)
>  {
>  	int groups, gdpblocks;
>  	int ret = 0;
> 
> -	groups = nrblocks + idxblocks;
> +	if (chunk)
> +		groups = 1 + idxblocks;

Not exactly right, the datablocks could spread over multiple block
groups with flex_bg.

if (chunk)
	groups = (nrblocks + 1 )/EXT4_BLOCKS_PER_GROUP + idxblocks;

> +	else
> +		groups = nrblocks + idxblocks;
>  	gdpblocks = groups;
>  	if (groups > EXT4_SB(inode->i_sb)->s_groups_count)
>  		groups = EXT4_SB(inode->i_sb)->s_groups_count;


I will fix the spell errors and update the patches.


--
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