Re: [PATCH -v2] ext4: Use inode preallocation with -o noextents

[Shen Feng <shen@cn.fujitsu.com>]

Aneesh Kumar K.V Wrote:
> On Tue, Jun 17, 2008 at 05:42:57PM +0800, Shen Feng wrote:
>>
>> Shen Feng Wrote:
>>> Mingming Cao Wrote:
>>>> On Thu, 2008-06-05 at 14:13 +0530, Aneesh Kumar K.V wrote:
>>>>> On Wed, Jun 04, 2008 at 11:22:20PM -0400, Theodore Tso wrote:
>>>>>> when I moved this patch to the beginning of the unstable patch queue,
>>>>>> it didn't apply.  When I tried to look at it, my head started
>>>>>> spinning.  The patch applied to the wrong function, apparently,
>>>>>> because there is so much code duplication "patch" got confused.  I
>>>>>> can't blame it, though, because *I* got confused.  
>>>>>>
>>> ...snip...
>>>
>>>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>>>> index 09922ae..a810a21 100644
>>>>> --- a/fs/ext4/mballoc.c
>>>>> +++ b/fs/ext4/mballoc.c
>>>>> @@ -4048,7 +4048,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>>>>>  	sbi = EXT4_SB(sb);
>>>>>
>>>>>  	if (!test_opt(sb, MBALLOC)) {
>>>>> -		block = ext4_new_blocks_old(handle, ar->inode, ar->goal,
>>>>> +		block = ext4_orlov_new_blocks(handle, ar->inode, ar->goal,
>>>>>  					    &(ar->len), errp);
>>>>>  		return block;
>>>>>  	}
>>>> when we get to ext4_mb_new_blocks, don't we already tested MBALLOC is
>>>> turned on?
>>>>
>>> ext4_ext_get_blocks calls ext4_mb_new_blocks. So we have to check this.
>>> So maybe ext4_ext_get_blocks should call ext4_new_blocks and 
>>> we can remove this check.
>>>
>> How about this patch?
>> I tested it with bonnie++ and ltp fs test using mballoc and nonomballoc
>> options.
> 
> NACK. you need the ar.lleft/pleft and ar.lright/pright values so that
> mballoc can merge the requests properly. Look at
> ext4_mb_normalize_request . So you can't do the below change.
> 

Thank you for your explanation.

OK. Now I understand why the call to ext4_new_blocks_old in ext4_mb_new_blocks
is necessary.

The ar.lleft/pleft and ar.lright/pright values are very useful in the case 
that extent and mballoc are used at the same time.
But in other case, i.e. noextent & mballoc, extent & nomballoc, noextent &
nomballoc, these values are useless.

Is that right?

-Shen Feng

> 
>> Signed-off-by: Shen Feng <shen@cn.fujitsu.com>
>> ---
>>  fs/ext4/extents.c |   25 +++++++++----------------
>>  fs/ext4/mballoc.c |    6 ------
>>  2 files changed, 9 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 47929c4..3f6be32 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -2528,7 +2528,8 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>>  	ext4_fsblk_t goal, newblock;
>>  	int err = 0, depth, ret;
>>  	unsigned long allocated = 0;
>> -	struct ext4_allocation_request ar;
>> +	ext4_lblk_t lleft, lright;
>> +	ext4_fsblk_t pleft, pright;
>>
>>  	__clear_bit(BH_New, &bh_result->b_state);
>>  	ext_debug("blocks %u/%lu requested for inode %u\n",
>> @@ -2653,12 +2654,12 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>>  		ext4_init_block_alloc_info(inode);
>>
>>  	/* find neighbour allocated blocks */
>> -	ar.lleft = iblock;
>> -	err = ext4_ext_search_left(inode, path, &ar.lleft, &ar.pleft);
>> +	lleft = iblock;
>> +	err = ext4_ext_search_left(inode, path, &lleft, &pleft);
>>  	if (err)
>>  		goto out2;
>> -	ar.lright = iblock;
>> -	err = ext4_ext_search_right(inode, path, &ar.lright, &ar.pright);
>> +	lright = iblock;
>> +	err = ext4_ext_search_right(inode, path, &lright, &pright);
>>  	if (err)
>>  		goto out2;
>>
>> @@ -2685,16 +2686,8 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>>  		allocated = max_blocks;
>>
>>  	/* allocate new block */
>> -	ar.inode = inode;
>> -	ar.goal = ext4_ext_find_goal(inode, path, iblock);
>> -	ar.logical = iblock;
>> -	ar.len = allocated;
>> -	if (S_ISREG(inode->i_mode))
>> -		ar.flags = EXT4_MB_HINT_DATA;
>> -	else
>> -		/* disable in-core preallocation for non-regular files */
>> -		ar.flags = 0;
>> -	newblock = ext4_mb_new_blocks(handle, &ar, &err);
>> +	goal = ext4_ext_find_goal(inode, path, iblock);
>> +	newblock = ext4_new_blocks(handle, inode, goal, &allocated, &err);
>>  	if (!newblock)
>>  		goto out2;
>>  	ext_debug("allocate new block: goal %llu, found %llu/%lu\n",
>> @@ -2702,7 +2695,7 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
>>
>>  	/* try to insert new extent into found leaf and return */
>>  	ext4_ext_store_pblock(&newex, newblock);
>> -	newex.ee_len = cpu_to_le16(ar.len);
>> +	newex.ee_len = cpu_to_le16(allocated);
>>  	if (create == EXT4_CREATE_UNINITIALIZED_EXT)  /* Mark uninitialized */
>>  		ext4_ext_mark_uninitialized(&newex);
>>  	err = ext4_ext_insert_extent(handle, inode, path, &newex);
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index c9900aa..bc82d39 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -4010,12 +4010,6 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>>  	sb = ar->inode->i_sb;
>>  	sbi = EXT4_SB(sb);
>>
>> -	if (!test_opt(sb, MBALLOC)) {
>> -		block = ext4_new_blocks_old(handle, ar->inode, ar->goal,
>> -					    &(ar->len), errp);
>> -		return block;
>> -	}
>> -
>>  	while (ar->len && DQUOT_ALLOC_BLOCK(ar->inode, ar->len)) {
>>  		ar->flags |= EXT4_MB_HINT_NOPREALLOC;
>>  		ar->len--;
>> -- 1.5.5.1 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>