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@xxxxxxxxxx>
>> 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.