| Message ID | 75f628bc-e8e7-b4fd-0bbb-fb3ca0fabac8@sandeen.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs_repair: rework inode clearing and free inode validation | expand |
On 7/23/18 7:34 PM, Eric Sandeen wrote: > xfs_repair checks allocated but unused (free) inodes in on-disk clusters, > and up until now silently repairs any errors, and as a result does not > alter exit status if errors are found. > > The in-kernel verifiers will be noisy about these errors and instruct > the user to run repair, so it's best if repair is explicit about any > fixes it makes as a result. > > To ensure we catch anything the kernel would complain about, re-use > xfs_dinode_verify to determine whether we must clear a free inode. > > Note, however, that the verifier contains only a subset of the checks > currently in clear_dinode. This should be ok; if it's not, the checks > should be added to the verifier in any case. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Ugh, dammit, pulled the trigger too soon, tests were looking fine until the last few. Need to look at this more. > --- > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index a98483b..05c06a7 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -134,6 +134,7 @@ > #define xfs_symlink_hdr_ok libxfs_symlink_hdr_ok > > #define xfs_verify_cksum libxfs_verify_cksum > +#define xfs_dinode_verify libxfs_dinode_verify > > #define xfs_alloc_ag_max_usable libxfs_alloc_ag_max_usable > #define xfs_allocbt_maxrecs libxfs_allocbt_maxrecs > diff --git a/repair/dinode.c b/repair/dinode.c > index d36338f..c0db15a 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > */ > if (was_free) { > /* > - * easy case, inode free -- inode and map agree, clear > + * easy case, inode free -- inode and map agree, check > * it just in case to ensure that format, etc. are > * set correctly > */ > - if (!no_modify) > - *dirty += clear_dinode(mp, dino, lino); > + if (libxfs_dinode_verify(mp, lino, dino) != NULL) { > + do_warn( > + _("free inode %" PRIu64 " contains errors, "), lino); > + if (!no_modify) { > + *dirty += clear_dinode(mp, dino, lino); > + do_warn(_("corrected\n")); > + } else { > + do_warn(_("would correct\n")); > + } > + } > *used = is_free; > return 0; > } > > -- > 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 7/23/18 9:35 PM, Eric Sandeen wrote: > On 7/23/18 7:34 PM, Eric Sandeen wrote: >> xfs_repair checks allocated but unused (free) inodes in on-disk clusters, >> and up until now silently repairs any errors, and as a result does not >> alter exit status if errors are found. >> >> The in-kernel verifiers will be noisy about these errors and instruct >> the user to run repair, so it's best if repair is explicit about any >> fixes it makes as a result. >> >> To ensure we catch anything the kernel would complain about, re-use >> xfs_dinode_verify to determine whether we must clear a free inode. >> >> Note, however, that the verifier contains only a subset of the checks >> currently in clear_dinode. This should be ok; if it's not, the checks >> should be added to the verifier in any case. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > Ugh, dammit, pulled the trigger too soon, tests were looking fine until > the last few. Need to look at this more. With the free inode extent validator fix I sent earlier today I think it's safe to resume normal review activities for these 2 patches. :) Thanks, -Eric -- 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 Mon, Jul 23, 2018 at 07:34:13PM -0700, Eric Sandeen wrote: > xfs_repair checks allocated but unused (free) inodes in on-disk clusters, > and up until now silently repairs any errors, and as a result does not > alter exit status if errors are found. > > The in-kernel verifiers will be noisy about these errors and instruct > the user to run repair, so it's best if repair is explicit about any > fixes it makes as a result. > > To ensure we catch anything the kernel would complain about, re-use > xfs_dinode_verify to determine whether we must clear a free inode. > > Note, however, that the verifier contains only a subset of the checks > currently in clear_dinode. This should be ok; if it's not, the checks > should be added to the verifier in any case. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > Looks good Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index a98483b..05c06a7 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -134,6 +134,7 @@ > #define xfs_symlink_hdr_ok libxfs_symlink_hdr_ok > > #define xfs_verify_cksum libxfs_verify_cksum > +#define xfs_dinode_verify libxfs_dinode_verify > > #define xfs_alloc_ag_max_usable libxfs_alloc_ag_max_usable > #define xfs_allocbt_maxrecs libxfs_allocbt_maxrecs > diff --git a/repair/dinode.c b/repair/dinode.c > index d36338f..c0db15a 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > */ > if (was_free) { > /* > - * easy case, inode free -- inode and map agree, clear > + * easy case, inode free -- inode and map agree, check > * it just in case to ensure that format, etc. are > * set correctly > */ > - if (!no_modify) > - *dirty += clear_dinode(mp, dino, lino); > + if (libxfs_dinode_verify(mp, lino, dino) != NULL) { > + do_warn( > + _("free inode %" PRIu64 " contains errors, "), lino); > + if (!no_modify) { > + *dirty += clear_dinode(mp, dino, lino); > + do_warn(_("corrected\n")); > + } else { > + do_warn(_("would correct\n")); > + } > + } > *used = is_free; > return 0; > } > > -- > 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 Mon, Jul 23, 2018 at 07:34:13PM -0700, Eric Sandeen wrote: > xfs_repair checks allocated but unused (free) inodes in on-disk clusters, > and up until now silently repairs any errors, and as a result does not > alter exit status if errors are found. > > The in-kernel verifiers will be noisy about these errors and instruct > the user to run repair, so it's best if repair is explicit about any > fixes it makes as a result. > > To ensure we catch anything the kernel would complain about, re-use > xfs_dinode_verify to determine whether we must clear a free inode. > > Note, however, that the verifier contains only a subset of the checks > currently in clear_dinode. This should be ok; if it's not, the checks > should be added to the verifier in any case. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h > index a98483b..05c06a7 100644 > --- a/libxfs/libxfs_api_defs.h > +++ b/libxfs/libxfs_api_defs.h > @@ -134,6 +134,7 @@ > #define xfs_symlink_hdr_ok libxfs_symlink_hdr_ok > > #define xfs_verify_cksum libxfs_verify_cksum > +#define xfs_dinode_verify libxfs_dinode_verify > > #define xfs_alloc_ag_max_usable libxfs_alloc_ag_max_usable > #define xfs_allocbt_maxrecs libxfs_allocbt_maxrecs > diff --git a/repair/dinode.c b/repair/dinode.c > index d36338f..c0db15a 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > */ > if (was_free) { > /* > - * easy case, inode free -- inode and map agree, clear > + * easy case, inode free -- inode and map agree, check > * it just in case to ensure that format, etc. are > * set correctly > */ > - if (!no_modify) > - *dirty += clear_dinode(mp, dino, lino); > + if (libxfs_dinode_verify(mp, lino, dino) != NULL) { > + do_warn( > + _("free inode %" PRIu64 " contains errors, "), lino); > + if (!no_modify) { > + *dirty += clear_dinode(mp, dino, lino); > + do_warn(_("corrected\n")); > + } else { > + do_warn(_("would correct\n")); > + } > + } > *used = is_free; > return 0; > } > > -- > 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/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h index a98483b..05c06a7 100644 --- a/libxfs/libxfs_api_defs.h +++ b/libxfs/libxfs_api_defs.h @@ -134,6 +134,7 @@ #define xfs_symlink_hdr_ok libxfs_symlink_hdr_ok #define xfs_verify_cksum libxfs_verify_cksum +#define xfs_dinode_verify libxfs_dinode_verify #define xfs_alloc_ag_max_usable libxfs_alloc_ag_max_usable #define xfs_allocbt_maxrecs libxfs_allocbt_maxrecs diff --git a/repair/dinode.c b/repair/dinode.c index d36338f..c0db15a 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), */ if (was_free) { /* - * easy case, inode free -- inode and map agree, clear + * easy case, inode free -- inode and map agree, check * it just in case to ensure that format, etc. are * set correctly */ - if (!no_modify) - *dirty += clear_dinode(mp, dino, lino); + if (libxfs_dinode_verify(mp, lino, dino) != NULL) { + do_warn( + _("free inode %" PRIu64 " contains errors, "), lino); + if (!no_modify) { + *dirty += clear_dinode(mp, dino, lino); + do_warn(_("corrected\n")); + } else { + do_warn(_("would correct\n")); + } + } *used = is_free; return 0; }
xfs_repair checks allocated but unused (free) inodes in on-disk clusters, and up until now silently repairs any errors, and as a result does not alter exit status if errors are found. The in-kernel verifiers will be noisy about these errors and instruct the user to run repair, so it's best if repair is explicit about any fixes it makes as a result. To ensure we catch anything the kernel would complain about, re-use xfs_dinode_verify to determine whether we must clear a free inode. Note, however, that the verifier contains only a subset of the checks currently in clear_dinode. This should be ok; if it's not, the checks should be added to the verifier in any case. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- -- 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