xfs: test support for xattr inactivate
cover xfs: test support for xattr inactivate
Hi all,
This series adds some testability to the issue fixed by "xfs: reinit btree pointer on attr tree inactivation walk." Patch 1 adds an assert and patch 2 adds an error injection tag to disable buffer LRU caching. I have an xfstests test that reproduces the aforementioned problem reliably using the error injection tag.
Note that patch 1 is kind of a cautious attempt to detect the issue. I'm wondering if the better fix is to pass -1 if we indeed should not ever expect to hit a hole in this particular path. Thoughts?
Brian
Brian Foster (2):
xfs: assert that xattr inactivation never reaches a hole
xfs: buffer lru reference count error injection tag
fs/xfs/xfs_attr_inactive.c | 1 + fs/xfs/xfs_buf.c | 16 ++++++++++++++++ fs/xfs/xfs_buf.h | 5 +---- fs/xfs/xfs_error.c | 3 +++ fs/xfs/xfs_error.h | 4 +++- 5 files changed, 24 insertions(+), 5 deletions(-)
Signature · 1 line
2.9.5
PATCH 1/2 xfs: assert that xattr inactivation never reaches a hole
The child buffer read in xfs_attr3_node_inactive() should never reach a hole in the attr fork. If this occurs, it is likely due to a bug. Prior to commit cd87d867 ("xfs: don't crash on unexpected holes in dir/attr btrees"), this would result in a crash. Now that the crash has been fixed, this is a silent failure.
Add an assert in this codepath to detect this particular condition. Note that the right fix here may be to pass -1 to xfs_da3_node_read() such that a hole returns an error. This is a cautious first step in that direction.
--- fs/xfs/xfs_attr_inactive.c | 1 + 1 file changed, 1 insertion(+)
fs/xfs/xfs_attr_inactive.c +1 -0
@@ -255,6 +255,7 @@ xfs_attr3_node_inactive( XFS_ATTR_FORK);if (error)return error; +ASSERT (child_bp);if (child_bp) {/* save for re-read later */ child_blkno =XFS_BUF_ADDR (child_bp);
Signature · 1 line
2.9.5
Re: [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole
Quoted 21 lines · Brian Foster
The child buffer read in
xfs_attr3_node_inactive()should never reach a hole in the attr fork. If this occurs, it is likely due to a bug. Prior to commitcd87d867("xfs: don't crash on unexpected holes in dir/attr btrees"), this would result in a crash. Now that the crash has been fixed, this is a silent failure.Add an assert in this codepath to detect this particular condition. Note that the right fix here may be to pass -1 to
xfs_da3_node_read()such that a hole returns an error. This is a cautious first step in that direction.Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_attr_inactive.c | 1 + 1 file changed, 1 insertion(+)fs/xfs/xfs_attr_inactive.c +0
-0
fs/xfs/xfs_attr_inactive.c xfs_attr3_node_inactive( · line 258
Quoted 4 lines
fs/xfs/xfs_attr_inactive.c +1
-0@@ -255,6 +255,7 @@ xfs_attr3_node_inactive( XFS_ATTR_FORK);if (error)return error; +ASSERT (child_bp);
xfs_dabuf_map will log an error message and return -EFSCORRUPTED if mappedbno == -1, so (afaict) you might as well do that instead of adding the ASSERT. We'll still see the dmesg report.
--D
Quoted 10 lines
fs/xfs/xfs_attr_inactive.c +0
-0if (child_bp) {/* save for re-read later */ child_blkno =XFS_BUF_ADDR (child_bp);Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole
Quoted 17 lines · Darrick J. Wong
Quoted 16 lines · Brian Foster
The child buffer read in
xfs_attr3_node_inactive()should never reach a hole in the attr fork. If this occurs, it is likely due to a bug. Prior to commitcd87d867("xfs: don't crash on unexpected holes in dir/attr btrees"), this would result in a crash. Now that the crash has been fixed, this is a silent failure.Add an assert in this codepath to detect this particular condition. Note that the right fix here may be to pass -1 to
xfs_da3_node_read()such that a hole returns an error. This is a cautious first step in that direction.Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_attr_inactive.c | 1 + 1 file changed, 1 insertion(+)
Quoted 14 lines
fs/xfs/xfs_attr_inactive.c xfs_attr3_node_inactive( · line 258
Quoted 9 lines
fs/xfs/xfs_attr_inactive.c +1
-0@@ -255,6 +255,7 @@ xfs_attr3_node_inactive( XFS_ATTR_FORK);if (error)return error; +ASSERT (child_bp);
xfs_dabuf_mapwill log an error message and return-EFSCORRUPTEDif mappedbno == -1, so (afaict) you might as well do that instead of adding the ASSERT. We'll still see the dmesg report.
Yep, noted above. I wasn't sure if we wanted to fail the broader inactivate, but I'm Ok with it if you are. :) I'll run some tests with that change instead.
Brian
Quoted 16 lines
--D
Quoted 10 lines
fs/xfs/xfs_attr_inactive.c +0
-0if (child_bp) {/* save for re-read later */ child_blkno =XFS_BUF_ADDR (child_bp);Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 xfs: fail if xattr inactivation hits a hole
In reply to Brian FosterThe child buffer read in xfs_attr3_node_inactive() should never reach a hole in the attr fork. If this occurs, it is likely due to a bug. Prior to commit cd87d867 ("xfs: don't crash on unexpected holes in dir/attr btrees"), this would result in a crash. Now that the crash has been fixed, this is a silent failure.
Pass -1 to xfs_da3_node_read() from xfs_da3_node_inactive() to indicate that reading from a hole is an error. This logs an error to syslog and fails the inode inactivation, leaving the inode on the AG unlinked list until removed by xfs_repair (or log recovery). Also update the subsequent code to reflect that the read now returns a non-NULL buffer or an error.
--- fs/xfs/xfs_attr_inactive.c | 69 ++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 36 deletions(-)
fs/xfs/xfs_attr_inactive.c +33 -36
@@ -251,47 +251,44 @@ xfs_attr3_node_inactive(* traversal of the tree so we may deal with many blocks * before we come back to this one. */ - error =xfs_da3_node_read (*trans, dp, child_fsb,-2 , &child_bp, -XFS_ATTR_FORK ); + error =xfs_da3_node_read (*trans, dp, child_fsb,-1 , &child_bp, +XFS_ATTR_FORK );if (error)return error; -if (child_bp) { -/* save for re-read later */ - child_blkno =XFS_BUF_ADDR (child_bp); -/* -* Invalidate the subtree, however we have to. -*/ - info = child_bp->b_addr; -switch (info->magic) { -case cpu_to_be16 (XFS_DA_NODE_MAGIC ): -case cpu_to_be16 (XFS_DA3_NODE_MAGIC ): - error =xfs_attr3_node_inactive (trans, dp, - child_bp, level +1 ); -break ; -case cpu_to_be16 (XFS_ATTR_LEAF_MAGIC ): -case cpu_to_be16 (XFS_ATTR3_LEAF_MAGIC ): - error =xfs_attr3_leaf_inactive (trans, dp, - child_bp); -break ; -default : - error = -EIO ; -xfs_trans_brelse (*trans, child_bp); -break ; - } -if (error) -return error; +/* save for re-read later */ + child_blkno =XFS_BUF_ADDR (child_bp); -/* -* Remove the subsidiary block from the cache -* and from the log. -*/ - error =xfs_da_get_buf (*trans, dp,0 , child_blkno, - &child_bp,XFS_ATTR_FORK ); -if (error) -return error; -xfs_trans_binval (*trans, child_bp); +/* +* Invalidate the subtree, however we have to. +*/ + info = child_bp->b_addr; +switch (info->magic) { +case cpu_to_be16 (XFS_DA_NODE_MAGIC ): +case cpu_to_be16 (XFS_DA3_NODE_MAGIC ): + error =xfs_attr3_node_inactive (trans, dp, child_bp, + level +1 ); +break ; +case cpu_to_be16 (XFS_ATTR_LEAF_MAGIC ): +case cpu_to_be16 (XFS_ATTR3_LEAF_MAGIC ): + error =xfs_attr3_leaf_inactive (trans, dp, child_bp); +break ; +default : + error = -EIO ; +xfs_trans_brelse (*trans, child_bp); +break ; } +if (error) +return error; + +/* +* Remove the subsidiary block from the cache and from the log. +*/ + error =xfs_da_get_buf (*trans, dp,0 , child_blkno, &child_bp, +XFS_ATTR_FORK ); +if (error) +return error; +xfs_trans_binval (*trans, child_bp);/* * If we're not done, re-read the parent to get the next
Signature · 1 line
2.9.5
Re: [PATCH v2] xfs: fail if xattr inactivation hits a hole
Quoted 6 lines · Brian Foster
The child buffer read in
xfs_attr3_node_inactive()should never reach a hole in the attr fork. If this occurs, it is likely due to a bug. Prior to commitcd87d867("xfs: don't crash on unexpected holes in dir/attr btrees"), this would result in a crash. Now that the crash has been fixed, this is a silent failure.
Quoted 8 lines
Pass -1 to
xfs_da3_node_read()fromxfs_da3_node_inactive()to indicate that reading from a hole is an error. This logs an error to syslog and fails the inode inactivation, leaving the inode on the AG unlinked list until removed byxfs_repair(or log recovery). Also update the subsequent code to reflect that the read now returns a non-NULL buffer or an error.Signed-off-by Brian Foster <[email protected]>
Looks ok,
Since we've already fixed the crash problem and the dangling pointer problem (as soon as I send this week's fixes to Linus, anyway), I think this can wait for 4.15, unless anyone wants me to push sooner.
--D
Quoted 96 lines
--- fs/xfs/xfs_attr_inactive.c | 69 ++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 36 deletions(-)fs/xfs/xfs_attr_inactive.c +33
-36@@ -251,47 +251,44 @@ xfs_attr3_node_inactive(* traversal of the tree so we may deal with many blocks * before we come back to this one. */ - error =xfs_da3_node_read (*trans, dp, child_fsb,-2 , &child_bp, -XFS_ATTR_FORK ); + error =xfs_da3_node_read (*trans, dp, child_fsb,-1 , &child_bp, +XFS_ATTR_FORK );if (error)return error; -if (child_bp) { -/* save for re-read later */ - child_blkno =XFS_BUF_ADDR (child_bp); -/* -* Invalidate the subtree, however we have to. -*/ - info = child_bp->b_addr; -switch (info->magic) { -case cpu_to_be16 (XFS_DA_NODE_MAGIC ): -case cpu_to_be16 (XFS_DA3_NODE_MAGIC ): - error =xfs_attr3_node_inactive (trans, dp, - child_bp, level +1 ); -break ; -case cpu_to_be16 (XFS_ATTR_LEAF_MAGIC ): -case cpu_to_be16 (XFS_ATTR3_LEAF_MAGIC ): - error =xfs_attr3_leaf_inactive (trans, dp, - child_bp); -break ; -default : - error = -EIO ; -xfs_trans_brelse (*trans, child_bp); -break ; - } -if (error) -return error; +/* save for re-read later */ + child_blkno =XFS_BUF_ADDR (child_bp); -/* -* Remove the subsidiary block from the cache -* and from the log. -*/ - error =xfs_da_get_buf (*trans, dp,0 , child_blkno, - &child_bp,XFS_ATTR_FORK ); -if (error) -return error; -xfs_trans_binval (*trans, child_bp); +/* +* Invalidate the subtree, however we have to. +*/ + info = child_bp->b_addr; +switch (info->magic) { +case cpu_to_be16 (XFS_DA_NODE_MAGIC ): +case cpu_to_be16 (XFS_DA3_NODE_MAGIC ): + error =xfs_attr3_node_inactive (trans, dp, child_bp, + level +1 ); +break ; +case cpu_to_be16 (XFS_ATTR_LEAF_MAGIC ): +case cpu_to_be16 (XFS_ATTR3_LEAF_MAGIC ): + error =xfs_attr3_leaf_inactive (trans, dp, child_bp); +break ; +default : + error = -EIO ; +xfs_trans_brelse (*trans, child_bp); +break ; } +if (error) +return error; + +/* +* Remove the subsidiary block from the cache and from the log. +*/ + error =xfs_da_get_buf (*trans, dp,0 , child_blkno, &child_bp, +XFS_ATTR_FORK ); +if (error) +return error; +xfs_trans_binval (*trans, child_bp);/* * If we're not done, re-read the parent to get the next Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 xfs: buffer lru reference count error injection tag
In reply to Brian FosterXFS uses a fixed reference count for certain types of buffers in the internal LRU cache. These reference counts dictate how aggressively certain buffers are reclaimed vs. others. While the reference counts implements priority across different buffer types, all buffers (other than uncached buffers) are typically cached for at least one reclaim cycle.
We've had at least one bug recently that has been hidden by a released buffer sitting around in the LRU. Users hitting the problem were able to reproduce under enough memory pressure to cause aggressive reclaim in a particular window of time.
To support future xfstests cases, add an error injection tag to hardcode the buffer reference count to zero. When enabled, this bypasses caching of associated buffers and facilitates test cases that depend on this behavior.
--- fs/xfs/xfs_buf.c | 16 ++++++++++++++++ fs/xfs/xfs_buf.h | 5 +---- fs/xfs/xfs_error.c | 3 +++ fs/xfs/xfs_error.h | 4 +++- 4 files changed, 23 insertions(+), 5 deletions(-)
fs/xfs/xfs_buf.c +16 -0
@@ -42,6 +42,7 @@#include "xfs_mount.h" #include "xfs_trace.h" #include "xfs_log.h" +#include "xfs_error.h" static kmem_zone_t *xfs_buf_zone;
@@ -2129,3 +2130,18 @@ xfs_buf_terminate(void) {kmem_zone_destroy (xfs_buf_zone); } + +void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref) +{ +struct xfs_mount *mp = bp->b_target->bt_mount; + +/* +* Set the lru reference count to 0 based on the error injection tag. +* This allows userspace to disrupt buffer caching for debug/testing +* purposes. +*/ +if (XFS_TEST_ERROR (false, mp,XFS_ERRTAG_BUF_LRU_REF )) + lru_ref =0 ; + +atomic_set (&bp->b_lru_ref, lru_ref); +}
fs/xfs/xfs_buf.h +1 -4
@@ -352,10 +352,7 @@ extern void xfs_buf_terminate(void);#define XFS_BUF_ADDR (bp) ((bp)->b_maps[0].bm_bn)#define XFS_BUF_SET_ADDR (bp, bno) ((bp)->b_maps[0].bm_bn = (xfs_daddr_t)(bno)) -static inline void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref) -{ -atomic_set (&bp->b_lru_ref, lru_ref); -} +void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref);static inline int xfs_buf_ispinned (struct xfs_buf *bp) {
fs/xfs/xfs_error.c +3 -0
@@ -58,6 +58,7 @@ static unsigned int xfs_errortag_random_default[] = {XFS_RANDOM_DROP_WRITES ,XFS_RANDOM_LOG_BAD_CRC ,XFS_RANDOM_LOG_ITEM_PIN , + XFS_RANDOM_BUF_LRU_REF, }; struct xfs_errortag_attr {
@@ -163,6 +164,7 @@ XFS_ERRORTAG_ATTR_RW(ag_resv_critical, XFS_ERRTAG_AG_RESV_CRITICAL);XFS_ERRORTAG_ATTR_RW (drop_writes,XFS_ERRTAG_DROP_WRITES );XFS_ERRORTAG_ATTR_RW (log_bad_crc,XFS_ERRTAG_LOG_BAD_CRC );XFS_ERRORTAG_ATTR_RW (log_item_pin,XFS_ERRTAG_LOG_ITEM_PIN ); +XFS_ERRORTAG_ATTR_RW (buf_lru_ref,XFS_ERRTAG_BUF_LRU_REF );static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (noerror),
@@ -196,6 +198,7 @@ static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (drop_writes),XFS_ERRORTAG_ATTR_LIST (log_bad_crc),XFS_ERRORTAG_ATTR_LIST (log_item_pin), +XFS_ERRORTAG_ATTR_LIST (buf_lru_ref), NULL, };
fs/xfs/xfs_error.h +3 -1
@@ -107,7 +107,8 @@ extern void xfs_verifier_error(struct xfs_buf *bp);#define XFS_ERRTAG_DROP_WRITES 28#define XFS_ERRTAG_LOG_BAD_CRC 29#define XFS_ERRTAG_LOG_ITEM_PIN 30 -#define XFS_ERRTAG_MAX 31 +#define XFS_ERRTAG_BUF_LRU_REF 31 +#define XFS_ERRTAG_MAX 32/* * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -143,6 +144,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);#define XFS_RANDOM_DROP_WRITES 1#define XFS_RANDOM_LOG_BAD_CRC 1#define XFS_RANDOM_LOG_ITEM_PIN 1 +#define XFS_RANDOM_BUF_LRU_REF 2#ifdef DEBUG extern int xfs_errortag_init (struct xfs_mount *mp);
Signature · 1 line
2.9.5
Re: [PATCH 2/2] xfs: buffer lru reference count error injection tag
Quoted 12 lines · Brian Foster
XFS uses a fixed reference count for certain types of buffers in the internal LRU cache. These reference counts dictate how aggressively certain buffers are reclaimed vs. others. While the reference counts implements priority across different buffer types, all buffers (other than uncached buffers) are typically cached for at least one reclaim cycle.
We've had at least one bug recently that has been hidden by a released buffer sitting around in the LRU. Users hitting the problem were able to reproduce under enough memory pressure to cause aggressive reclaim in a particular window of time.
Quoted 6 lines
To support future xfstests cases, add an error injection tag to hardcode the buffer reference count to zero. When enabled, this bypasses caching of associated buffers and facilitates test cases that depend on this behavior.
Signed-off-by Brian Foster <[email protected]>
Looks ok,
Quoted 111 lines
--- fs/xfs/xfs_buf.c | 16 ++++++++++++++++ fs/xfs/xfs_buf.h | 5 +---- fs/xfs/xfs_error.c | 3 +++ fs/xfs/xfs_error.h | 4 +++- 4 files changed, 23 insertions(+), 5 deletions(-)fs/xfs/xfs_buf.c +16
-0@@ -42,6 +42,7 @@#include "xfs_mount.h" #include "xfs_trace.h" #include "xfs_log.h" +#include "xfs_error.h" static kmem_zone_t *xfs_buf_zone;@@ -2129,3 +2130,18 @@ xfs_buf_terminate(void) {kmem_zone_destroy (xfs_buf_zone); } + +void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref) +{ +struct xfs_mount *mp = bp->b_target->bt_mount; + +/* +* Set the lru reference count to 0 based on the error injection tag. +* This allows userspace to disrupt buffer caching for debug/testing +* purposes. +*/ +if (XFS_TEST_ERROR (false, mp,XFS_ERRTAG_BUF_LRU_REF )) + lru_ref =0 ; + +atomic_set (&bp->b_lru_ref, lru_ref); +}fs/xfs/xfs_buf.h +1
-4@@ -352,10 +352,7 @@ extern void xfs_buf_terminate(void);#define XFS_BUF_ADDR (bp) ((bp)->b_maps[0].bm_bn)#define XFS_BUF_SET_ADDR (bp, bno) ((bp)->b_maps[0].bm_bn = (xfs_daddr_t)(bno)) -static inline void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref) -{ -atomic_set (&bp->b_lru_ref, lru_ref); -} +void xfs_buf_set_ref (struct xfs_buf *bp,int lru_ref);static inline int xfs_buf_ispinned (struct xfs_buf *bp) {fs/xfs/xfs_error.c +3
-0@@ -58,6 +58,7 @@ static unsigned int xfs_errortag_random_default[] = {XFS_RANDOM_DROP_WRITES ,XFS_RANDOM_LOG_BAD_CRC ,XFS_RANDOM_LOG_ITEM_PIN , + XFS_RANDOM_BUF_LRU_REF, }; struct xfs_errortag_attr {@@ -163,6 +164,7 @@ XFS_ERRORTAG_ATTR_RW(ag_resv_critical, XFS_ERRTAG_AG_RESV_CRITICAL);XFS_ERRORTAG_ATTR_RW (drop_writes,XFS_ERRTAG_DROP_WRITES );XFS_ERRORTAG_ATTR_RW (log_bad_crc,XFS_ERRTAG_LOG_BAD_CRC );XFS_ERRORTAG_ATTR_RW (log_item_pin,XFS_ERRTAG_LOG_ITEM_PIN ); +XFS_ERRORTAG_ATTR_RW (buf_lru_ref,XFS_ERRTAG_BUF_LRU_REF );static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (noerror),@@ -196,6 +198,7 @@ static struct attribute *xfs_errortag_attrs[] = {XFS_ERRORTAG_ATTR_LIST (drop_writes),XFS_ERRORTAG_ATTR_LIST (log_bad_crc),XFS_ERRORTAG_ATTR_LIST (log_item_pin), +XFS_ERRORTAG_ATTR_LIST (buf_lru_ref), NULL, };fs/xfs/xfs_error.h +3
-1@@ -107,7 +107,8 @@ extern void xfs_verifier_error(struct xfs_buf *bp);#define XFS_ERRTAG_DROP_WRITES 28#define XFS_ERRTAG_LOG_BAD_CRC 29#define XFS_ERRTAG_LOG_ITEM_PIN 30 -#define XFS_ERRTAG_MAX 31 +#define XFS_ERRTAG_BUF_LRU_REF 31 +#define XFS_ERRTAG_MAX 32/* * Random factors for above tags, 1 means always, 2 means 1/2 time, etc. @@ -143,6 +144,7 @@ extern void xfs_verifier_error(struct xfs_buf *bp);#define XFS_RANDOM_DROP_WRITES 1#define XFS_RANDOM_LOG_BAD_CRC 1#define XFS_RANDOM_LOG_ITEM_PIN 1 +#define XFS_RANDOM_BUF_LRU_REF 2#ifdef DEBUG extern int xfs_errortag_init (struct xfs_mount *mp);Signature · 6 lines
2.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html