xfs: buffer types need to be set
cover xfs: buffer types need to be set
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/3 xfs: ensure buffer types are set correctly
From: Dave Chinner <[email protected]>
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.
--- fs/xfs/xfs_buf_item.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/xfs/xfs_buf_item.c +4 -0
@@ -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
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/3] xfs: ensure buffer types are set correctly
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 8 lines
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 <[email protected]>Signed-off-by Dave Chinner <[email protected]>---
Quoted 25 lines
fs/xfs/xfs_buf_item.c | 4 ++++ 1 file changed, 4 insertions(+)fs/xfs/xfs_buf_item.c +4
-0@@ -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 Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/3 xfs: inode unlink does not set AGI buffer type
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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/0x1ffff8800ffc53800: 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.
--- fs/xfs/xfs_inode.c | 2 ++ 1 file changed, 2 insertions(+)
fs/xfs/xfs_inode.c +2 -0
@@ -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 {
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/3] xfs: inode unlink does not set AGI buffer type
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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/0x1ffff8800ffc53800: 58 41 47 49 .....
Quoted 7 lines
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 <[email protected]>---
Quoted 30 lines
fs/xfs/xfs_inode.c | 2 ++ 1 file changed, 2 insertions(+)fs/xfs/xfs_inode.c +2
-0@@ -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 {Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/3 xfs: set buf types when converting extent formats
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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.
--- fs/xfs/libxfs/xfs_bmap.c | 6 +++++- fs/xfs/libxfs/xfs_symlink_remote.c | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-)
fs/xfs/libxfs/xfs_bmap.c +5 -1
@@ -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 */
fs/xfs/libxfs/xfs_symlink_remote.c +2 -0
@@ -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);
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/3] xfs: set buf types when converting extent formats
Quoted 6 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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.
Quoted 6 lines
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 <[email protected]>---
Quoted 41 lines
fs/xfs/libxfs/xfs_bmap.c | 6 +++++- fs/xfs/libxfs/xfs_symlink_remote.c | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-)fs/xfs/libxfs/xfs_bmap.c +5
-1@@ -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 */ fs/xfs/libxfs/xfs_symlink_remote.c +2
-0@@ -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);Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
[PATCH 4/3] xfs: set superblock buffer type correctly
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
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()...
--- fs/xfs/xfs_trans.c | 1 + 1 file changed, 1 insertion(+)
fs/xfs/xfs_trans.c +1 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Brian Foster
Reviewed-byRe: [PATCH 4/3] xfs: set superblock buffer type correctly
Quoted 10 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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).
Quoted 7 lines
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 <[email protected]>---
Quoted 16 lines
fs/xfs/xfs_trans.c | 1 + 1 file changed, 1 insertion(+)fs/xfs/xfs_trans.c +1
-0@@ -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.
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Hi Dave,
Quoted 8 lines · Dave Chinner
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.
Quoted 6 lines
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=yorCONFIG_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
Signature · 7 lines
Jan Kara <[email protected]> SUSE Labs, CR _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 17 lines · Jan Kara
Hi Dave,
Quoted 8 lines · Dave Chinner
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.Quoted 6 lines
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=yorCONFIG_XFS_DEBUG=y) on your test setup?
Quoted 4 lines
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
Signature · 7 lines
Jan Kara <[email protected]> SUSE Labs, CR _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 4 lines · Jan Kara
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...
Quoted 2 lines
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs