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