Re: [PATCH 1/4] xfs: kill XBF_FS_MANAGED buffers

[Christoph Hellwig <hch@infradead.org>]

Looks good, but a few comments below:

> +	bp = xfs_buf_get_noaddr(sector_size, mp->m_ddev_targp);
> +
>  	if (!bp || XFS_BUF_ISERROR(bp)) {

xfs_buf_get_noaddr will never return a buffer with an error set.

> -	ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
> +
> +	/* set up the buffer for a read IO */
> +	xfs_buf_lock(bp);
> +	XFS_BUF_SET_ADDR(bp, XFS_SB_DADDR);
> +        XFS_BUF_READ(bp);
> +        XFS_BUF_BUSY(bp);

Various indentation problems.

> +	/* grab a reference for caching the buffer */
> +        XFS_BUF_HOLD(bp);
>  	mp->m_sb_bp = bp;
> +
>  	xfs_buf_relse(bp);

Grabbing the reference just to drop it three lines later is rather
pointless, just remove both.

>  	ASSERT(XFS_BUF_VALUSEMA(bp) > 0);

Given that we took the lock a few lines above this one also feels rather
poinless.

> +fail:
> +	if (bp)
>  		xfs_buf_relse(bp);
> -	}
>  	return error;

I'd rather see this split into a fail_buf_relese label that puts the
buffer, and a fail label that just returns the error.

>  	 * when we call xfs_buf_relse().
>  	 */
>  	bp = xfs_getsb(mp, 0);
> -	XFS_BUF_UNMANAGE(bp);
> -	xfs_buf_relse(bp);
>  	mp->m_sb_bp = NULL;
> +
> +	/*
> +	 * need to release the buffer twice to free it because we hold an extra
> +	 * reference count on it.
> +	 */
> +	xfs_buf_relse(bp);
> +	xfs_buf_relse(bp);

I'd rather rewrite xfs_freesb to not use xfs_getsb and thus avoid taking
the superflous reference:

void
xfs_freesb(
	struct xfs_mount	*mp);

	struct xfs_buf		*bp = mp->m_sb_bp;

	mp->m_sb_bp = NULL;
	if (xfs_buf_cond_lock(bp)
		BUG();
	xfs_buf_relse(bp);
}

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