On Thu, Nov 3, 2011 at 2:29 AM, Andreas Dilger <adilger@xxxxxxxxx> wrote:
> On 2011-11-01, at 4:53 AM, Robin Dong wrote:
>> From: Robin Dong <sanbai@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
