| Message ID | 153262652554.8934.816888163232208002.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] libxfs: add more bounds checking to sb sanity checks | expand |
On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Add a helper predicate to check the inode count for sanity, then use it > in the superblock write verifier to inspect sb_icount. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Looks good. Thanks. Reviewed-by: Bill O'Donnell <billodo@redhat.com> > fs/xfs/libxfs/xfs_sb.c | 1 + > fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ > fs/xfs/libxfs/xfs_types.h | 1 + > 3 files changed, 36 insertions(+) > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index b2c683588519..1659016875f9 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -714,6 +714,7 @@ xfs_sb_write_verify( > * cases. > */ > if (sb.sb_fdblocks > sb.sb_dblocks || > + !xfs_verify_icount(mp, sb.sb_icount) || > sb.sb_ifree > sb.sb_icount) { > xfs_notice(mp, "SB summary counter sanity check failed"); > error = -EFSCORRUPTED; > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c > index 2e2a243cef2e..2e9c0c25ccb6 100644 > --- a/fs/xfs/libxfs/xfs_types.c > +++ b/fs/xfs/libxfs/xfs_types.c > @@ -171,3 +171,37 @@ xfs_verify_rtbno( > { > return rtbno < mp->m_sb.sb_rblocks; > } > + > +/* Calculate the range of valid icount values. */ > +static void > +xfs_icount_range( > + struct xfs_mount *mp, > + unsigned long long *min, > + unsigned long long *max) > +{ > + unsigned long long nr_inos = 0; > + xfs_agnumber_t agno; > + > + /* root, rtbitmap, rtsum all live in the first chunk */ > + *min = XFS_INODES_PER_CHUNK; > + > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > + xfs_agino_t first, last; > + > + xfs_agino_range(mp, agno, &first, &last); > + nr_inos += first - last + 1; > + } > + *max = nr_inos; > +} > + > +/* Sanity-checking of inode counts. */ > +bool > +xfs_verify_icount( > + struct xfs_mount *mp, > + unsigned long long icount) > +{ > + unsigned long long min, max; > + > + xfs_icount_range(mp, &min, &max); > + return icount >= min && icount < max; > +} > diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h > index 4055d62f690c..b9e6c89284c3 100644 > --- a/fs/xfs/libxfs/xfs_types.h > +++ b/fs/xfs/libxfs/xfs_types.h > @@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino); > bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino); > bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino); > bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno); > +bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount); > > #endif /* __XFS_TYPES_H__ */ > > -- > 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 Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Add a helper predicate to check the inode count for sanity, then use it > in the superblock write verifier to inspect sb_icount. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_sb.c | 1 + > fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ > fs/xfs/libxfs/xfs_types.h | 1 + > 3 files changed, 36 insertions(+) > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index b2c683588519..1659016875f9 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -714,6 +714,7 @@ xfs_sb_write_verify( > * cases. > */ > if (sb.sb_fdblocks > sb.sb_dblocks || > + !xfs_verify_icount(mp, sb.sb_icount) || > sb.sb_ifree > sb.sb_icount) { > xfs_notice(mp, "SB summary counter sanity check failed"); > error = -EFSCORRUPTED; > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c > index 2e2a243cef2e..2e9c0c25ccb6 100644 > --- a/fs/xfs/libxfs/xfs_types.c > +++ b/fs/xfs/libxfs/xfs_types.c > @@ -171,3 +171,37 @@ xfs_verify_rtbno( > { > return rtbno < mp->m_sb.sb_rblocks; > } > + > +/* Calculate the range of valid icount values. */ > +static void > +xfs_icount_range( > + struct xfs_mount *mp, > + unsigned long long *min, > + unsigned long long *max) > +{ > + unsigned long long nr_inos = 0; > + xfs_agnumber_t agno; > + > + /* root, rtbitmap, rtsum all live in the first chunk */ > + *min = XFS_INODES_PER_CHUNK; > + > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > + xfs_agino_t first, last; > + > + xfs_agino_range(mp, agno, &first, &last); > + nr_inos += first - last + 1; > + } > + *max = nr_inos; > +} And the effect of the inode32 mount option on the valid icount range? Cheers, Dave.
On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote: > On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Add a helper predicate to check the inode count for sanity, then use it > > in the superblock write verifier to inspect sb_icount. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/libxfs/xfs_sb.c | 1 + > > fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ > > fs/xfs/libxfs/xfs_types.h | 1 + > > 3 files changed, 36 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > index b2c683588519..1659016875f9 100644 > > --- a/fs/xfs/libxfs/xfs_sb.c > > +++ b/fs/xfs/libxfs/xfs_sb.c > > @@ -714,6 +714,7 @@ xfs_sb_write_verify( > > * cases. > > */ > > if (sb.sb_fdblocks > sb.sb_dblocks || > > + !xfs_verify_icount(mp, sb.sb_icount) || > > sb.sb_ifree > sb.sb_icount) { > > xfs_notice(mp, "SB summary counter sanity check failed"); > > error = -EFSCORRUPTED; > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c > > index 2e2a243cef2e..2e9c0c25ccb6 100644 > > --- a/fs/xfs/libxfs/xfs_types.c > > +++ b/fs/xfs/libxfs/xfs_types.c > > @@ -171,3 +171,37 @@ xfs_verify_rtbno( > > { > > return rtbno < mp->m_sb.sb_rblocks; > > } > > + > > +/* Calculate the range of valid icount values. */ > > +static void > > +xfs_icount_range( > > + struct xfs_mount *mp, > > + unsigned long long *min, > > + unsigned long long *max) > > +{ > > + unsigned long long nr_inos = 0; > > + xfs_agnumber_t agno; > > + > > + /* root, rtbitmap, rtsum all live in the first chunk */ > > + *min = XFS_INODES_PER_CHUNK; > > + > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > + xfs_agino_t first, last; > > + > > + xfs_agino_range(mp, agno, &first, &last); > > + nr_inos += first - last + 1; > > + } > > + *max = nr_inos; > > +} > > And the effect of the inode32 mount option on the valid icount range? Heh, I wondered about that. The premise of inode32 is that we will never allocate an inode with a number exceeding 2^32, correct? Do we ever write anything to that fs to say "this fs must never have inode numbers > 2^32"? i.e. something that permanently restricts it to 32-bit inode numbers and counts? I don't think I see any such device. What's supposed to happen if I create a > 1TB fs, put a bunch of files on it such that some of them end up with inode numbers exceeding 2^32, unmount it, and then mount it again with inode32? Do we detect this and refuse the mount because we can't honor the inode32 constraints? Similarly, what if I create a filesystem with more than 4 billion files on it, then unmount and remount with inode32? Do we actually detect this situation and refuse to mount because we know the counter is already larger than 2^32? If we allow the mount today, should we start failing superblock writes because sb_icount is greater than 2^32? In other words, I'm not sure inode32 can have any effect on the icount *max if we don't refuse the mount if the fs already has 64-bit inodes. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.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 -- 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 Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote: > On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote: > > On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Add a helper predicate to check the inode count for sanity, then use it > > > in the superblock write verifier to inspect sb_icount. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_sb.c | 1 + > > > fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ > > > fs/xfs/libxfs/xfs_types.h | 1 + > > > 3 files changed, 36 insertions(+) > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > index b2c683588519..1659016875f9 100644 > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > @@ -714,6 +714,7 @@ xfs_sb_write_verify( > > > * cases. > > > */ > > > if (sb.sb_fdblocks > sb.sb_dblocks || > > > + !xfs_verify_icount(mp, sb.sb_icount) || > > > sb.sb_ifree > sb.sb_icount) { > > > xfs_notice(mp, "SB summary counter sanity check failed"); > > > error = -EFSCORRUPTED; > > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c > > > index 2e2a243cef2e..2e9c0c25ccb6 100644 > > > --- a/fs/xfs/libxfs/xfs_types.c > > > +++ b/fs/xfs/libxfs/xfs_types.c > > > @@ -171,3 +171,37 @@ xfs_verify_rtbno( > > > { > > > return rtbno < mp->m_sb.sb_rblocks; > > > } > > > + > > > +/* Calculate the range of valid icount values. */ > > > +static void > > > +xfs_icount_range( > > > + struct xfs_mount *mp, > > > + unsigned long long *min, > > > + unsigned long long *max) > > > +{ > > > + unsigned long long nr_inos = 0; > > > + xfs_agnumber_t agno; > > > + > > > + /* root, rtbitmap, rtsum all live in the first chunk */ > > > + *min = XFS_INODES_PER_CHUNK; > > > + > > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > > + xfs_agino_t first, last; > > > + > > > + xfs_agino_range(mp, agno, &first, &last); > > > + nr_inos += first - last + 1; Shouldn't this be last - first? > > > + } > > > + *max = nr_inos; > > > +} > > > > And the effect of the inode32 mount option on the valid icount range? > > Heh, I wondered about that. The premise of inode32 is that we will > never allocate an inode with a number exceeding 2^32, correct? Do we > ever write anything to that fs to say "this fs must never have inode > numbers > 2^32"? i.e. something that permanently restricts it to > 32-bit inode numbers and counts? I don't think I see any such device. > > What's supposed to happen if I create a > 1TB fs, put a bunch of files > on it such that some of them end up with inode numbers exceeding 2^32, > unmount it, and then mount it again with inode32? Do we detect this and > refuse the mount because we can't honor the inode32 constraints? > > Similarly, what if I create a filesystem with more than 4 billion files > on it, then unmount and remount with inode32? Do we actually detect > this situation and refuse to mount because we know the counter is > already larger than 2^32? If we allow the mount today, should we start > failing superblock writes because sb_icount is greater than 2^32? > I thought an inode32 mount should allow reading existing inode64 inodes without an issue. As noted above, it just prevents the allocation of further inodes beyond 1TB. > In other words, I'm not sure inode32 can have any effect on the icount > *max if we don't refuse the mount if the fs already has 64-bit inodes. > This patch looks like it doesn't consider inode32. It just ensures that the icount falls into a valid range based on the ag geometry, which seems broad enough to cover all cases... hm? That aside.. since these values shouldn't change often I'm wondering if it's worth calculating the global min/max once at mount time (we'd have to recalc on growfs) rather than in the sb verifier path... It looks like we already have a bunch of such misc min/max counters in xfs_mount. Brian > --D > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.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 > -- > 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/27/18 3:44 AM, Brian Foster wrote: > On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote: >> On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote: >>> On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: >>>> From: Darrick J. Wong <darrick.wong@oracle.com> >>>> >>>> Add a helper predicate to check the inode count for sanity, then use it >>>> in the superblock write verifier to inspect sb_icount. >>>> >>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> >>>> --- >>>> fs/xfs/libxfs/xfs_sb.c | 1 + >>>> fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ >>>> fs/xfs/libxfs/xfs_types.h | 1 + >>>> 3 files changed, 36 insertions(+) >>>> >>>> >>>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c >>>> index b2c683588519..1659016875f9 100644 >>>> --- a/fs/xfs/libxfs/xfs_sb.c >>>> +++ b/fs/xfs/libxfs/xfs_sb.c >>>> @@ -714,6 +714,7 @@ xfs_sb_write_verify( >>>> * cases. >>>> */ >>>> if (sb.sb_fdblocks > sb.sb_dblocks || >>>> + !xfs_verify_icount(mp, sb.sb_icount) || >>>> sb.sb_ifree > sb.sb_icount) { >>>> xfs_notice(mp, "SB summary counter sanity check failed"); >>>> error = -EFSCORRUPTED; >>>> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c >>>> index 2e2a243cef2e..2e9c0c25ccb6 100644 >>>> --- a/fs/xfs/libxfs/xfs_types.c >>>> +++ b/fs/xfs/libxfs/xfs_types.c >>>> @@ -171,3 +171,37 @@ xfs_verify_rtbno( >>>> { >>>> return rtbno < mp->m_sb.sb_rblocks; >>>> } >>>> + >>>> +/* Calculate the range of valid icount values. */ >>>> +static void >>>> +xfs_icount_range( >>>> + struct xfs_mount *mp, >>>> + unsigned long long *min, >>>> + unsigned long long *max) >>>> +{ >>>> + unsigned long long nr_inos = 0; >>>> + xfs_agnumber_t agno; >>>> + >>>> + /* root, rtbitmap, rtsum all live in the first chunk */ >>>> + *min = XFS_INODES_PER_CHUNK; >>>> + >>>> + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { >>>> + xfs_agino_t first, last; >>>> + >>>> + xfs_agino_range(mp, agno, &first, &last); >>>> + nr_inos += first - last + 1; > > Shouldn't this be last - first? > >>>> + } >>>> + *max = nr_inos; >>>> +} >>> >>> And the effect of the inode32 mount option on the valid icount range? >> >> Heh, I wondered about that. The premise of inode32 is that we will >> never allocate an inode with a number exceeding 2^32, correct? Do we >> ever write anything to that fs to say "this fs must never have inode >> numbers > 2^32"? i.e. something that permanently restricts it to >> 32-bit inode numbers and counts? I don't think I see any such device. >> >> What's supposed to happen if I create a > 1TB fs, put a bunch of files >> on it such that some of them end up with inode numbers exceeding 2^32, >> unmount it, and then mount it again with inode32? Do we detect this and >> refuse the mount because we can't honor the inode32 constraints? >> >> Similarly, what if I create a filesystem with more than 4 billion files >> on it, then unmount and remount with inode32? Do we actually detect >> this situation and refuse to mount because we know the counter is >> already larger than 2^32? If we allow the mount today, should we start >> failing superblock writes because sb_icount is greater than 2^32? >> > > I thought an inode32 mount should allow reading existing inode64 inodes > without an issue. As noted above, it just prevents the allocation of > further inodes beyond 1TB. > >> In other words, I'm not sure inode32 can have any effect on the icount >> *max if we don't refuse the mount if the fs already has 64-bit inodes. >> > > This patch looks like it doesn't consider inode32. It just ensures that > the icount falls into a valid range based on the ag geometry, which > seems broad enough to cover all cases... hm? > > That aside.. since these values shouldn't change often I'm wondering if > it's worth calculating the global min/max once at mount time (we'd have > to recalc on growfs) rather than in the sb verifier path... It looks > like we already have a bunch of such misc min/max counters in xfs_mount. It does seem like a fair bit of work to calculate unchanging values. Since this is really only an order-ofmagnitude sanity check anyway, I wonder if this part of the verifier isn't working too hard to arrive at the best-possible upper bound. My 64-bit divide was dumb & broken, but wouldn't ~(dblocks/inopb) get us close enough with a lot less work? -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 Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote: > On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote: > > On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote: > > > On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > Add a helper predicate to check the inode count for sanity, then use it > > > > in the superblock write verifier to inspect sb_icount. > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > --- > > > > fs/xfs/libxfs/xfs_sb.c | 1 + > > > > fs/xfs/libxfs/xfs_types.c | 34 ++++++++++++++++++++++++++++++++++ > > > > fs/xfs/libxfs/xfs_types.h | 1 + > > > > 3 files changed, 36 insertions(+) > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > > > > index b2c683588519..1659016875f9 100644 > > > > --- a/fs/xfs/libxfs/xfs_sb.c > > > > +++ b/fs/xfs/libxfs/xfs_sb.c > > > > @@ -714,6 +714,7 @@ xfs_sb_write_verify( > > > > * cases. > > > > */ > > > > if (sb.sb_fdblocks > sb.sb_dblocks || > > > > + !xfs_verify_icount(mp, sb.sb_icount) || > > > > sb.sb_ifree > sb.sb_icount) { > > > > xfs_notice(mp, "SB summary counter sanity check failed"); > > > > error = -EFSCORRUPTED; > > > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c > > > > index 2e2a243cef2e..2e9c0c25ccb6 100644 > > > > --- a/fs/xfs/libxfs/xfs_types.c > > > > +++ b/fs/xfs/libxfs/xfs_types.c > > > > @@ -171,3 +171,37 @@ xfs_verify_rtbno( > > > > { > > > > return rtbno < mp->m_sb.sb_rblocks; > > > > } > > > > + > > > > +/* Calculate the range of valid icount values. */ > > > > +static void > > > > +xfs_icount_range( > > > > + struct xfs_mount *mp, > > > > + unsigned long long *min, > > > > + unsigned long long *max) > > > > +{ > > > > + unsigned long long nr_inos = 0; > > > > + xfs_agnumber_t agno; > > > > + > > > > + /* root, rtbitmap, rtsum all live in the first chunk */ > > > > + *min = XFS_INODES_PER_CHUNK; > > > > + > > > > + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { > > > > + xfs_agino_t first, last; > > > > + > > > > + xfs_agino_range(mp, agno, &first, &last); > > > > + nr_inos += first - last + 1; > > Shouldn't this be last - first? Oops, yes, will fix that. > > > > + } > > > > + *max = nr_inos; > > > > +} > > > > > > And the effect of the inode32 mount option on the valid icount range? > > > > Heh, I wondered about that. The premise of inode32 is that we will > > never allocate an inode with a number exceeding 2^32, correct? Do we > > ever write anything to that fs to say "this fs must never have inode > > numbers > 2^32"? i.e. something that permanently restricts it to > > 32-bit inode numbers and counts? I don't think I see any such device. > > > > What's supposed to happen if I create a > 1TB fs, put a bunch of files > > on it such that some of them end up with inode numbers exceeding 2^32, > > unmount it, and then mount it again with inode32? Do we detect this and > > refuse the mount because we can't honor the inode32 constraints? > > > > Similarly, what if I create a filesystem with more than 4 billion files > > on it, then unmount and remount with inode32? Do we actually detect > > this situation and refuse to mount because we know the counter is > > already larger than 2^32? If we allow the mount today, should we start > > failing superblock writes because sb_icount is greater than 2^32? > > > > I thought an inode32 mount should allow reading existing inode64 inodes > without an issue. As noted above, it just prevents the allocation of > further inodes beyond 1TB. > > > In other words, I'm not sure inode32 can have any effect on the icount > > *max if we don't refuse the mount if the fs already has 64-bit inodes. > > > > This patch looks like it doesn't consider inode32. It just ensures that > the icount falls into a valid range based on the ag geometry, which > seems broad enough to cover all cases... hm? Correct. > That aside.. since these values shouldn't change often I'm wondering if > it's worth calculating the global min/max once at mount time (we'd have > to recalc on growfs) rather than in the sb verifier path... It looks > like we already have a bunch of such misc min/max counters in xfs_mount. <shrug> I suppose so, but otoh I doubt this function has a lot of overhead. I'll look into it for the next version. --D > Brian > > > --D > > > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@fromorbit.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 > > -- > > 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
On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote: > That aside.. since these values shouldn't change often I'm wondering if > it's worth calculating the global min/max once at mount time (we'd have The values are needed during the initial read of the superblock before any "for the life of the mount" calculations can be made, and they can change across log recovery, too, if recovery replays a growfs transaction. Hence they really need to be point in time calculations based on the current superblock values during superblock reads.... Cheers, Dave.
On Sun, Jul 29, 2018 at 02:39:00PM +1000, Dave Chinner wrote: > On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote: > > That aside.. since these values shouldn't change often I'm wondering if > > it's worth calculating the global min/max once at mount time (we'd have > > The values are needed during the initial read of the superblock > before any "for the life of the mount" calculations can be made, > and they can change across log recovery, too, if recovery replays a > growfs transaction. Hence they really need to be point in time > calculations based on the current superblock values during > superblock reads.... > I'm not so worried about storing the calculated total as much as simplifying the implementation. All but the last iteration in the current loop calculate a fixed value, so this can trivially be simplified to something like: xfs_agino_range(mp, --agcount, first, last); max = last - first + 1; if (agcount) { xfs_agino_range(mp, agcount - 1, first, last); max += (last - first + 1) * agcount; } ... which (if it mattered) could probably simplify even further if we factored out the additional portion of the calculation that is fixed: xfs_agino_range(mp, --agcount, first, last); max = last - first + 1; if (agcount) max += agcount * mp->m_aginodes; I.e., with an ->m_aginodes field in xfs_mount that is analogous to sb_agblocks. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.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 -- 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_sb.c b/fs/xfs/libxfs/xfs_sb.c index b2c683588519..1659016875f9 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -714,6 +714,7 @@ xfs_sb_write_verify( * cases. */ if (sb.sb_fdblocks > sb.sb_dblocks || + !xfs_verify_icount(mp, sb.sb_icount) || sb.sb_ifree > sb.sb_icount) { xfs_notice(mp, "SB summary counter sanity check failed"); error = -EFSCORRUPTED; diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c index 2e2a243cef2e..2e9c0c25ccb6 100644 --- a/fs/xfs/libxfs/xfs_types.c +++ b/fs/xfs/libxfs/xfs_types.c @@ -171,3 +171,37 @@ xfs_verify_rtbno( { return rtbno < mp->m_sb.sb_rblocks; } + +/* Calculate the range of valid icount values. */ +static void +xfs_icount_range( + struct xfs_mount *mp, + unsigned long long *min, + unsigned long long *max) +{ + unsigned long long nr_inos = 0; + xfs_agnumber_t agno; + + /* root, rtbitmap, rtsum all live in the first chunk */ + *min = XFS_INODES_PER_CHUNK; + + for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) { + xfs_agino_t first, last; + + xfs_agino_range(mp, agno, &first, &last); + nr_inos += first - last + 1; + } + *max = nr_inos; +} + +/* Sanity-checking of inode counts. */ +bool +xfs_verify_icount( + struct xfs_mount *mp, + unsigned long long icount) +{ + unsigned long long min, max; + + xfs_icount_range(mp, &min, &max); + return icount >= min && icount < max; +} diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h index 4055d62f690c..b9e6c89284c3 100644 --- a/fs/xfs/libxfs/xfs_types.h +++ b/fs/xfs/libxfs/xfs_types.h @@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino); bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino); bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino); bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno); +bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount); #endif /* __XFS_TYPES_H__ */