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. > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
