Re: [PATCH 1/8 bigalloc] ext4: get blocks from ext4_ext_get_actual_len

[Yongqiang Yang <xiaoqiangnk@gmail.com>]

On Thu, Nov 3, 2011 at 2:29 AM, Andreas Dilger <adilger@dilger.ca> wrote:
> On 2011-11-01, at 4:53 AM, Robin Dong wrote:
>> From: Robin Dong <sanbai@taobao.com>
>>
>> Since ee_len's unit change to cluster, it need to transform from clusters
>> to blocks when use ext4_ext_get_actual_len.
>
> Robin,
> thanks for working on and submitting these patches so quickly.
>
>> struct ext4_extent {
>>       __le32  ee_block;       /* first logical block extent covers */
>> -     __le16  ee_len;         /* number of blocks covered by extent */
>> +     __le16  ee_len;         /* number of clusters covered by extent */
>
> It would make sense that ee_block should also be changed to be measured
> in units of clusters instead of blocks, since there is no value to
> using extents with cluster size if they are not also cluster aligned.
>
> I think this would also simplify some of the code.
Actually, after these patches are applied, both logical block and
physical block are all cluster sized.  So I have a suggestion that we
can simply tell users that ext4 can use large size block rather than
cluster.

Yongqiang.

>
>> static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
>> {
>> +     struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>
> Why allocate "*sbi" on the stack in all of these functions for a
> single use?  This provides no benefit, but can increase the stack
> usage considerably due to repeated allocations.
>
>>       ext4_fsblk_t block = ext4_ext_pblock(ext);
>> +     int len = EXT4_C2B(sbi, ext4_ext_get_actual_len(ext));
>
> It probably makes more sense to pass "sb" or "sbi" as a parameter to
> ext4_ext_get_actual_len() and then have it return the proper length
> in blocks (i.e. call EXT4_C2B() internally), which will simplify all
> of the callers and avoid potential bugs if some code does not use it.
>
>> @@ -1523,7 +1534,7 @@ ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
>>       ext1_ee_len = ext4_ext_get_actual_len(ex1);
>>       ext2_ee_len = ext4_ext_get_actual_len(ex2);
>>
>> -     if (le32_to_cpu(ex1->ee_block) + ext1_ee_len !=
>> +     if (le32_to_cpu(ex1->ee_block) + EXT4_C2B(sbi, ext1_ee_len) !=
>>                       le32_to_cpu(ex2->ee_block))
>
> If both ee_len and ee_block are in the same units (blocks or clusters),
> then there is no need to convert units for this function at all.
>
>
> Cheers, Andreas
>
>
>
>
>
> --
> 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
>



-- 
Best Wishes
Yongqiang Yang
--
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