Date:	Thu, 3 Nov 2011 11:57:11 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Yongqiang Yang <xiaoqiangnk@...il.com>
Cc:	Robin Dong <hao.bigrat@...il.com>, linux-ext4@...r.kernel.org,
	Robin Dong <sanbai@...bao.com>
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 <adilger@...ger.ca> wrote:
>> On 2011-11-01, at 4:53 AM, Robin Dong wrote:
>>> From: Robin Dong <sanbai@...bao.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.

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists