Re: [PATCH] xfs: replace bp->flags usage with predefined macros

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 28, 2011 at 05:53:04PM -0700, Chandra Seetharaman wrote:
> Cleanup: Replace bp->flags usage with predefined macros.
> 
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

Christoph can correct me if I'm wrong, but I'm pretty sure his long term
direction is to remove the XFS_BUF_* macros completely.

Even so, in any new code we add or modify in xfs_buf.c we tend to
open code the flags. Hence the only code there that still uses the
XFS_BUF_* macros for manipulating flags are those we haven't needed
to touch for a long time....

> ---
> diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
> index 5e68099..8b24dc4 100644
> --- a/fs/xfs/linux-2.6/xfs_buf.c
> +++ b/fs/xfs/linux-2.6/xfs_buf.c
> @@ -470,7 +470,7 @@ _xfs_buf_find(
>  			 * continue searching to the right for an exact match.
>  			 */
>  			if (bp->b_buffer_length != range_length) {
> -				ASSERT(bp->b_flags & XBF_STALE);
> +				ASSERT(XFS_BUF_ISSTALE(bp));
>  				rbp = &(*rbp)->rb_right;
>  				continue;
>  			}
> @@ -516,7 +516,7 @@ found:
>  	 * it. We need to keep flags such as how we allocated the buffer memory
>  	 * intact here.
>  	 */
> -	if (bp->b_flags & XBF_STALE) {
> +	if (XFS_BUF_ISSTALE(bp)) {
>  		ASSERT((bp->b_flags & _XBF_DELWRI_Q) == 0);
>  		bp->b_flags &= XBF_MAPPED | _XBF_KMEM | _XBF_PAGES;
>  	}

And this code fragment is an example of why we are open coding the
flags checks: we do non-trivial open-coded flag manipulations
in many places, and hiding the fact that state is held in b_flags
makes it harder to read the code.

That is, the code as it stands in this case makes it obvious
that that if the buffer was stale, afterwards it is not stale
because the bit was cleared from b_flags. Using the macro hides the
fact that the stale state is held in b_flags and therefore cleared
by the conditional code and that the buffer is no longer stale...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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