by Yongqiang Yang

[permalink] [raw]

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

On Thu, Nov 3, 2011 at 2:29 AM, Andreas Dilger <[email protected]> wrote:
> On 2011-11-01, at 4:53 AM, Robin Dong wrote:
>> From: Robin Dong <[email protected]>
>>
>> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

--
Best Wishes
Yongqiang Yang

2011-11-03 17:56:44

by Andreas Dilger

[permalink] [raw]

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

On 2011-11-03, at 2:50 AM, Yongqiang Yang wrote:
> On Thu, Nov 3, 2011 at 2:29 AM, Andreas Dilger <[email protected]> wrote:
>> On 2011-11-01, at 4:53 AM, Robin Dong wrote:
>>> From: Robin Dong <[email protected]>
>>>
>>> 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