Re: [PATCH] fix barrier fail detection

[Timothy Shimmin <tes@sgi.com>]

Christoph Hellwig wrote:
> Currently we disable barriers as soon as we get a buffer in xlog_iodone
> that has the XBF_ORDERED flag cleared.  But this can be the case not only
> for buffers where the barrier failed, but also the first buffer of a
> split log write in case of a log wraparound.  Due to the disabled
> barriers we can easily get directory corruption on unclean shutdowns.
> So instead of using this check add a new buffer flag for failed barrier
> writes.
> 
> This is a regression vs 2.6.26 caused by patch to use the right macro
> to check for the ORDERED flag, as we previously got true returned for
> every buffer.
> 
> Thanks to Toei Rei for reporting the bug.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_buf.c	2008-10-09 13:36:50.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c	2008-10-09 13:38:38.000000000 +0200
> @@ -1007,6 +1007,7 @@ xfs_buf_iodone_work(
>  	    (bp->b_flags & (XBF_ORDERED|XBF_ASYNC)) == (XBF_ORDERED|XBF_ASYNC)) {
>  		XB_TRACE(bp, "ordered_retry", bp->b_iodone);
>  		bp->b_flags &= ~XBF_ORDERED;
> +		bp->b_flags |= _XFS_BARRIER_FAILED;
>  		xfs_buf_iorequest(bp);
>  	} else if (bp->b_iodone)
>  		(*(bp->b_iodone))(bp);

Actually, probably need to update the comment for this one.

The existing comment being:
> 	/*
> 	 * We can get an EOPNOTSUPP to ordered writes.  Here we clear the
> 	 * ordered flag and reissue them.  Because we can't tell the higher
> 	 * layers directly that they should not issue ordered I/O anymore, they
> 	 * need to check if the ordered flag was cleared during I/O completion.
> 	 */
> 	if ((bp->b_error == EOPNOTSUPP) &&
> 	    (bp->b_flags & (XBF_ORDERED|XBF_ASYNC)) == (XBF_ORDERED|XBF_ASYNC)) {
> 		XB_TRACE(bp, "ordered_retry", bp->b_iodone);
> 		bp->b_flags &= ~XBF_ORDERED;
> 		bp->b_flags |= _XFS_BARRIER_FAILED;


> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_buf.h	2008-10-09 13:36:50.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.h	2008-10-09 13:38:15.000000000 +0200
> @@ -85,6 +85,14 @@ typedef enum {
>  	 * modifications being lost.
>  	 */
>  	_XBF_PAGE_LOCKED = (1 << 22),
> +
> +	/*
> +	 * If we try a barrier write, but it fails we have to communicate
> +	 * this to the upper layers.  Unfortunately b_error gets overwritten
> +	 * when the buffer is re-issued so we have to add another flag to
> +	 * keep this information.
> +	 */
> +	_XFS_BARRIER_FAILED = (1 << 23),
>  } xfs_buf_flags_t;
>  
>  typedef enum {


> Index: linux-2.6-xfs/fs/xfs/xfs_log.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_log.c	2008-10-09 13:38:44.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_log.c	2008-10-09 13:39:32.000000000 +0200
> @@ -1037,7 +1037,8 @@ xlog_iodone(xfs_buf_t *bp)
>  	 * layer, it means the underlyin device no longer supports
>  	 * barrier I/O. Warn loudly and turn off barriers.
>  	 */
> -	if ((l->l_mp->m_flags & XFS_MOUNT_BARRIER) && !XFS_BUF_ISORDERED(bp)) {
> +	if (bp->b_flags & _XFS_BARRIER_FAILED) {
> +		bp->b_flags &= ~_XFS_BARRIER_FAILED;
>  		l->l_mp->m_flags &= ~XFS_MOUNT_BARRIER;
>  		xfs_fs_cmn_err(CE_WARN, l->l_mp,
>  				"xlog_iodone: Barriers are no longer supported"
> 
Okay, we probably should update this comment too.

The existing comment being:
> 	/*
> 	 * If the ordered flag has been removed by a lower
> 	 * layer, it means the underlyin device no longer supports
> 	 * barrier I/O. Warn loudly and turn off barriers.
> 	 */


Might as well fix the existing typo "underlyin" in the comment as well :)

Thanks,
--Tim