| Message ID | 160679384513.447856.3675245763779550446.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: add the ability to flag a fs for repair | expand |
On Mon, Nov 30, 2020 at 07:37:25PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > A couple of the superblock validation checks apply only to the kernel, > so move them to xfs_mount.c before we start changing sb_inprogress. > This also reduces the diff between kernel and userspace libxfs. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/libxfs/xfs_sb.c | 23 ++++------------------- > fs/xfs/libxfs/xfs_sb.h | 3 +++ > fs/xfs/xfs_mount.c | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 19 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 5aeafa59ed27..a2c43fe38f64 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -223,6 +223,7 @@ xfs_validate_sb_common( > struct xfs_dsb *dsb = bp->b_addr; > uint32_t agcount = 0; > uint32_t rem; > + int error; > > if (!xfs_verify_magic(bp, dsb->sb_magicnum)) { > xfs_warn(mp, "bad magic number"); > @@ -382,16 +383,9 @@ xfs_validate_sb_common( > return -EFSCORRUPTED; > } > > - /* > - * Until this is fixed only page-sized or smaller data blocks work. > - */ > - if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { > - xfs_warn(mp, > - "File system with blocksize %d bytes. " > - "Only pagesize (%ld) or less will currently work.", > - sbp->sb_blocksize, PAGE_SIZE); > - return -ENOSYS; > - } > + error = xfs_sb_validate_mount(mp, bp, sbp); > + if (error) > + return error; > > /* > * Currently only very few inode sizes are supported. > @@ -415,15 +409,6 @@ xfs_validate_sb_common( > return -EFBIG; > } > > - /* > - * Don't touch the filesystem if a user tool thinks it owns the primary > - * superblock. mkfs doesn't clear the flag from secondary supers, so > - * we don't check them at all. > - */ > - if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { > - xfs_warn(mp, "Offline file system operation in progress!"); > - return -EFSCORRUPTED; > - } > return 0; > } > > diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h > index 92465a9a5162..ee0a5858dd47 100644 > --- a/fs/xfs/libxfs/xfs_sb.h > +++ b/fs/xfs/libxfs/xfs_sb.h > @@ -42,4 +42,7 @@ extern int xfs_sb_get_secondary(struct xfs_mount *mp, > struct xfs_trans *tp, xfs_agnumber_t agno, > struct xfs_buf **bpp); > > +int xfs_sb_validate_mount(struct xfs_mount *mp, struct xfs_buf *bp, > + struct xfs_sb *sbp); > + > #endif /* __XFS_SB_H__ */ > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 7110507a2b6b..7bc7901d648d 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -259,6 +259,37 @@ xfs_initialize_perag( > return error; > } > > +/* Validate the superblock is compatible with this mount. */ > +int > +xfs_sb_validate_mount( > + struct xfs_mount *mp, > + struct xfs_buf *bp, > + struct xfs_sb *sbp) > +{ > + /* > + * Don't touch the filesystem if a user tool thinks it owns the primary > + * superblock. mkfs doesn't clear the flag from secondary supers, so > + * we don't check them at all. > + */ > + if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { > + xfs_warn(mp, "Offline file system operation in progress!"); > + return -EFSCORRUPTED; > + } > + > + /* > + * Until this is fixed only page-sized or smaller data blocks work. > + */ > + if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { > + xfs_warn(mp, > + "File system with blocksize %d bytes. " > + "Only pagesize (%ld) or less will currently work.", > + sbp->sb_blocksize, PAGE_SIZE); > + return -ENOSYS; > + } > + > + return 0; > +} > + > /* > * xfs_readsb > * >
On 11/30/20 9:37 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > A couple of the superblock validation checks apply only to the kernel, > so move them to xfs_mount.c before we start changing sb_inprogress. > This also reduces the diff between kernel and userspace libxfs. My only complaint is that "xfs_sb_validate_mount" isn't really descriptive at all, and nobody reading the code or comments will know why we've chosen to move just these two checks out of the common validator... What does "compatible with this mount" mean? Maybe just fess up in the comment, and say "these checks are different for kernel vs. userspace so we keep them over here" - and as for the function name, *shrug* not sure I have anything better... -Eric
On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote: > On 11/30/20 9:37 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > A couple of the superblock validation checks apply only to the kernel, > > so move them to xfs_mount.c before we start changing sb_inprogress. > > This also reduces the diff between kernel and userspace libxfs. > > My only complaint is that "xfs_sb_validate_mount" isn't really descriptive > at all, and nobody reading the code or comments will know why we've chosen > to move just these two checks out of the common validator... > > What does "compatible with this mount" mean? Compatible with this implementation? > Maybe just fess up in the comment, and say "these checks are different > for kernel vs. userspace so we keep them over here" - and as for the > function name, *shrug* not sure I have anything better... _validate_implementation? I don't have a better suggestion either. --D > -Eric >
On 12/4/20 3:12 PM, Darrick J. Wong wrote: > On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote: >> On 11/30/20 9:37 PM, Darrick J. Wong wrote: >>> From: Darrick J. Wong <darrick.wong@oracle.com> >>> >>> A couple of the superblock validation checks apply only to the kernel, >>> so move them to xfs_mount.c before we start changing sb_inprogress. oh also, you're not changing sb_inprogress anymore, right? ;) >>> This also reduces the diff between kernel and userspace libxfs. >> >> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive >> at all, and nobody reading the code or comments will know why we've chosen >> to move just these two checks out of the common validator... >> >> What does "compatible with this mount" mean? > > Compatible with this implementation? Hm. So most of xfs_validate_sb_common is doing internal consistency checking that has nothing at all to do with the host's core capabilities or filesystem "state" (other than version/features I guess). You've moved out the PAGE_SIZE check, which depends on the host. You've also moved the inprogress check, which depends on state. (and that's not really "kernel-specific" is it?) You'll later move the NEEDSREPAIR check, which I guess is state. But you haven't moved the fsb_count-vs-host check, which depends on the host. (and ... I think that one may actually be kernel-specific, because it depends on pagecache limitations in the kernel, so maybe it should be moved out as well?) So maybe the distinction is internal consistency checks, vs host-compatibility-and-filesystem-state checks. How about ultimately: /* * Do host compatibility and filesystem state checks here; these are unique * to the kernel, and may differ in userspace. */ xfs_validate_sb_host( struct xfs_mount *mp, struct xfs_buf *bp, struct xfs_sb *sbp) { /* * Don't touch the filesystem if a user tool thinks it owns the primary * superblock. mkfs doesn't clear the flag from secondary supers, so * we don't check them at all. */ if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { xfs_warn(mp, "Offline file system operation in progress!"); return -EFSCORRUPTED; } /* Filesystem claims it needs repair, so refuse the mount. */ if (xfs_sb_version_needsrepair(&mp->m_sb)) { xfs_warn(mp, "Filesystem needs repair. Please run xfs_repair."); return -EFSCORRUPTED; } /* * Until this is fixed only page-sized or smaller data blocks work. */ if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { xfs_warn(mp, "File system with blocksize %d bytes. " "Only pagesize (%ld) or less will currently work.", sbp->sb_blocksize, PAGE_SIZE); return -ENOSYS; } /* Ensure this filesystem fits in the page cache limits */ if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) || xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) { xfs_warn(mp, "file system too large to be mounted on this system."); return -EFBIG; } return 0; } >> Maybe just fess up in the comment, and say "these checks are different >> for kernel vs. userspace so we keep them over here" - and as for the >> function name, *shrug* not sure I have anything better... > > _validate_implementation? I don't have a better suggestion either. > > --D > >> -Eric >> >
On Fri, Dec 04, 2020 at 03:46:19PM -0600, Eric Sandeen wrote: > On 12/4/20 3:12 PM, Darrick J. Wong wrote: > > On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote: > >> On 11/30/20 9:37 PM, Darrick J. Wong wrote: > >>> From: Darrick J. Wong <darrick.wong@oracle.com> > >>> > >>> A couple of the superblock validation checks apply only to the kernel, > >>> so move them to xfs_mount.c before we start changing sb_inprogress. > > oh also, you're not changing sb_inprogress anymore, right? ;) Fixed. > >>> This also reduces the diff between kernel and userspace libxfs. > >> > >> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive > >> at all, and nobody reading the code or comments will know why we've chosen > >> to move just these two checks out of the common validator... > >> > >> What does "compatible with this mount" mean? > > > > Compatible with this implementation? > > Hm. > > So most of xfs_validate_sb_common is doing internal consistency checking > that has nothing at all to do with the host's core capabilities or filesystem > "state" (other than version/features I guess). > > You've moved out the PAGE_SIZE check, which depends on the host. > > You've also moved the inprogress check, which depends on state. > (and that's not really "kernel-specific" is it?) > > You'll later move the NEEDSREPAIR check, which I guess is state. > > But you haven't moved the fsb_count-vs-host check, which depends on the host. > > (and ... I think that one may actually be kernel-specific, > because it depends on pagecache limitations in the kernel, so maybe it > should be moved out as well?) Aha, yes, I missed that. > So maybe the distinction is internal consistency checks, vs > host-compatibility-and-filesystem-state checks. > > How about ultimately: > > /* > * Do host compatibility and filesystem state checks here; these are unique > * to the kernel, and may differ in userspace. > */ > xfs_validate_sb_host( > struct xfs_mount *mp, > struct xfs_buf *bp, > struct xfs_sb *sbp) > { > /* > * Don't touch the filesystem if a user tool thinks it owns the primary > * superblock. mkfs doesn't clear the flag from secondary supers, so > * we don't check them at all. > */ > if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { > xfs_warn(mp, "Offline file system operation in progress!"); > return -EFSCORRUPTED; > } > > /* Filesystem claims it needs repair, so refuse the mount. */ > if (xfs_sb_version_needsrepair(&mp->m_sb)) { > xfs_warn(mp, "Filesystem needs repair. Please run xfs_repair."); > return -EFSCORRUPTED; > } > > /* > * Until this is fixed only page-sized or smaller data blocks work. > */ > if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { > xfs_warn(mp, > "File system with blocksize %d bytes. " > "Only pagesize (%ld) or less will currently work.", > sbp->sb_blocksize, PAGE_SIZE); > return -ENOSYS; > } > > /* Ensure this filesystem fits in the page cache limits */ > if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) || > xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) { > xfs_warn(mp, > "file system too large to be mounted on this system."); > return -EFBIG; Sounds good to me. --D > } > > return 0; > } > > >> Maybe just fess up in the comment, and say "these checks are different > >> for kernel vs. userspace so we keep them over here" - and as for the > >> function name, *shrug* not sure I have anything better... > > > > _validate_implementation? I don't have a better suggestion either. > > > > --D > > > >> -Eric > >> > >
On Fri, Dec 04, 2020 at 03:46:19PM -0600, Eric Sandeen wrote: > On 12/4/20 3:12 PM, Darrick J. Wong wrote: > > On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote: > >> On 11/30/20 9:37 PM, Darrick J. Wong wrote: > >>> From: Darrick J. Wong <darrick.wong@oracle.com> > >>> > >>> A couple of the superblock validation checks apply only to the kernel, > >>> so move them to xfs_mount.c before we start changing sb_inprogress. > > oh also, you're not changing sb_inprogress anymore, right? ;) > > >>> This also reduces the diff between kernel and userspace libxfs. > >> > >> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive > >> at all, and nobody reading the code or comments will know why we've chosen > >> to move just these two checks out of the common validator... > >> > >> What does "compatible with this mount" mean? > > > > Compatible with this implementation? > > Hm. > > So most of xfs_validate_sb_common is doing internal consistency checking > that has nothing at all to do with the host's core capabilities or filesystem > "state" (other than version/features I guess). > > You've moved out the PAGE_SIZE check, which depends on the host. > > You've also moved the inprogress check, which depends on state. > (and that's not really "kernel-specific" is it?) > > You'll later move the NEEDSREPAIR check, which I guess is state. > > But you haven't moved the fsb_count-vs-host check, which depends on the host. > > (and ... I think that one may actually be kernel-specific, > because it depends on pagecache limitations in the kernel, so maybe it > should be moved out as well?) > > So maybe the distinction is internal consistency checks, vs > host-compatibility-and-filesystem-state checks. > > How about ultimately: > > /* > * Do host compatibility and filesystem state checks here; these are unique > * to the kernel, and may differ in userspace. > */ > xfs_validate_sb_host( > struct xfs_mount *mp, > struct xfs_buf *bp, > struct xfs_sb *sbp) > { THis host stuff should be checked in xfs_fs_fill_super(), right? i.e. it's not really part of the superblock validation, but checking if the host can actually run this filesystem. That's what we do in xfs_fc_fill_super(), such as the max file size support, whether mount options are valid for the superblock config (e.g. reflink vs rt) and so on. IOWs, these aren't corruption verifiers, just config checks, so we should put them with all the other config checks we do at mount time... i.e. call it something like xfs_fs_validate_sb_config() and move all the config and experimental function checks into it, call it only from xfs_fs_fill_super() once we've already read in and validated the superblock is within the bounds defined by the on-disk format. Oh, and just another thing: can we rename all the "xfs_fc_*" functions in xfs_super.c back to xfs_fs_*? I'm getting tired of not being about to find the superblock init functions in xfs_super.c (e.g. xfs_fs_fill_super()) with grep or cscope because they have a whacky, one-off namespace now... Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index 5aeafa59ed27..a2c43fe38f64 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -223,6 +223,7 @@ xfs_validate_sb_common( struct xfs_dsb *dsb = bp->b_addr; uint32_t agcount = 0; uint32_t rem; + int error; if (!xfs_verify_magic(bp, dsb->sb_magicnum)) { xfs_warn(mp, "bad magic number"); @@ -382,16 +383,9 @@ xfs_validate_sb_common( return -EFSCORRUPTED; } - /* - * Until this is fixed only page-sized or smaller data blocks work. - */ - if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { - xfs_warn(mp, - "File system with blocksize %d bytes. " - "Only pagesize (%ld) or less will currently work.", - sbp->sb_blocksize, PAGE_SIZE); - return -ENOSYS; - } + error = xfs_sb_validate_mount(mp, bp, sbp); + if (error) + return error; /* * Currently only very few inode sizes are supported. @@ -415,15 +409,6 @@ xfs_validate_sb_common( return -EFBIG; } - /* - * Don't touch the filesystem if a user tool thinks it owns the primary - * superblock. mkfs doesn't clear the flag from secondary supers, so - * we don't check them at all. - */ - if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { - xfs_warn(mp, "Offline file system operation in progress!"); - return -EFSCORRUPTED; - } return 0; } diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h index 92465a9a5162..ee0a5858dd47 100644 --- a/fs/xfs/libxfs/xfs_sb.h +++ b/fs/xfs/libxfs/xfs_sb.h @@ -42,4 +42,7 @@ extern int xfs_sb_get_secondary(struct xfs_mount *mp, struct xfs_trans *tp, xfs_agnumber_t agno, struct xfs_buf **bpp); +int xfs_sb_validate_mount(struct xfs_mount *mp, struct xfs_buf *bp, + struct xfs_sb *sbp); + #endif /* __XFS_SB_H__ */ diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 7110507a2b6b..7bc7901d648d 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -259,6 +259,37 @@ xfs_initialize_perag( return error; } +/* Validate the superblock is compatible with this mount. */ +int +xfs_sb_validate_mount( + struct xfs_mount *mp, + struct xfs_buf *bp, + struct xfs_sb *sbp) +{ + /* + * Don't touch the filesystem if a user tool thinks it owns the primary + * superblock. mkfs doesn't clear the flag from secondary supers, so + * we don't check them at all. + */ + if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) { + xfs_warn(mp, "Offline file system operation in progress!"); + return -EFSCORRUPTED; + } + + /* + * Until this is fixed only page-sized or smaller data blocks work. + */ + if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) { + xfs_warn(mp, + "File system with blocksize %d bytes. " + "Only pagesize (%ld) or less will currently work.", + sbp->sb_blocksize, PAGE_SIZE); + return -ENOSYS; + } + + return 0; +} + /* * xfs_readsb *