Re: [PATCH] xfs: cleanup log reservation calculactions

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 27, 2010 at 10:19:09AM -0400, Christoph Hellwig wrote:
> Instead of having small helper functions calling big macros do the
> calculations for the log reservations directly in the functions.  These
> are mostly 1:1 from the macros execept that the macros kept the quota
> calculations in their callers.

I like the idea, just a coupl eof small(ish) comments.

Firstly:

> -	return XFS_CALC_WRITE_LOG_RES(mp) + XFS_DQUOT_LOGRES(mp);
> +	return MAX(

	<some large expression>

> +		) + XFS_DQUOT_LOGRES(mp);

It's not very obvious that the dquot logres is separate to the large
calculation.  The single space indents as perenthesis are nested
just doesn't make this stand out like I think it should.  Can you
put the XFS_DQUOT_LOGRES(mp) first in all these calculations so that
it is more obvious that it is both present and separate to the main
reservation?i

That would also address the second issue that the way you terminate
the major expression and add the dquot logres is different in a
couple of functions.

I'm also wondering if we should be converting some of the magic
numbers like:

> +xfs_calc_ichange_reservation(
> +	struct xfs_mount	*mp)
>  {
> -	return XFS_CALC_ICHANGE_LOG_RES(mp) + XFS_DQUOT_LOGRES(mp);
> +	return mp->m_sb.sb_inodesize +
> +		mp->m_sb.sb_sectsize +
> +		512 +
> +		XFS_DQUOT_LOGRES(mp);
> +
>  }

to be correct. i.e.

	return mp->m_sb.sb_inodesize +
		mp->m_sb.sb_sectsize +
		sizeof(struct xfs_inode_log_format) +
		sizeof(struct xfs_buf_log_format) +
		XFS_DQUOT_LOGRES(mp);

instead of the magic 512 bytes that are added? I guess that's more
of a followup correctness patch than a cleanup patch....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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