xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: push rounddown_pow_of_two() to after prealloc throttle
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 14 Dec 2012 11:05:38 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121213003913.GE16353@dastard>
References: <1354726078-31793-1-git-send-email-bfoster@xxxxxxxxxx> <1354726078-31793-3-git-send-email-bfoster@xxxxxxxxxx> <20121213003913.GE16353@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1
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.

Brian

> Cheers,
> 
> Dave.
> 

<Prev in Thread] Current Thread [Next in Thread>