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