ext4_ext_calc_credits_for_single_extent() bug?

ext4_ext_calc_credits_for_single_extent() bug?

[Andreas Dilger]

I was looking at the function ext4_ext_calc_credits_for_insert(), because
it appeared similar to a function we were using in Lustre and I wanted to
remove redundant code from our patches.  On closer inspection, however,
that function is doing something strange that wasn't in our original code.

int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int nrblocks,
                                                struct ext4_ext_path *path)
{
        if (path) {
                int depth = ext_depth(inode);
                int ret = 0;

                /* probably there is space in leaf? */
                if (le16_to_cpu(path[depth].p_hdr->eh_entries)
                                < le16_to_cpu(path[depth].p_hdr->eh_max)) {

                        /*
                         *  There are some space in the leaf tree, no
                         *  need to account for leaf block credit
                         *
                         *  bitmaps and block group descriptor blocks
                         *  and other metadat blocks still need to be
                         *  accounted.
                         */
                        /* 1 bitmap, 1 block group descriptor */
                        ret = 2 + EXT4_META_TRANS_BLOCKS(inode->i_sb);
                }
        }

        return ext4_chunk_trans_blocks(inode, nrblocks);
}

It computes "ret" but doesn't actually use it, and then
ext4_chunk_trans_blocks() does nothing but call ext4_meta_trans_blocks(),
which is doing the bulk of the calculation.

It looks like this was committed by Mingming in "ext4: journal credits
reservation fixes for extent file writepage"
ee12b630687d510f6f4b6d4acdc4e267fd4adeda

Ideally we would use the "path" information to generate an accurate block
count for this extent.  Can we just return "ret" in this case?

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: ext4_ext_calc_credits_for_single_extent() bug?

[Aneesh Kumar K.V]

On Tue, Jun 30, 2009 at 12:41:33AM +0200, Andreas Dilger wrote:
> I was looking at the function ext4_ext_calc_credits_for_insert(), because
> it appeared similar to a function we were using in Lustre and I wanted to
> remove redundant code from our patches.  On closer inspection, however,
> that function is doing something strange that wasn't in our original code.
> 
> int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int nrblocks,
>                                                 struct ext4_ext_path *path)
> {
>         if (path) {
>                 int depth = ext_depth(inode);
>                 int ret = 0;
> 
>                 /* probably there is space in leaf? */
>                 if (le16_to_cpu(path[depth].p_hdr->eh_entries)
>                                 < le16_to_cpu(path[depth].p_hdr->eh_max)) {
> 
>                         /*
>                          *  There are some space in the leaf tree, no
>                          *  need to account for leaf block credit
>                          *
>                          *  bitmaps and block group descriptor blocks
>                          *  and other metadat blocks still need to be
>                          *  accounted.
>                          */
>                         /* 1 bitmap, 1 block group descriptor */
>                         ret = 2 + EXT4_META_TRANS_BLOCKS(inode->i_sb);
>                 }
>         }
> 
>         return ext4_chunk_trans_blocks(inode, nrblocks);
> }
> 
> It computes "ret" but doesn't actually use it, and then
> ext4_chunk_trans_blocks() does nothing but call ext4_meta_trans_blocks(),
> which is doing the bulk of the calculation.
> 
> It looks like this was committed by Mingming in "ext4: journal credits
> reservation fixes for extent file writepage"
> ee12b630687d510f6f4b6d4acdc4e267fd4adeda
> 
> Ideally we would use the "path" information to generate an accurate block
> count for this extent.  Can we just return "ret" in this case?
> 

I guess so a return ret is missing. If we have space in the leaf tree
then i guess we can safely add the extent information to the leaf and
update only bitmap and group descriptors.

-aneesh