| Message ID | 152107378261.19571.1457221354115584570.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 14, 2018 at 05:29:42PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Refactor the bmap validator into a more complete helper that looks for > extents that run off the end of the device, overflow into the next AG, > or have invalid flag states, leaving a raw helper so that the inode > repair can check things even if iget fails. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 55 ++++++++++++++++++++++++++++++++++++++-- > fs/xfs/libxfs/xfs_bmap.h | 5 ++++ > fs/xfs/libxfs/xfs_bmap_btree.h | 14 ---------- > fs/xfs/libxfs/xfs_inode_fork.c | 12 ++++++--- > 4 files changed, 65 insertions(+), 21 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index daae00e..5850e76 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -1261,11 +1261,15 @@ xfs_iread_extents( > */ > frp = XFS_BMBT_REC_ADDR(mp, block, 1); > for (j = 0; j < num_recs; j++, frp++, i++) { > + xfs_failaddr_t fa; > + > xfs_bmbt_disk_get_all(frp, &new); > - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { > - XFS_ERROR_REPORT("xfs_bmap_read_extents(2)", > - XFS_ERRLEVEL_LOW, mp); > + fa = xfs_bmap_validate_extent(ip, whichfork, &new); > + if (fa) { > error = -EFSCORRUPTED; > + xfs_inode_verifier_error(ip, error, > + "xfs_bmap_read_extents(2)", Might as well fix up the stale function name string. > + frp, sizeof(*frp), fa); > goto out_brelse; > } > xfs_iext_insert(ip, &icur, &new, state); > @@ -6154,3 +6158,48 @@ xfs_bmap_finish_one( > > return error; > } > + > +/* Check that an extent does not have invalid flags or bad ranges. */ > +xfs_failaddr_t > +xfs_bmbt_validate_extent( > + struct xfs_mount *mp, > + bool isrt, > + int whichfork, > + struct xfs_bmbt_irec *irec) > +{ > + xfs_fsblock_t endfsb; > + > + endfsb = irec->br_startblock + irec->br_blockcount - 1; > + if (isrt) { > + if (!xfs_verify_rtbno(mp, irec->br_startblock)) > + return __this_address; > + if (!xfs_verify_rtbno(mp, endfsb)) > + return __this_address; > + } else { > + if (!xfs_verify_fsbno(mp, irec->br_startblock)) > + return __this_address; > + if (!xfs_verify_fsbno(mp, endfsb)) > + return __this_address; > + if (XFS_FSB_TO_AGNO(mp, irec->br_startblock) != > + XFS_FSB_TO_AGNO(mp, endfsb)) > + return __this_address; > + } > + if (irec->br_state != XFS_EXT_NORM) { > + if (whichfork != XFS_DATA_FORK) > + return __this_address; > + if (!xfs_sb_version_hasextflgbit(&mp->m_sb)) > + return __this_address; > + } > + return NULL; > +} > + > +/* Check that an inode's extent does not have invalid flags or bad ranges. */ > +xfs_failaddr_t > +xfs_bmap_validate_extent( > + struct xfs_inode *ip, > + int whichfork, > + struct xfs_bmbt_irec *irec) > +{ > + return xfs_bmbt_validate_extent(ip->i_mount, XFS_IS_REALTIME_INODE(ip), > + whichfork, irec); > +} Do we really need this wrapper just for an rt check? Why not just pass ip down to xfs_bmbt_validate_extent() and pull isrt from there (if present)? The commit log implies the helper could be left around for future repair code. If that's the case, I think it's more appropriate to refactor it as such in whatever patch the helper is first used. Otherwise looks fine. Brian > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index e36d757..e0fef89 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -274,4 +274,9 @@ static inline int xfs_bmap_fork_to_state(int whichfork) > } > } > > +xfs_failaddr_t xfs_bmbt_validate_extent(struct xfs_mount *mp, bool isrt, > + int whichfork, struct xfs_bmbt_irec *irec); > +xfs_failaddr_t xfs_bmap_validate_extent(struct xfs_inode *ip, int whichfork, > + struct xfs_bmbt_irec *irec); > + > #endif /* __XFS_BMAP_H__ */ > diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h > index 135b8c5..e450574 100644 > --- a/fs/xfs/libxfs/xfs_bmap_btree.h > +++ b/fs/xfs/libxfs/xfs_bmap_btree.h > @@ -118,18 +118,4 @@ extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip, > extern struct xfs_btree_cur *xfs_bmbt_init_cursor(struct xfs_mount *, > struct xfs_trans *, struct xfs_inode *, int); > > -/* > - * Check that the extent does not contain an invalid unwritten extent flag. > - */ > -static inline bool xfs_bmbt_validate_extent(struct xfs_mount *mp, int whichfork, > - struct xfs_bmbt_irec *irec) > -{ > - if (irec->br_state == XFS_EXT_NORM) > - return true; > - if (whichfork == XFS_DATA_FORK && > - xfs_sb_version_hasextflgbit(&mp->m_sb)) > - return true; > - return false; > -} > - > #endif /* __XFS_BMAP_BTREE_H__ */ > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > index 866d2861..613fba2 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.c > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -245,10 +245,14 @@ xfs_iformat_extents( > > xfs_iext_first(ifp, &icur); > for (i = 0; i < nex; i++, dp++) { > + xfs_failaddr_t fa; > + > xfs_bmbt_disk_get_all(dp, &new); > - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { > - XFS_ERROR_REPORT("xfs_iformat_extents(2)", > - XFS_ERRLEVEL_LOW, mp); > + fa = xfs_bmap_validate_extent(ip, whichfork, &new); > + if (fa) { > + xfs_inode_verifier_error(ip, -EFSCORRUPTED, > + "xfs_iformat_extents(2)", > + dp, sizeof(*dp), fa); > return -EFSCORRUPTED; > } > > @@ -595,7 +599,7 @@ xfs_iextents_copy( > for_each_xfs_iext(ifp, &icur, &rec) { > if (isnullstartblock(rec.br_startblock)) > continue; > - ASSERT(xfs_bmbt_validate_extent(ip->i_mount, whichfork, &rec)); > + ASSERT(xfs_bmap_validate_extent(ip, whichfork, &rec) == NULL); > xfs_bmbt_disk_set_all(dp, &rec); > trace_xfs_write_extent(ip, &icur, state, _RET_IP_); > copied += sizeof(struct xfs_bmbt_rec); > > -- > 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
On Wed, Mar 21, 2018 at 09:55:20AM -0400, Brian Foster wrote: > On Wed, Mar 14, 2018 at 05:29:42PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Refactor the bmap validator into a more complete helper that looks for > > extents that run off the end of the device, overflow into the next AG, > > or have invalid flag states, leaving a raw helper so that the inode > > repair can check things even if iget fails. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/libxfs/xfs_bmap.c | 55 ++++++++++++++++++++++++++++++++++++++-- > > fs/xfs/libxfs/xfs_bmap.h | 5 ++++ > > fs/xfs/libxfs/xfs_bmap_btree.h | 14 ---------- > > fs/xfs/libxfs/xfs_inode_fork.c | 12 ++++++--- > > 4 files changed, 65 insertions(+), 21 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > index daae00e..5850e76 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.c > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > @@ -1261,11 +1261,15 @@ xfs_iread_extents( > > */ > > frp = XFS_BMBT_REC_ADDR(mp, block, 1); > > for (j = 0; j < num_recs; j++, frp++, i++) { > > + xfs_failaddr_t fa; > > + > > xfs_bmbt_disk_get_all(frp, &new); > > - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { > > - XFS_ERROR_REPORT("xfs_bmap_read_extents(2)", > > - XFS_ERRLEVEL_LOW, mp); > > + fa = xfs_bmap_validate_extent(ip, whichfork, &new); > > + if (fa) { > > error = -EFSCORRUPTED; > > + xfs_inode_verifier_error(ip, error, > > + "xfs_bmap_read_extents(2)", > > Might as well fix up the stale function name string. Ok. > > + frp, sizeof(*frp), fa); > > goto out_brelse; > > } > > xfs_iext_insert(ip, &icur, &new, state); > > @@ -6154,3 +6158,48 @@ xfs_bmap_finish_one( > > > > return error; > > } > > + > > +/* Check that an extent does not have invalid flags or bad ranges. */ > > +xfs_failaddr_t > > +xfs_bmbt_validate_extent( > > + struct xfs_mount *mp, > > + bool isrt, > > + int whichfork, > > + struct xfs_bmbt_irec *irec) > > +{ > > + xfs_fsblock_t endfsb; > > + > > + endfsb = irec->br_startblock + irec->br_blockcount - 1; > > + if (isrt) { > > + if (!xfs_verify_rtbno(mp, irec->br_startblock)) > > + return __this_address; > > + if (!xfs_verify_rtbno(mp, endfsb)) > > + return __this_address; > > + } else { > > + if (!xfs_verify_fsbno(mp, irec->br_startblock)) > > + return __this_address; > > + if (!xfs_verify_fsbno(mp, endfsb)) > > + return __this_address; > > + if (XFS_FSB_TO_AGNO(mp, irec->br_startblock) != > > + XFS_FSB_TO_AGNO(mp, endfsb)) > > + return __this_address; > > + } > > + if (irec->br_state != XFS_EXT_NORM) { > > + if (whichfork != XFS_DATA_FORK) > > + return __this_address; > > + if (!xfs_sb_version_hasextflgbit(&mp->m_sb)) > > + return __this_address; > > + } > > + return NULL; > > +} > > + > > +/* Check that an inode's extent does not have invalid flags or bad ranges. */ > > +xfs_failaddr_t > > +xfs_bmap_validate_extent( > > + struct xfs_inode *ip, > > + int whichfork, > > + struct xfs_bmbt_irec *irec) > > +{ > > + return xfs_bmbt_validate_extent(ip->i_mount, XFS_IS_REALTIME_INODE(ip), > > + whichfork, irec); > > +} > > Do we really need this wrapper just for an rt check? Why not just pass > ip down to xfs_bmbt_validate_extent() and pull isrt from there (if > present)? > > The commit log implies the helper could be left around for future repair > code. If that's the case, I think it's more appropriate to refactor it > as such in whatever patch the helper is first used. Otherwise looks > fine. Ok. FWIW the intended usage for xfs_bmbt_validate_extent is that if the user asks us to repair an inode for which _iget returns EFSCORRUPTED we'll walk through all the inode fork sanity checks; if an extents-format fork has bad bmbt records, we reset the inode fork and indicate that a bmbt rebuild (via the rmapbt) is necessary. In that case there is no xfs_inode hence the need for a raw function. However... I'll move the creation of the wrapper to that patch and we can argue about it there. :) --D > Brian > > > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > > index e36d757..e0fef89 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.h > > +++ b/fs/xfs/libxfs/xfs_bmap.h > > @@ -274,4 +274,9 @@ static inline int xfs_bmap_fork_to_state(int whichfork) > > } > > } > > > > +xfs_failaddr_t xfs_bmbt_validate_extent(struct xfs_mount *mp, bool isrt, > > + int whichfork, struct xfs_bmbt_irec *irec); > > +xfs_failaddr_t xfs_bmap_validate_extent(struct xfs_inode *ip, int whichfork, > > + struct xfs_bmbt_irec *irec); > > + > > #endif /* __XFS_BMAP_H__ */ > > diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h > > index 135b8c5..e450574 100644 > > --- a/fs/xfs/libxfs/xfs_bmap_btree.h > > +++ b/fs/xfs/libxfs/xfs_bmap_btree.h > > @@ -118,18 +118,4 @@ extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip, > > extern struct xfs_btree_cur *xfs_bmbt_init_cursor(struct xfs_mount *, > > struct xfs_trans *, struct xfs_inode *, int); > > > > -/* > > - * Check that the extent does not contain an invalid unwritten extent flag. > > - */ > > -static inline bool xfs_bmbt_validate_extent(struct xfs_mount *mp, int whichfork, > > - struct xfs_bmbt_irec *irec) > > -{ > > - if (irec->br_state == XFS_EXT_NORM) > > - return true; > > - if (whichfork == XFS_DATA_FORK && > > - xfs_sb_version_hasextflgbit(&mp->m_sb)) > > - return true; > > - return false; > > -} > > - > > #endif /* __XFS_BMAP_BTREE_H__ */ > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > > index 866d2861..613fba2 100644 > > --- a/fs/xfs/libxfs/xfs_inode_fork.c > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > > @@ -245,10 +245,14 @@ xfs_iformat_extents( > > > > xfs_iext_first(ifp, &icur); > > for (i = 0; i < nex; i++, dp++) { > > + xfs_failaddr_t fa; > > + > > xfs_bmbt_disk_get_all(dp, &new); > > - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { > > - XFS_ERROR_REPORT("xfs_iformat_extents(2)", > > - XFS_ERRLEVEL_LOW, mp); > > + fa = xfs_bmap_validate_extent(ip, whichfork, &new); > > + if (fa) { > > + xfs_inode_verifier_error(ip, -EFSCORRUPTED, > > + "xfs_iformat_extents(2)", > > + dp, sizeof(*dp), fa); > > return -EFSCORRUPTED; > > } > > > > @@ -595,7 +599,7 @@ xfs_iextents_copy( > > for_each_xfs_iext(ifp, &icur, &rec) { > > if (isnullstartblock(rec.br_startblock)) > > continue; > > - ASSERT(xfs_bmbt_validate_extent(ip->i_mount, whichfork, &rec)); > > + ASSERT(xfs_bmap_validate_extent(ip, whichfork, &rec) == NULL); > > xfs_bmbt_disk_set_all(dp, &rec); > > trace_xfs_write_extent(ip, &icur, state, _RET_IP_); > > copied += sizeof(struct xfs_bmbt_rec); > > > > -- > > 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 -- 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
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index daae00e..5850e76 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1261,11 +1261,15 @@ xfs_iread_extents( */ frp = XFS_BMBT_REC_ADDR(mp, block, 1); for (j = 0; j < num_recs; j++, frp++, i++) { + xfs_failaddr_t fa; + xfs_bmbt_disk_get_all(frp, &new); - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { - XFS_ERROR_REPORT("xfs_bmap_read_extents(2)", - XFS_ERRLEVEL_LOW, mp); + fa = xfs_bmap_validate_extent(ip, whichfork, &new); + if (fa) { error = -EFSCORRUPTED; + xfs_inode_verifier_error(ip, error, + "xfs_bmap_read_extents(2)", + frp, sizeof(*frp), fa); goto out_brelse; } xfs_iext_insert(ip, &icur, &new, state); @@ -6154,3 +6158,48 @@ xfs_bmap_finish_one( return error; } + +/* Check that an extent does not have invalid flags or bad ranges. */ +xfs_failaddr_t +xfs_bmbt_validate_extent( + struct xfs_mount *mp, + bool isrt, + int whichfork, + struct xfs_bmbt_irec *irec) +{ + xfs_fsblock_t endfsb; + + endfsb = irec->br_startblock + irec->br_blockcount - 1; + if (isrt) { + if (!xfs_verify_rtbno(mp, irec->br_startblock)) + return __this_address; + if (!xfs_verify_rtbno(mp, endfsb)) + return __this_address; + } else { + if (!xfs_verify_fsbno(mp, irec->br_startblock)) + return __this_address; + if (!xfs_verify_fsbno(mp, endfsb)) + return __this_address; + if (XFS_FSB_TO_AGNO(mp, irec->br_startblock) != + XFS_FSB_TO_AGNO(mp, endfsb)) + return __this_address; + } + if (irec->br_state != XFS_EXT_NORM) { + if (whichfork != XFS_DATA_FORK) + return __this_address; + if (!xfs_sb_version_hasextflgbit(&mp->m_sb)) + return __this_address; + } + return NULL; +} + +/* Check that an inode's extent does not have invalid flags or bad ranges. */ +xfs_failaddr_t +xfs_bmap_validate_extent( + struct xfs_inode *ip, + int whichfork, + struct xfs_bmbt_irec *irec) +{ + return xfs_bmbt_validate_extent(ip->i_mount, XFS_IS_REALTIME_INODE(ip), + whichfork, irec); +} diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index e36d757..e0fef89 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -274,4 +274,9 @@ static inline int xfs_bmap_fork_to_state(int whichfork) } } +xfs_failaddr_t xfs_bmbt_validate_extent(struct xfs_mount *mp, bool isrt, + int whichfork, struct xfs_bmbt_irec *irec); +xfs_failaddr_t xfs_bmap_validate_extent(struct xfs_inode *ip, int whichfork, + struct xfs_bmbt_irec *irec); + #endif /* __XFS_BMAP_H__ */ diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h index 135b8c5..e450574 100644 --- a/fs/xfs/libxfs/xfs_bmap_btree.h +++ b/fs/xfs/libxfs/xfs_bmap_btree.h @@ -118,18 +118,4 @@ extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip, extern struct xfs_btree_cur *xfs_bmbt_init_cursor(struct xfs_mount *, struct xfs_trans *, struct xfs_inode *, int); -/* - * Check that the extent does not contain an invalid unwritten extent flag. - */ -static inline bool xfs_bmbt_validate_extent(struct xfs_mount *mp, int whichfork, - struct xfs_bmbt_irec *irec) -{ - if (irec->br_state == XFS_EXT_NORM) - return true; - if (whichfork == XFS_DATA_FORK && - xfs_sb_version_hasextflgbit(&mp->m_sb)) - return true; - return false; -} - #endif /* __XFS_BMAP_BTREE_H__ */ diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 866d2861..613fba2 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -245,10 +245,14 @@ xfs_iformat_extents( xfs_iext_first(ifp, &icur); for (i = 0; i < nex; i++, dp++) { + xfs_failaddr_t fa; + xfs_bmbt_disk_get_all(dp, &new); - if (!xfs_bmbt_validate_extent(mp, whichfork, &new)) { - XFS_ERROR_REPORT("xfs_iformat_extents(2)", - XFS_ERRLEVEL_LOW, mp); + fa = xfs_bmap_validate_extent(ip, whichfork, &new); + if (fa) { + xfs_inode_verifier_error(ip, -EFSCORRUPTED, + "xfs_iformat_extents(2)", + dp, sizeof(*dp), fa); return -EFSCORRUPTED; } @@ -595,7 +599,7 @@ xfs_iextents_copy( for_each_xfs_iext(ifp, &icur, &rec) { if (isnullstartblock(rec.br_startblock)) continue; - ASSERT(xfs_bmbt_validate_extent(ip->i_mount, whichfork, &rec)); + ASSERT(xfs_bmap_validate_extent(ip, whichfork, &rec) == NULL); xfs_bmbt_disk_set_all(dp, &rec); trace_xfs_write_extent(ip, &icur, state, _RET_IP_); copied += sizeof(struct xfs_bmbt_rec);