[PATCH 0/3] xfs: fix realtime device assert failures

[PATCH 0/3] xfs: fix realtime device assert failures

[Dave Chinner]

Hi folks,

This is a followup to Eric's patch that attempted to fix the assert
failure that Ross reported. I'd mentioned I'd looked at this
recently, because xfs/090 was triggering the same failure when
configured with a RT device. This series fixes the original locking
based assert failures, followed by the issues that fixing the
original assert uncovered. This now results in xfs/090 failing
because of xfs_db check problem (repair is clean) but it (and other
tests) no longer assert fail.

Cheers,

Dave.

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

[PATCH 1/3] xfs: lock rt summary inode on allocation

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

RT allocation can fail on a debug kernel with:

XFS: Assertion failed: xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL), file: fs/xfs/libxfs/xfs_bmap.c, line: 4039

When modifying the summary inode during allocation. This occurs
because the summary inode is never locked, and xfs_bmapi_*
operations expect it to be locked. The summary inode is effectively
protected byt he lock on the bitmap inode, so this really is only a
debug kernel issue.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bmap_util.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 7087756..fd7f51c 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -202,10 +202,12 @@ xfs_bmap_rtalloc(
 		ralen = MAXEXTLEN / mp->m_sb.sb_rextsize;
 
 	/*
-	 * Lock out other modifications to the RT bitmap inode.
+	 * Lock out modifications to both the RT bitmap and summary inodes
 	 */
 	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
+	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
 
 	/*
 	 * If it's an allocation to an empty file at offset 0,
-- 
2.5.0

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

[PATCH 2/3] xfs: RT bitmap and summary buffers are not typed

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When logging buffers, we attach a type to them that follows the
buffer all the way into the log and is used to identify the buffer
contents in log recovery. Both the realtime summary buffers and the
bitmap buffers do not have types defined or set, so when we try to
log them we see assert failure:

XFS: Assertion failed: (bip->bli_flags & XFS_BLI_STALE) || (xfs_blft_from_flags(&bip->__bli_format) > XFS_BLFT_UNKNOWN_BUF && xfs_blft_from_flags(&bip->__bli_format) < XFS_BLFT_MAX_BUF), file: fs/xfs/xfs_buf_item.c, line: 294

Fix this by adding buffer log format types for these buffers, and
add identification support into log recovery for them.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_log_format.h | 2 ++
 fs/xfs/libxfs/xfs_rtbitmap.c   | 3 +++
 fs/xfs/xfs_log_recover.c       | 5 +++++
 3 files changed, 10 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 03f90b9..d54a801 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -496,6 +496,8 @@ enum xfs_blft {
 	XFS_BLFT_ATTR_LEAF_BUF,
 	XFS_BLFT_ATTR_RMT_BUF,
 	XFS_BLFT_SB_BUF,
+	XFS_BLFT_RTBITMAP_BUF,
+	XFS_BLFT_RTSUMMARY_BUF,
 	XFS_BLFT_MAX_BUF = (1 << XFS_BLFT_BITS),
 };
 
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index acc71dd..bfa7b85 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -71,6 +71,9 @@ xfs_rtbuf_get(
 				   mp->m_bsize, 0, &bp, NULL);
 	if (error)
 		return error;
+
+	xfs_trans_buf_set_type(tp, bp, issum ? XFS_BLFT_RTSUMMARY_BUF
+					     : XFS_BLFT_RTBITMAP_BUF);
 	*bpp = bp;
 	return 0;
 }
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 464c5d4..e2d7533 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2473,6 +2473,11 @@ xlog_recover_validate_buf_type(
 		}
 		bp->b_ops = &xfs_sb_buf_ops;
 		break;
+	case XFS_BLFT_RTBITMAP_BUF:
+	case XFS_BLFT_RTSUMMARY_BUF:
+		/* no verification of RT buffers is done */
+		bp->b_ops = NULL;
+		break;
 	default:
 		xfs_warn(mp, "Unknown buffer type %d!",
 			 xfs_blft_from_flags(buf_f));
-- 
2.5.0

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

[PATCH 3/3] xfs: RT bitmap and summary buffers need verifiers

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Buffers without verifiers issue runtime warnings on XFS. We don't
have anything we can actually verify in the RT buffers (no CRCs, not
magic numbers, etc), but we still need verifiers to avoid the
warnings.

Add a set of dummy verifier operations for the realtime buffers and
apply them in the appropriate places.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_rtbitmap.c | 27 ++++++++++++++++++++++++++-
 fs/xfs/libxfs/xfs_shared.h   |  1 +
 fs/xfs/xfs_log_recover.c     |  4 ++--
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index bfa7b85..01c0cdb 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -42,6 +42,31 @@
  */
 
 /*
+ * Real time buffers need verifiers to avoid runtime warnigns during IO.
+ * We don't have anything to verify, however, so these are just dummy
+ * operations.
+ */
+static void
+xfs_rtbuf_verify_read(
+	struct xfs_buf	*bp)
+{
+	return;
+}
+
+static void
+xfs_rtbuf_verify_write(
+	struct xfs_buf	*bp)
+{
+	return;
+}
+
+const struct xfs_buf_ops xfs_rtbuf_ops = {
+	.name = "rtbuf",
+	.verify_read = xfs_rtbuf_verify_read,
+	.verify_write = xfs_rtbuf_verify_write,
+};
+
+/*
  * Get a buffer for the bitmap or summary file block specified.
  * The buffer is returned read and locked.
  */
@@ -68,7 +93,7 @@ xfs_rtbuf_get(
 	ASSERT(map.br_startblock != NULLFSBLOCK);
 	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
 				   XFS_FSB_TO_DADDR(mp, map.br_startblock),
-				   mp->m_bsize, 0, &bp, NULL);
+				   mp->m_bsize, 0, &bp, &xfs_rtbuf_ops);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 15c3ceb..81ac870 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -53,6 +53,7 @@ extern const struct xfs_buf_ops xfs_dquot_buf_ra_ops;
 extern const struct xfs_buf_ops xfs_sb_buf_ops;
 extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
 extern const struct xfs_buf_ops xfs_symlink_buf_ops;
+extern const struct xfs_buf_ops xfs_rtbuf_ops;
 
 /*
  * Transaction types.  Used to distinguish types of buffers. These never reach
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e2d7533..9d09a0d 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2475,8 +2475,8 @@ xlog_recover_validate_buf_type(
 		break;
 	case XFS_BLFT_RTBITMAP_BUF:
 	case XFS_BLFT_RTSUMMARY_BUF:
-		/* no verification of RT buffers is done */
-		bp->b_ops = NULL;
+		/* no magic numbers for verification of RT buffers */
+		bp->b_ops = &xfs_rtbuf_ops;
 		break;
 	default:
 		xfs_warn(mp, "Unknown buffer type %d!",
-- 
2.5.0

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

Re: [PATCH 3/3] xfs: RT bitmap and summary buffers need verifiers

[Eric Sandeen]

On 2/1/16 6:39 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Buffers without verifiers issue runtime warnings on XFS. We don't
> have anything we can actually verify in the RT buffers (no CRCs, not
> magic numbers, etc), but we still need verifiers to avoid the
> warnings.
> 
> Add a set of dummy verifier operations for the realtime buffers and
> apply them in the appropriate places.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c | 27 ++++++++++++++++++++++++++-
>  fs/xfs/libxfs/xfs_shared.h   |  1 +
>  fs/xfs/xfs_log_recover.c     |  4 ++--
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index bfa7b85..01c0cdb 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -42,6 +42,31 @@
>   */
>  
>  /*
> + * Real time buffers need verifiers to avoid runtime warnigns during IO.

"warnings" ;)

Otherwise:
Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> + * We don't have anything to verify, however, so these are just dummy
> + * operations.
> + */
> +static void
> +xfs_rtbuf_verify_read(
> +	struct xfs_buf	*bp)
> +{
> +	return;
> +}
> +
> +static void
> +xfs_rtbuf_verify_write(
> +	struct xfs_buf	*bp)
> +{
> +	return;
> +}
> +
> +const struct xfs_buf_ops xfs_rtbuf_ops = {
> +	.name = "rtbuf",
> +	.verify_read = xfs_rtbuf_verify_read,
> +	.verify_write = xfs_rtbuf_verify_write,
> +};
> +
> +/*
>   * Get a buffer for the bitmap or summary file block specified.
>   * The buffer is returned read and locked.
>   */
> @@ -68,7 +93,7 @@ xfs_rtbuf_get(
>  	ASSERT(map.br_startblock != NULLFSBLOCK);
>  	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
>  				   XFS_FSB_TO_DADDR(mp, map.br_startblock),
> -				   mp->m_bsize, 0, &bp, NULL);
> +				   mp->m_bsize, 0, &bp, &xfs_rtbuf_ops);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 15c3ceb..81ac870 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -53,6 +53,7 @@ extern const struct xfs_buf_ops xfs_dquot_buf_ra_ops;
>  extern const struct xfs_buf_ops xfs_sb_buf_ops;
>  extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
>  extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> +extern const struct xfs_buf_ops xfs_rtbuf_ops;
>  
>  /*
>   * Transaction types.  Used to distinguish types of buffers. These never reach
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e2d7533..9d09a0d 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2475,8 +2475,8 @@ xlog_recover_validate_buf_type(
>  		break;
>  	case XFS_BLFT_RTBITMAP_BUF:
>  	case XFS_BLFT_RTSUMMARY_BUF:
> -		/* no verification of RT buffers is done */
> -		bp->b_ops = NULL;
> +		/* no magic numbers for verification of RT buffers */
> +		bp->b_ops = &xfs_rtbuf_ops;
>  		break;
>  	default:
>  		xfs_warn(mp, "Unknown buffer type %d!",
> 

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

Re: [PATCH 2/3] xfs: RT bitmap and summary buffers are not typed

[Eric Sandeen]

On 2/1/16 6:39 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When logging buffers, we attach a type to them that follows the
> buffer all the way into the log and is used to identify the buffer
> contents in log recovery. Both the realtime summary buffers and the
> bitmap buffers do not have types defined or set, so when we try to
> log them we see assert failure:
> 
> XFS: Assertion failed: (bip->bli_flags & XFS_BLI_STALE) || (xfs_blft_from_flags(&bip->__bli_format) > XFS_BLFT_UNKNOWN_BUF && xfs_blft_from_flags(&bip->__bli_format) < XFS_BLFT_MAX_BUF), file: fs/xfs/xfs_buf_item.c, line: 294
> 
> Fix this by adding buffer log format types for these buffers, and
> add identification support into log recovery for them.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_log_format.h | 2 ++
>  fs/xfs/libxfs/xfs_rtbitmap.c   | 3 +++
>  fs/xfs/xfs_log_recover.c       | 5 +++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 03f90b9..d54a801 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -496,6 +496,8 @@ enum xfs_blft {
>  	XFS_BLFT_ATTR_LEAF_BUF,
>  	XFS_BLFT_ATTR_RMT_BUF,
>  	XFS_BLFT_SB_BUF,
> +	XFS_BLFT_RTBITMAP_BUF,
> +	XFS_BLFT_RTSUMMARY_BUF,
>  	XFS_BLFT_MAX_BUF = (1 << XFS_BLFT_BITS),
>  };
>  
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index acc71dd..bfa7b85 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -71,6 +71,9 @@ xfs_rtbuf_get(
>  				   mp->m_bsize, 0, &bp, NULL);
>  	if (error)
>  		return error;
> +
> +	xfs_trans_buf_set_type(tp, bp, issum ? XFS_BLFT_RTSUMMARY_BUF
> +					     : XFS_BLFT_RTBITMAP_BUF);
>  	*bpp = bp;
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 464c5d4..e2d7533 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2473,6 +2473,11 @@ xlog_recover_validate_buf_type(
>  		}
>  		bp->b_ops = &xfs_sb_buf_ops;
>  		break;
> +	case XFS_BLFT_RTBITMAP_BUF:
> +	case XFS_BLFT_RTSUMMARY_BUF:
> +		/* no verification of RT buffers is done */
> +		bp->b_ops = NULL;
> +		break;
>  	default:
>  		xfs_warn(mp, "Unknown buffer type %d!",
>  			 xfs_blft_from_flags(buf_f));
> 

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

Re: [PATCH 1/3] xfs: lock rt summary inode on allocation

[Eric Sandeen]

On 2/1/16 6:39 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> RT allocation can fail on a debug kernel with:
> 
> XFS: Assertion failed: xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL), file: fs/xfs/libxfs/xfs_bmap.c, line: 4039
> 
> When modifying the summary inode during allocation. This occurs
> because the summary inode is never locked, and xfs_bmapi_*
> operations expect it to be locked. The summary inode is effectively
> protected byt he lock on the bitmap inode, so this really is only a
> debug kernel issue.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Oh, it was easy after all...

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_bmap_util.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 7087756..fd7f51c 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -202,10 +202,12 @@ xfs_bmap_rtalloc(
>  		ralen = MAXEXTLEN / mp->m_sb.sb_rextsize;
>  
>  	/*
> -	 * Lock out other modifications to the RT bitmap inode.
> +	 * Lock out modifications to both the RT bitmap and summary inodes
>  	 */
>  	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> +	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
>  
>  	/*
>  	 * If it's an allocation to an empty file at offset 0,
> 

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

Re: [PATCH 0/3] xfs: fix realtime device assert failures

[Ross Zwisler]

This series solves the assert and the follow-up XFS warning for me.

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

The lockdep splat is still there, but I assume you know that.

On Mon, Feb 1, 2016 at 5:39 PM, Dave Chinner <david@fromorbit.com> wrote:
> Hi folks,
>
> This is a followup to Eric's patch that attempted to fix the assert
> failure that Ross reported. I'd mentioned I'd looked at this
> recently, because xfs/090 was triggering the same failure when
> configured with a RT device. This series fixes the original locking
> based assert failures, followed by the issues that fixing the
> original assert uncovered. This now results in xfs/090 failing
> because of xfs_db check problem (repair is clean) but it (and other
> tests) no longer assert fail.
>
> Cheers,
>
> Dave.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 0/3] xfs: fix realtime device assert failures

[Dave Chinner]

On Thu, Feb 04, 2016 at 02:22:17PM -0700, Ross Zwisler wrote:
> This series solves the assert and the follow-up XFS warning for me.
> 
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> The lockdep splat is still there, but I assume you know that.

Mostly I don't care about lockdep splats because experience tells me
they are wild goose chases that end with us jumping through ever
more complex flaming hoops to annotate the special corner case
lockdep has falsely triggered on.

I also don't have time to care about lockdep right now, so I'm
ignoring it until it manifests itself as a real deadlock.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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