Re: [PATCH 02.5/32] xfs: remove xfs_tosspages

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 21, 2012 at 11:10:02PM -0600, Andrew Dahl wrote:
> On 11/21/2012 02:05 AM, Dave Chinner wrote:
> ...
> > 
> > [ Here's a tip for the future: anything that changes allocation
> > corner cases needs to be run through the entire of xfstests suite
> > because they have a nasty habit of causing secondary problems.... ]
> > 
> Makes sense -- I'll keep that in mind for the future. (Thanks!)
> 
> ...
> 
> > +
> > +STATIC int
> > +xfs_zero_file_space(
> > +	struct xfs_inode	*ip,
> > +	xfs_off_t		offset,
> > +	xfs_off_t		len,
> > +	int			attr_flags)
> > +{
> > +	struct xfs_mount	*mp = ip->i_mount;
> > +	uint			rounding;
> > +	xfs_off_t		start;
> > +	xfs_off_t		end;
> > +	int			error;
> > +
> > +	rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
> Let's say rounding is 4K
> > +
> > +	/* round the range iof extents we are going to convert inwards */
> > +	start = round_up(offset, rounding);
> > +	end = round_down(offset + len, rounding);
> Now, let's say we pass in (4K-1) for the offset and (4K-1).
> 
> Then start would be 4K and the end would be 4K, right?
> 
> > +
> > +	ASSERT(start >= offset);
> > +	ASSERT(end <= offset + len);
> These are both true, so this is good.
> > +
> > +	if (!(attr_flags & XFS_ATTR_NOLOCK))
> > +		xfs_ilock(ip, XFS_IOLOCK_EXCL);
> > +
> > +	if (start < end - 1) {
> This is false, as expected.
> > +		/* punch out the page cache over the conversion range */
> > +		truncate_pagecache_range(VFS_I(ip), start, end - 1);
> > +		/* convert the blocks */
> > +		error = xfs_alloc_file_space(ip, start, end - start - 1,
> > +				    XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT,
> > +				    attr_flags);
> > +		if (error)
> > +			goto out_unlock;
> > +	} else {
> > +		/* it's a sub-rounding range */
> > +		ASSERT(offset + len <= rounding);
> This is false. (8K - 2) <= 4K -- Not so good.

Right, I put this in after testing without thinking too hard about
it. It's always completely wrong, because offset can be an arbitrary
64 bit number, and rounding will always be <=64k...

> Maybe (2*rounding) would be better, as offset + len could never be
> greater than 2rounding (but can be greater than 1rounding). Or removing
> this assert altogether.

No, the correct thing to assert is:

		ASSERT(offset + len <= start);

That is, start is rounded up, and end is rounded down, so for a
sub-block range the end should always be less than the start of the
next block. That's what my current code has in it.

> Beyond that, I think it all looks good and like what you've done!

Thanks for looking at it. now all I've got to do if fix all the test
output. :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs