From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/9] xfs: report block map corruption errors to the health tracking system
Date: Wed, 20 Nov 2019 09:21:19 -0500 [thread overview]
Message-ID: <20191120142119.GD15542@bfoster> (raw)
In-Reply-To: <157375557349.3692735.15868119551132443897.stgit@magnolia>
On Thu, Nov 14, 2019 at 10:19:33AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Whenever we encounter a corrupt block mapping, we should report that to
> the health monitoring system for later reporting.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 39 +++++++++++++++++++++++++++++++++------
> fs/xfs/libxfs/xfs_health.h | 1 +
> fs/xfs/xfs_health.c | 26 ++++++++++++++++++++++++++
> fs/xfs/xfs_iomap.c | 15 +++++++++++----
> 4 files changed, 71 insertions(+), 10 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 4acc6e37c31d..c4674fb0bfb4 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -35,7 +35,7 @@
> #include "xfs_refcount.h"
> #include "xfs_icache.h"
> #include "xfs_iomap.h"
> -
> +#include "xfs_health.h"
>
> kmem_zone_t *xfs_bmap_free_item_zone;
>
> @@ -732,6 +732,7 @@ xfs_bmap_extents_to_btree(
> xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
> abp = xfs_btree_get_bufl(mp, tp, args.fsbno);
> if (XFS_IS_CORRUPT(mp, !abp)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> error = -EFSCORRUPTED;
> goto out_unreserve_dquot;
> }
> @@ -1021,6 +1022,7 @@ xfs_bmap_add_attrfork_local(
>
> /* should only be called for types that support local format data */
> ASSERT(0);
> + xfs_bmap_mark_sick(ip, XFS_ATTR_FORK);
> return -EFSCORRUPTED;
> }
Is it really the attr fork that's corrupt if we get here?
>
> @@ -1090,6 +1092,7 @@ xfs_bmap_add_attrfork(
> if (XFS_IFORK_Q(ip))
> goto trans_cancel;
> if (XFS_IS_CORRUPT(mp, ip->i_d.di_anextents != 0)) {
> + xfs_bmap_mark_sick(ip, XFS_ATTR_FORK);
Similar question here given we haven't added the fork yet. di_anextents
is at least related I suppose, but it's not clear that
scrubbing/repairing the attr fork is what needs to happen.
> error = -EFSCORRUPTED;
> goto trans_cancel;
> }
...
> @@ -1239,6 +1244,7 @@ xfs_iread_extents(
> if (XFS_IS_CORRUPT(mp,
> XFS_IFORK_FORMAT(ip, whichfork) !=
> XFS_DINODE_FMT_BTREE)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> error = -EFSCORRUPTED;
> goto out;
> }
> @@ -1254,6 +1260,7 @@ xfs_iread_extents(
>
> if (XFS_IS_CORRUPT(mp,
> ir.loaded != XFS_IFORK_NEXTENTS(ip, whichfork))) {
> + xfs_bmap_mark_sick(ip, whichfork);
> error = -EFSCORRUPTED;
> goto out;
> }
> @@ -1262,6 +1269,8 @@ xfs_iread_extents(
> ifp->if_flags |= XFS_IFEXTENTS;
> return 0;
> out:
> + if (xfs_metadata_is_sick(error))
> + xfs_bmap_mark_sick(ip, whichfork);
> xfs_iext_destroy(ifp);
> return error;
> }
Duplicate calls in xfs_iread_extents()?
Brian
> @@ -1344,6 +1353,7 @@ xfs_bmap_last_before(
> break;
> default:
> ASSERT(0);
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -1443,8 +1453,11 @@ xfs_bmap_last_offset(
> if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL)
> return 0;
>
> - if (XFS_IS_CORRUPT(ip->i_mount, !xfs_ifork_has_extents(ip, whichfork)))
> + if (XFS_IS_CORRUPT(ip->i_mount,
> + !xfs_ifork_has_extents(ip, whichfork))) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> + }
>
> error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, &is_empty);
> if (error || is_empty)
> @@ -3905,6 +3918,7 @@ xfs_bmapi_read(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -3935,6 +3949,7 @@ xfs_bmapi_read(
> xfs_alert(mp, "%s: inode %llu missing fork %d",
> __func__, ip->i_ino, whichfork);
> #endif /* DEBUG */
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -4414,6 +4429,7 @@ xfs_bmapi_write(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -4621,9 +4637,11 @@ xfs_bmapi_convert_delalloc(
> error = -ENOSPC;
> if (WARN_ON_ONCE(bma.blkno == NULLFSBLOCK))
> goto out_finish;
> - error = -EFSCORRUPTED;
> - if (WARN_ON_ONCE(!xfs_valid_startblock(ip, bma.got.br_startblock)))
> + if (WARN_ON_ONCE(!xfs_valid_startblock(ip, bma.got.br_startblock))) {
> + xfs_bmap_mark_sick(ip, whichfork);
> + error = -EFSCORRUPTED;
> goto out_finish;
> + }
>
> XFS_STATS_ADD(mp, xs_xstrat_bytes, XFS_FSB_TO_B(mp, bma.length));
> XFS_STATS_INC(mp, xs_xstrat_quick);
> @@ -4681,6 +4699,7 @@ xfs_bmapi_remap(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -5319,8 +5338,10 @@ __xfs_bunmapi(
> whichfork = xfs_bmapi_whichfork(flags);
> ASSERT(whichfork != XFS_COW_FORK);
> ifp = XFS_IFORK_PTR(ip, whichfork);
> - if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)))
> + if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork))) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> + }
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> @@ -5815,6 +5836,7 @@ xfs_bmap_collapse_extents(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -5932,6 +5954,7 @@ xfs_bmap_insert_extents(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -6038,6 +6061,7 @@ xfs_bmap_split_extent_at(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> }
>
> @@ -6253,8 +6277,10 @@ xfs_bmap_finish_one(
> XFS_FSB_TO_AGBNO(tp->t_mountp, startblock),
> ip->i_ino, whichfork, startoff, *blockcount, state);
>
> - if (WARN_ON_ONCE(whichfork != XFS_DATA_FORK))
> + if (WARN_ON_ONCE(whichfork != XFS_DATA_FORK)) {
> + xfs_bmap_mark_sick(ip, whichfork);
> return -EFSCORRUPTED;
> + }
>
> if (XFS_TEST_ERROR(false, tp->t_mountp,
> XFS_ERRTAG_BMAP_FINISH_ONE))
> @@ -6272,6 +6298,7 @@ xfs_bmap_finish_one(
> break;
> default:
> ASSERT(0);
> + xfs_bmap_mark_sick(ip, whichfork);
> error = -EFSCORRUPTED;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_health.h b/fs/xfs/libxfs/xfs_health.h
> index ce8954a10c66..25b61180b562 100644
> --- a/fs/xfs/libxfs/xfs_health.h
> +++ b/fs/xfs/libxfs/xfs_health.h
> @@ -138,6 +138,7 @@ void xfs_inode_measure_sickness(struct xfs_inode *ip, unsigned int *sick,
> unsigned int *checked);
>
> void xfs_health_unmount(struct xfs_mount *mp);
> +void xfs_bmap_mark_sick(struct xfs_inode *ip, int whichfork);
>
> /* Now some helpers. */
>
> diff --git a/fs/xfs/xfs_health.c b/fs/xfs/xfs_health.c
> index 36c32b108b39..5e5de5338476 100644
> --- a/fs/xfs/xfs_health.c
> +++ b/fs/xfs/xfs_health.c
> @@ -452,3 +452,29 @@ xfs_bulkstat_health(
> bs->bs_sick |= m->ioctl_mask;
> }
> }
> +
> +/* Mark a block mapping sick. */
> +void
> +xfs_bmap_mark_sick(
> + struct xfs_inode *ip,
> + int whichfork)
> +{
> + unsigned int mask;
> +
> + switch (whichfork) {
> + case XFS_DATA_FORK:
> + mask = XFS_SICK_INO_BMBTD;
> + break;
> + case XFS_ATTR_FORK:
> + mask = XFS_SICK_INO_BMBTA;
> + break;
> + case XFS_COW_FORK:
> + mask = XFS_SICK_INO_BMBTC;
> + break;
> + default:
> + ASSERT(0);
> + return;
> + }
> +
> + xfs_inode_mark_sick(ip, mask);
> +}
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 28e2d1f37267..c1befb899911 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -27,7 +27,7 @@
> #include "xfs_dquot_item.h"
> #include "xfs_dquot.h"
> #include "xfs_reflink.h"
> -
> +#include "xfs_health.h"
>
> #define XFS_ALLOC_ALIGN(mp, off) \
> (((off) >> mp->m_allocsize_log) << mp->m_allocsize_log)
> @@ -59,8 +59,10 @@ xfs_bmbt_to_iomap(
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_buftarg *target = xfs_inode_buftarg(ip);
>
> - if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock)))
> + if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
> + xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
> return xfs_alert_fsblock_zero(ip, imap);
> + }
>
> if (imap->br_startblock == HOLESTARTBLOCK) {
> iomap->addr = IOMAP_NULL_ADDR;
> @@ -277,8 +279,10 @@ xfs_iomap_write_direct(
> goto out_unlock;
> }
>
> - if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock)))
> + if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
> + xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
> error = xfs_alert_fsblock_zero(ip, imap);
> + }
>
> out_unlock:
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -598,8 +602,10 @@ xfs_iomap_write_unwritten(
> if (error)
> return error;
>
> - if (unlikely(!xfs_valid_startblock(ip, imap.br_startblock)))
> + if (unlikely(!xfs_valid_startblock(ip, imap.br_startblock))) {
> + xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
> return xfs_alert_fsblock_zero(ip, &imap);
> + }
>
> if ((numblks_fsb = imap.br_blockcount) == 0) {
> /*
> @@ -858,6 +864,7 @@ xfs_buffered_write_iomap_begin(
>
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, XFS_DATA_FORK)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> + xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
> error = -EFSCORRUPTED;
> goto out_unlock;
> }
>
next prev parent reply other threads:[~2019-11-20 14:21 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-14 18:19 [PATCH v4 0/9] xfs: report corruption to the health trackers Darrick J. Wong
2019-11-14 18:19 ` [PATCH 1/9] xfs: separate the marking of sick and checked metadata Darrick J. Wong
2019-11-20 14:20 ` Brian Foster
2019-11-20 16:12 ` Darrick J. Wong
2019-11-14 18:19 ` [PATCH 2/9] xfs: report ag header corruption errors to the health tracking system Darrick J. Wong
2019-11-20 14:20 ` Brian Foster
2019-11-20 16:43 ` Darrick J. Wong
2019-11-21 13:26 ` Brian Foster
2019-11-22 0:53 ` Darrick J. Wong
2019-11-22 11:57 ` Brian Foster
2019-11-22 18:10 ` Darrick J. Wong
2019-11-14 18:19 ` [PATCH 3/9] xfs: report block map " Darrick J. Wong
2019-11-20 14:21 ` Brian Foster [this message]
2019-11-20 16:57 ` Darrick J. Wong
2019-11-14 18:19 ` [PATCH 4/9] xfs: report btree block corruption errors to the health system Darrick J. Wong
2019-11-14 18:19 ` [PATCH 5/9] xfs: report dir/attr " Darrick J. Wong
2019-11-20 16:11 ` Brian Foster
2019-11-20 16:55 ` Darrick J. Wong
2019-11-21 13:26 ` Brian Foster
2019-11-22 1:03 ` Darrick J. Wong
2019-11-22 12:28 ` Brian Foster
2019-11-22 18:35 ` Darrick J. Wong
2019-11-14 18:19 ` [PATCH 6/9] xfs: report symlink " Darrick J. Wong
2019-11-14 18:19 ` [PATCH 7/9] xfs: report inode " Darrick J. Wong
2019-11-14 18:20 ` [PATCH 8/9] xfs: report quota block " Darrick J. Wong
2019-11-14 18:20 ` [PATCH 9/9] xfs: report realtime metadata " Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191120142119.GD15542@bfoster \
--to=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox