在 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@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
