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

[Andrew Dahl <adahl@sgi.com>]

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.

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.

> +		error = xfs_iozero(ip, offset, len);
> +		goto out_unlock;
> +	}
> +
> +	/* now we've handled the interior of the range, handle the edges */
> +	if (start != offset)
> +		error = xfs_iozero(ip, offset, start - offset);
> +	if (!error && end != offset + len)
> +		error = xfs_iozero(ip, end, offset + len - end);
This looks good.
> +
> +out_unlock:
> +	if (!(attr_flags & XFS_ATTR_NOLOCK))
> +		xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> +	return error;
...

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

Thanks,

Andrew

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