fix barrier fail detection

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 <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c +1 -0

@@ -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);

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.h
===================================================================

fs/xfs/linux-2.6/xfs_buf.h +8 -0

@@ -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
===================================================================

fs/xfs/xfs_log.c +2 -1

@@ -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"

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.

Looks good. Should this be sent straight to Linus to get it into .27? If you want to do that, you can add my SOB.

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 <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c +1 -0

@@ -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);

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.h
===================================================================

fs/xfs/linux-2.6/xfs_buf.h +8 -0

@@ -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
===================================================================

fs/xfs/xfs_log.c +2 -1

@@ -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"

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 <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

	/*
	 * 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
===================================================================

fs/xfs/linux-2.6/xfs_buf.h +8 -0

@@ -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
===================================================================

/*
* 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. */

fs/xfs/linux-2.6/xfs_buf.c xfs_buf_iodone_work( · line 1013

Quoted 12 lines · Christoph Hellwig

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c +1 -0

@@ -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.

Quoted 6 lines

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================

fs/xfs/xfs_log.c xlog_iodone(xfs_buf_t *bp) · line 1044

Quoted 9 lines

fs/xfs/xfs_log.c +2 -1

@@ -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.

Might as well fix the existing typo "underlyin" in the comment as well :) Thanks,
--Tim

Timothy Shimmin wrote:

Quoted 1 line

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

i.e. something like:

Quoted 1 line · Timothy Shimmin

Timothy Shimmin wrote:

.....

Quoted 1 line

Quoted 1 line

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

.....

Quoted 1 line

i.e. something like:

Looks fine to me.