[PATCH 2/4] xfs: push rounddown_pow_of_two() to after prealloc throttle

[Brian Foster]

On 12/12/2012 07:39 PM, Dave Chinner wrote:
> On Wed, Dec 05, 2012 at 11:47:56AM -0500, Brian Foster wrote:
>> The round down occurs towards the beginning of the function. Push
>> it down after throttling has occurred. This is to support adding
>> further transformations to 'alloc_blocks' that might not preserve
>> power-of-two alignment (and thus could lead to rounding down
>> multiple times).
>>
>> Signed-off-by: Brian Foster <bfoster at redhat.com>
>> ---
>>  fs/xfs/xfs_iomap.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index bd7c060..d381326 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -329,13 +329,11 @@ xfs_iomap_prealloc_size(
>>  		goto check_writeio;
>>  
>>  	/*
>> -	 * rounddown_pow_of_two() returns an undefined result
>> -	 * if we pass in alloc_blocks = 0. Hence the "+ 1" to
>> -	 * ensure we always pass in a non-zero value.
>> +	 * MAXEXTLEN is 21 bits, add one to protect against the rounddown
>> +	 * further down.
>>  	 */
>> -	alloc_blocks = XFS_B_TO_FSB(mp, XFS_ISIZE(ip)) + 1;
>> -	alloc_blocks = XFS_FILEOFF_MIN(MAXEXTLEN,
>> -				rounddown_pow_of_two(alloc_blocks));
>> +	alloc_blocks = XFS_FILEOFF_MIN(MAXEXTLEN + 1,
>> +				XFS_B_TO_FSB(mp, XFS_ISIZE(ip)));
> 
> I suspect this is will lead to a bug - if the round down doesn't
> modify the value when it is MAXEXTLEN + 1, then we are returning a
> value greater than MAXEXTLEN to the caller....
> 

Good catch. I didn't reproduce anything catastrophic in my testing so
far, but this is definitely an unintended change in behavior.

>>  	xfs_icsb_sync_counters(mp, XFS_ICSB_LAZY_COUNT);
>>  	freesp = mp->m_sb.sb_fdblocks;
>> @@ -352,6 +350,8 @@ xfs_iomap_prealloc_size(
>>  	}
>>  	if (shift)
>>  		alloc_blocks >>= shift;
>> +	if (alloc_blocks)
>> +		alloc_blocks = rounddown_pow_of_two(alloc_blocks);
> 
> This needs the comment about rounddown_pow_of_two() and zero values.
> 
> It then needs to cap alloc_blocks to MAXEXTLEN, because it can
> clearly be larger thanks to the above (MAXEXTLEN + 1) code.
> 

Ok. Considering your point above... in a sense this is kind of playing a
game with the MAXEXTLEN value by assuming it is <somepowerof2value>-1.
Perhaps what I'll do is roundup_pow_of_two(MAXEXTLEN) at the top (as
well as add the check that caps alloc_blocks to MAXEXTLEN at the very
end, with associated comment fixups). Thanks.

Brian

> Cheers,
> 
> Dave.
>