[PATCH 0/3] xfs: buffer types need to be set

[PATCH 0/3] xfs: buffer types need to be set

[Dave Chinner]

Hi Jan,

These three patches detect and fix the issues you reported with log
recovery finding buffers with a format type of zero. The type of
zero (XFS_BLFT_UNKNOWN_BUF) is only valid for buffers that have
been cancelled (i.e. invalidated or marked stale as they have been
freed), so the series adds asserts to ensure these conditions are
met during transaction commit. Hence we shouldn't ever get new code
that fails to set the buffer type getting through testing.

The last two patches fix the cases that running xfstests uncovered
where we don't set the buffer type appropriately. There may be more,
but doing this much made my head hurt and xfstests is clean, so it's
as much as I'm going to do right now. Can you test it and see if it
runs clean (with CONFIG_XFS_WARN=y or CONFIG_XFS_DEBUG=y) on your
test setup?

Cheers,

Dave.

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

[PATCH 1/3] xfs: ensure buffer types are set correctly

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Jan Kara reported that log recovery was finding buffers with invalid
types in them. This should not happen, and indicates a bug in the
logging of buffers. To catch this, add asserts to the buffer
formatting code to ensure that the buffer type is in range when the
transaction is committed.

We don't set a type on buffers being marked stale - they are not
going to get replayed, the format item exists only for recovery to
be able to prevent replay of the buffer, so the type does not
matter. Hence that needs special casing here.

Reported-by: Jan Kara <jack@suse.cz>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 925ead2..507d96a 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -319,6 +319,10 @@ xfs_buf_item_format(
 	ASSERT(atomic_read(&bip->bli_refcount) > 0);
 	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
 	       (bip->bli_flags & XFS_BLI_STALE));
+	ASSERT((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));
+
 
 	/*
 	 * If it is an inode buffer, transfer the in-memory state to the
-- 
2.0.0

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

Re: [PATCH 1/3] xfs: ensure buffer types are set correctly

[Brian Foster]

On Wed, Jan 21, 2015 at 11:39:38AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Jan Kara reported that log recovery was finding buffers with invalid
> types in them. This should not happen, and indicates a bug in the
> logging of buffers. To catch this, add asserts to the buffer
> formatting code to ensure that the buffer type is in range when the
> transaction is committed.
> 
> We don't set a type on buffers being marked stale - they are not
> going to get replayed, the format item exists only for recovery to
> be able to prevent replay of the buffer, so the type does not
> matter. Hence that needs special casing here.
> 
> Reported-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_buf_item.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 925ead2..507d96a 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -319,6 +319,10 @@ xfs_buf_item_format(
>  	ASSERT(atomic_read(&bip->bli_refcount) > 0);
>  	ASSERT((bip->bli_flags & XFS_BLI_LOGGED) ||
>  	       (bip->bli_flags & XFS_BLI_STALE));
> +	ASSERT((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));
> +
>  
>  	/*
>  	 * If it is an inode buffer, transfer the in-memory state to the
> -- 
> 2.0.0
> 
> _______________________________________________
> 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

[PATCH 2/3] xfs: inode unlink does not set AGI buffer type

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

This leads to log recovery throwing errors like:

XFS (md0): Mounting V5 Filesystem
XFS (md0): Starting recovery (logdev: internal)
XFS (md0): Unknown buffer type 0!
XFS (md0): _xfs_buf_ioapply: no ops on block 0xaea8802/0x1
ffff8800ffc53800: 58 41 47 49 .....

Which is the AGI buffer magic number.

Ensure that we set the type appropriately in both unlink list
addition and removal.

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

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 573b49c..f616c8f 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2051,6 +2051,7 @@ xfs_iunlink(
 	agi->agi_unlinked[bucket_index] = cpu_to_be32(agino);
 	offset = offsetof(xfs_agi_t, agi_unlinked) +
 		(sizeof(xfs_agino_t) * bucket_index);
+	xfs_trans_buf_set_type(tp, agibp, XFS_BLFT_AGI_BUF);
 	xfs_trans_log_buf(tp, agibp, offset,
 			  (offset + sizeof(xfs_agino_t) - 1));
 	return 0;
@@ -2142,6 +2143,7 @@ xfs_iunlink_remove(
 		agi->agi_unlinked[bucket_index] = cpu_to_be32(next_agino);
 		offset = offsetof(xfs_agi_t, agi_unlinked) +
 			(sizeof(xfs_agino_t) * bucket_index);
+		xfs_trans_buf_set_type(tp, agibp, XFS_BLFT_AGI_BUF);
 		xfs_trans_log_buf(tp, agibp, offset,
 				  (offset + sizeof(xfs_agino_t) - 1));
 	} else {
-- 
2.0.0

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

Re: [PATCH 2/3] xfs: inode unlink does not set AGI buffer type

[Brian Foster]

On Wed, Jan 21, 2015 at 11:39:39AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This leads to log recovery throwing errors like:
> 
> XFS (md0): Mounting V5 Filesystem
> XFS (md0): Starting recovery (logdev: internal)
> XFS (md0): Unknown buffer type 0!
> XFS (md0): _xfs_buf_ioapply: no ops on block 0xaea8802/0x1
> ffff8800ffc53800: 58 41 47 49 .....
> 
> Which is the AGI buffer magic number.
> 
> Ensure that we set the type appropriately in both unlink list
> addition and removal.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_inode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 573b49c..f616c8f 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2051,6 +2051,7 @@ xfs_iunlink(
>  	agi->agi_unlinked[bucket_index] = cpu_to_be32(agino);
>  	offset = offsetof(xfs_agi_t, agi_unlinked) +
>  		(sizeof(xfs_agino_t) * bucket_index);
> +	xfs_trans_buf_set_type(tp, agibp, XFS_BLFT_AGI_BUF);
>  	xfs_trans_log_buf(tp, agibp, offset,
>  			  (offset + sizeof(xfs_agino_t) - 1));
>  	return 0;
> @@ -2142,6 +2143,7 @@ xfs_iunlink_remove(
>  		agi->agi_unlinked[bucket_index] = cpu_to_be32(next_agino);
>  		offset = offsetof(xfs_agi_t, agi_unlinked) +
>  			(sizeof(xfs_agino_t) * bucket_index);
> +		xfs_trans_buf_set_type(tp, agibp, XFS_BLFT_AGI_BUF);
>  		xfs_trans_log_buf(tp, agibp, offset,
>  				  (offset + sizeof(xfs_agino_t) - 1));
>  	} else {
> -- 
> 2.0.0
> 
> _______________________________________________
> 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

[PATCH 3/3] xfs: set buf types when converting extent formats

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Conversion from local to extent format does not set the buffer type
correctly on the new extent buffer when a symlink data is moved out
of line.

Fix the symlink code and leave a comment in the generic bmap code
reminding us that the format-specific data copy needs to set the
destination buffer type appropriately.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c           | 6 +++++-
 fs/xfs/libxfs/xfs_symlink_remote.c | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 63a5bb9..61ec015 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -973,7 +973,11 @@ xfs_bmap_local_to_extents(
 	*firstblock = args.fsbno;
 	bp = xfs_btree_get_bufl(args.mp, tp, args.fsbno, 0);
 
-	/* initialise the block and copy the data */
+	/*
+	 * Initialise the block and copy the data
+	 *
+	 * Note: init_fn must set the buffer log item type correctly!
+	 */
 	init_fn(tp, bp, ip, ifp);
 
 	/* account for the change in fork size and log everything */
diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index c80c523..e7e26bd 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -178,6 +178,8 @@ xfs_symlink_local_to_remote(
 	struct xfs_mount	*mp = ip->i_mount;
 	char			*buf;
 
+	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SYMLINK_BUF);
+
 	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
 		bp->b_ops = NULL;
 		memcpy(bp->b_addr, ifp->if_u1.if_data, ifp->if_bytes);
-- 
2.0.0

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

Re: [PATCH 3/3] xfs: set buf types when converting extent formats

[Brian Foster]

On Wed, Jan 21, 2015 at 11:39:40AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Conversion from local to extent format does not set the buffer type
> correctly on the new extent buffer when a symlink data is moved out
> of line.
> 
> Fix the symlink code and leave a comment in the generic bmap code
> reminding us that the format-specific data copy needs to set the
> destination buffer type appropriately.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c           | 6 +++++-
>  fs/xfs/libxfs/xfs_symlink_remote.c | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 63a5bb9..61ec015 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -973,7 +973,11 @@ xfs_bmap_local_to_extents(
>  	*firstblock = args.fsbno;
>  	bp = xfs_btree_get_bufl(args.mp, tp, args.fsbno, 0);
>  
> -	/* initialise the block and copy the data */
> +	/*
> +	 * Initialise the block and copy the data
> +	 *
> +	 * Note: init_fn must set the buffer log item type correctly!
> +	 */
>  	init_fn(tp, bp, ip, ifp);
>  
>  	/* account for the change in fork size and log everything */
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index c80c523..e7e26bd 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -178,6 +178,8 @@ xfs_symlink_local_to_remote(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	char			*buf;
>  
> +	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SYMLINK_BUF);
> +
>  	if (!xfs_sb_version_hascrc(&mp->m_sb)) {
>  		bp->b_ops = NULL;
>  		memcpy(bp->b_addr, ifp->if_u1.if_data, ifp->if_bytes);
> -- 
> 2.0.0
> 
> _______________________________________________
> 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

[PATCH 4/3] xfs: set superblock buffer type correctly

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When the superblock is modified in a transaction, the commonly
modified fields are not actually copied to the superblock buffer to
avoid the buffer lock becoming a serialisation point. However, there
are some other operations that modify the superblock fields within
the transaction that don't directly log to the superblock but rely
on the changes to be applied during the transaction commit (to
minimise the buffer lock hold time).

When we do this, we fail to mark the buffer log item as being a
superblock buffer and that can lead to the buffer not being marked
with the corect type in the log and hence causing recovery issues.
Fix it by setting the type correctly, similar to xfs_mod_sb()...

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index fa3135b..eb90cd5 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -472,6 +472,7 @@ xfs_trans_apply_sb_deltas(
 		whole = 1;
 	}
 
+	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
 	if (whole)
 		/*
 		 * Log the whole thing, the fields are noncontiguous.

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

Re: [PATCH 4/3] xfs: set superblock buffer type correctly

[Brian Foster]

On Wed, Jan 21, 2015 at 01:34:44PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When the superblock is modified in a transaction, the commonly
> modified fields are not actually copied to the superblock buffer to
> avoid the buffer lock becoming a serialisation point. However, there
> are some other operations that modify the superblock fields within
> the transaction that don't directly log to the superblock but rely
> on the changes to be applied during the transaction commit (to
> minimise the buffer lock hold time).
> 
> When we do this, we fail to mark the buffer log item as being a
> superblock buffer and that can lead to the buffer not being marked
> with the corect type in the log and hence causing recovery issues.
> Fix it by setting the type correctly, similar to xfs_mod_sb()...
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_trans.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index fa3135b..eb90cd5 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -472,6 +472,7 @@ xfs_trans_apply_sb_deltas(
>  		whole = 1;
>  	}
>  
> +	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
>  	if (whole)
>  		/*
>  		 * Log the whole thing, the fields are noncontiguous.
> 
> _______________________________________________
> 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: buffer types need to be set

[Jan Kara]

   Hi Dave,

On Wed 21-01-15 11:39:37, Dave Chinner wrote:
> These three patches detect and fix the issues you reported with log
> recovery finding buffers with a format type of zero. The type of
> zero (XFS_BLFT_UNKNOWN_BUF) is only valid for buffers that have
> been cancelled (i.e. invalidated or marked stale as they have been
> freed), so the series adds asserts to ensure these conditions are
> met during transaction commit. Hence we shouldn't ever get new code
> that fails to set the buffer type getting through testing.
> 
> The last two patches fix the cases that running xfstests uncovered
> where we don't set the buffer type appropriately. There may be more,
> but doing this much made my head hurt and xfstests is clean, so it's
> as much as I'm going to do right now. Can you test it and see if it
> runs clean (with CONFIG_XFS_WARN=y or CONFIG_XFS_DEBUG=y) on your
> test setup?
  Thanks for a quick response with patches. My test round has finished and
the new assertion didn't trigger so things look fine. I've also provided a
test kernel to the guy seeing these issues in the wild but there it took
days / weeks to trigger so I wouldn't wait for it...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH 0/3] xfs: buffer types need to be set

[Jan Kara]

On Wed 21-01-15 17:07:57, Jan Kara wrote:
>    Hi Dave,
> 
> On Wed 21-01-15 11:39:37, Dave Chinner wrote:
> > These three patches detect and fix the issues you reported with log
> > recovery finding buffers with a format type of zero. The type of
> > zero (XFS_BLFT_UNKNOWN_BUF) is only valid for buffers that have
> > been cancelled (i.e. invalidated or marked stale as they have been
> > freed), so the series adds asserts to ensure these conditions are
> > met during transaction commit. Hence we shouldn't ever get new code
> > that fails to set the buffer type getting through testing.
> > 
> > The last two patches fix the cases that running xfstests uncovered
> > where we don't set the buffer type appropriately. There may be more,
> > but doing this much made my head hurt and xfstests is clean, so it's
> > as much as I'm going to do right now. Can you test it and see if it
> > runs clean (with CONFIG_XFS_WARN=y or CONFIG_XFS_DEBUG=y) on your
> > test setup?
>   Thanks for a quick response with patches. My test round has finished and
> the new assertion didn't trigger so things look fine. I've also provided a
> test kernel to the guy seeing these issues in the wild but there it took
> days / weeks to trigger so I wouldn't wait for it...
  BTW, this should be stable material I guess (just that I didn't see CC to
stable in the patches).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH 0/3] xfs: buffer types need to be set

[Dave Chinner]

On Wed, Jan 21, 2015 at 05:07:57PM +0100, Jan Kara wrote:
>   Thanks for a quick response with patches. My test round has finished and
> the new assertion didn't trigger so things look fine. I've also provided a
> test kernel to the guy seeing these issues in the wild but there it took
> days / weeks to trigger so I wouldn't wait for it...

Given the timing requirement to hit the logging condition just
right, then to crash immeidately afterwards, I'm not surprised it
has taken this long to be uncovered in the wild...

> BTW, this should be stable material I guess (just that I didn't
> see CC to stable in the patches).

Probably should - I hadn't even thought that far ahead.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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