On Wed, Mar 25, 2020 at 04:00:28PM +1100, Dave Chinner wrote:
> On Tue, Mar 24, 2020 at 08:24:43PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Validate the geometry of the realtime geometry when we mount the
> > filesystem, so that we don't abruptly shut down the filesystem later on.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_sb.c | 35 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 35 insertions(+)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 2f60fc3c99a0..dee0a1a594dc 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -328,6 +328,41 @@ xfs_validate_sb_common(
> > return -EFSCORRUPTED;
> > }
> >
> > + /* Validate the realtime geometry; stolen from xfs_repair */
> > + if (unlikely(
> > + sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE ||
>
> Whacky whitespace before the ||
>
> > + sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)) {
> > + xfs_notice(mp,
> > + "realtime extent sanity check failed");
> > + return -EFSCORRUPTED;
> > + }
>
> We really don't need unlikely() in code like this. the compiler
> already considers code that returns inside an if statement as
> "unlikely" because it's the typical error handling pattern, for
> cases like this it really isn't necessary.
>
>
> > +
> > + if (sbp->sb_rblocks == 0) {
> > + if (unlikely(
> > + sbp->sb_rextents != 0 ||
> > + sbp->sb_rbmblocks != 0 ||
> > + sbp->sb_rextslog != 0 ||
> > + sbp->sb_frextents != 0)) {
>
> Ditto on the unlikely and the whitespace. That code looks weird...
>
> if (sbp->sb_rextents || sbp->sb_rbmblocks ||
> sbp->sb_rextslog || sbp->sb_frextents) {
>
> > + xfs_notice(mp,
> > + "realtime zeroed geometry sanity check failed");
> > + return -EFSCORRUPTED;
> > + }
> > + } else {
> > + xfs_rtblock_t rexts;
> > + uint32_t temp;
> > +
> > + rexts = div_u64_rem(sbp->sb_rblocks, sbp->sb_rextsize, &temp);
> > + if (unlikely(
> > + rexts != sbp->sb_rextents ||
> > + sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents) ||
>
> And again.
>
> At least you're consistent, Darrick :)
Copy-pastaing the weird style of the rest of that function. :)
I'll fix it & resend.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
