On 2011-11-03, at 2:50 AM, Yongqiang Yang wrote:
> 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.
I hadn't actually looked at the later patches in the series yet. In
that case, I'm happy to allow bigalloc to continue with its current
approach of cluster size > blocksize, but extents are measured in blocks,
and use the support support you've added for blocksize > PAGE_SIZE by
scaling the in-memory "block" addresses to match PAGE_SIZE (along with
other fixes here to handle zeroing of neighbouring pages in the block).
Essentially, this would be very similar to internally setting the cluster
size to blocksize >> PAGE_SHIFT even though this isn't set in the superblock
at format time.
The other comments below should still be addressed.
>>> 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
