* [PATCH 0/2] xfs: test support for xattr inactivate
@ 2017-10-10 16:47 Brian Foster
2017-10-10 16:47 ` [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole Brian Foster
2017-10-10 16:47 ` [PATCH 2/2] xfs: buffer lru reference count error injection tag Brian Foster
0 siblings, 2 replies; 8+ messages in thread
From: Brian Foster @ 2017-10-10 16:47 UTC (permalink / raw)
To: linux-xfs
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(-)
--
2.9.5
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole
2017-10-10 16:47 [PATCH 0/2] xfs: test support for xattr inactivate Brian Foster
@ 2017-10-10 16:47 ` Brian Foster
2017-10-10 16:55 ` Darrick J. Wong
2017-10-12 11:27 ` [PATCH v2] xfs: fail if xattr inactivation hits " Brian Foster
2017-10-10 16:47 ` [PATCH 2/2] xfs: buffer lru reference count error injection tag Brian Foster
1 sibling, 2 replies; 8+ messages in thread
From: Brian Foster @ 2017-10-10 16:47 UTC (permalink / raw)
To: linux-xfs
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.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_attr_inactive.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index ebd66b1..6b4f5c6 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -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);
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: buffer lru reference count error injection tag
2017-10-10 16:47 [PATCH 0/2] xfs: test support for xattr inactivate Brian Foster
2017-10-10 16:47 ` [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole Brian Foster
@ 2017-10-10 16:47 ` Brian Foster
2017-10-10 16:59 ` Darrick J. Wong
1 sibling, 1 reply; 8+ messages in thread
From: Brian Foster @ 2017-10-10 16:47 UTC (permalink / raw)
To: linux-xfs
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.
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 <bfoster@redhat.com>
---
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(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2f97c12..d481dd2 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -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);
+}
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index bf71507d..f873bb7 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -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)
{
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index eaf86f5..6732b0a 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -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,
};
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 7c4bef3..78a7f43 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -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);
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole
2017-10-10 16:47 ` [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole Brian Foster
@ 2017-10-10 16:55 ` Darrick J. Wong
2017-10-10 17:09 ` Brian Foster
2017-10-12 11:27 ` [PATCH v2] xfs: fail if xattr inactivation hits " Brian Foster
1 sibling, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2017-10-10 16:55 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, Oct 10, 2017 at 12:47:55PM -0400, Brian Foster wrote:
> 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.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/xfs_attr_inactive.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index ebd66b1..6b4f5c6 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -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
> if (child_bp) {
> /* save for re-read later */
> child_blkno = XFS_BUF_ADDR(child_bp);
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: buffer lru reference count error injection tag
2017-10-10 16:47 ` [PATCH 2/2] xfs: buffer lru reference count error injection tag Brian Foster
@ 2017-10-10 16:59 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2017-10-10 16:59 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, Oct 10, 2017 at 12:47:56PM -0400, Brian Foster wrote:
> 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.
>
> 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 <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> 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(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 2f97c12..d481dd2 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -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);
> +}
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index bf71507d..f873bb7 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -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)
> {
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index eaf86f5..6732b0a 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -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,
> };
>
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 7c4bef3..78a7f43 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -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);
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole
2017-10-10 16:55 ` Darrick J. Wong
@ 2017-10-10 17:09 ` Brian Foster
0 siblings, 0 replies; 8+ messages in thread
From: Brian Foster @ 2017-10-10 17:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Oct 10, 2017 at 09:55:46AM -0700, Darrick J. Wong wrote:
> On Tue, Oct 10, 2017 at 12:47:55PM -0400, Brian Foster wrote:
> > 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.
> >
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > fs/xfs/xfs_attr_inactive.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > index ebd66b1..6b4f5c6 100644
> > --- a/fs/xfs/xfs_attr_inactive.c
> > +++ b/fs/xfs/xfs_attr_inactive.c
> > @@ -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.
>
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
> --D
>
> > if (child_bp) {
> > /* save for re-read later */
> > child_blkno = XFS_BUF_ADDR(child_bp);
> > --
> > 2.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] xfs: fail if xattr inactivation hits a hole
2017-10-10 16:47 ` [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole Brian Foster
2017-10-10 16:55 ` Darrick J. Wong
@ 2017-10-12 11:27 ` Brian Foster
2017-10-12 19:55 ` Darrick J. Wong
1 sibling, 1 reply; 8+ messages in thread
From: Brian Foster @ 2017-10-12 11:27 UTC (permalink / raw)
To: linux-xfs
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.
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.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_attr_inactive.c | 69 ++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 36 deletions(-)
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index e3a950e..52818ea 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -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
--
2.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] xfs: fail if xattr inactivation hits a hole
2017-10-12 11:27 ` [PATCH v2] xfs: fail if xattr inactivation hits " Brian Foster
@ 2017-10-12 19:55 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2017-10-12 19:55 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Thu, Oct 12, 2017 at 07:27:32AM -0400, Brian Foster wrote:
> 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.
>
> 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.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
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
> ---
> fs/xfs/xfs_attr_inactive.c | 69 ++++++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index e3a950e..52818ea 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -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
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-10-12 19:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-10 16:47 [PATCH 0/2] xfs: test support for xattr inactivate Brian Foster
2017-10-10 16:47 ` [PATCH 1/2] xfs: assert that xattr inactivation never reaches a hole Brian Foster
2017-10-10 16:55 ` Darrick J. Wong
2017-10-10 17:09 ` Brian Foster
2017-10-12 11:27 ` [PATCH v2] xfs: fail if xattr inactivation hits " Brian Foster
2017-10-12 19:55 ` Darrick J. Wong
2017-10-10 16:47 ` [PATCH 2/2] xfs: buffer lru reference count error injection tag Brian Foster
2017-10-10 16:59 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.