* [PATCH v2 0/3] xfs: add the ability to flag a fs for repair @ 2020-12-06 23:09 Darrick J. Wong 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Darrick J. Wong @ 2020-12-06 23:09 UTC (permalink / raw) To: darrick.wong, sandeen, bfoster, david; +Cc: Eric Sandeen, linux-xfs Hi all, This "new feature" adds a new incompat feature flag so that we can force a sysadmin to run xfs_repair on a filesystem before mounting. The intent for this code is to make it so that one can use xfs_db to upgrade a filesystem to support new V5 features (e.g. y2038 or inode btree counters). Because some upgrades may require xfs_repair to fix or add things before the filesystem goes back into use, this is the means for xfs_db to force that to happen. Note: xfs_admin will automatically run repair when required, so sysadmins won't have to issue the repair command directly. v2: move all the kernel-specific checks to xfs_fs_fill_super since that's where we put the rest of them already If you're going to start using this mess, you probably ought to just pull from my git trees, which are linked below. This is an extraordinary way to destroy everything. Enjoy! Comments and questions are, as always, welcome. --D kernel git tree: https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=needsrepair-5.11 --- fs/xfs/libxfs/xfs_format.h | 10 +++++++++- fs/xfs/libxfs/xfs_sb.c | 27 --------------------------- fs/xfs/xfs_super.c | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 28 deletions(-) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-06 23:09 [PATCH v2 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong @ 2020-12-06 23:09 ` Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner ` (3 more replies) 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong 2020-12-06 23:09 ` [PATCH 3/3] xfs: enable the needsrepair feature Darrick J. Wong 2 siblings, 4 replies; 21+ messages in thread From: Darrick J. Wong @ 2020-12-06 23:09 UTC (permalink / raw) To: darrick.wong, sandeen, bfoster, david; +Cc: linux-xfs 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_fc_fill_super before we add the needsrepair "feature", which will prevent the kernel (but not xfsprogs) from mounting the filesystem. This also reduces the diff between kernel and userspace libxfs. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/libxfs/xfs_sb.c | 27 --------------------------- fs/xfs/xfs_super.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index 5aeafa59ed27..05359690aaed 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -382,17 +382,6 @@ 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; - } - /* * Currently only very few inode sizes are supported. */ @@ -408,22 +397,6 @@ xfs_validate_sb_common( return -ENOSYS; } - 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; - } - - /* - * 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/xfs_super.c b/fs/xfs/xfs_super.c index e3e229e52512..599566c1a3b4 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1467,6 +1467,38 @@ xfs_fc_fill_super( #endif } + /* + * 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 (mp->m_sb.sb_inprogress) { + xfs_warn(mp, "Offline file system operation in progress!"); + error = -EFSCORRUPTED; + goto out_free_sb; + } + + /* + * Until this is fixed only page-sized or smaller data blocks work. + */ + if (mp->m_sb.sb_blocksize > PAGE_SIZE) { + xfs_warn(mp, + "File system with blocksize %d bytes. " + "Only pagesize (%ld) or less will currently work.", + mp->m_sb.sb_blocksize, PAGE_SIZE); + error = -ENOSYS; + goto out_free_sb; + } + + /* Ensure this filesystem fits in the page cache limits */ + if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) || + xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) { + xfs_warn(mp, + "file system too large to be mounted on this system."); + error = -EFBIG; + goto out_free_sb; + } + /* * XFS block mappings use 54 bits to store the logical block offset. * This should suffice to handle the maximum file size that the VFS ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong @ 2020-12-06 23:47 ` Dave Chinner 2020-12-07 17:17 ` Brian Foster ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Dave Chinner @ 2020-12-06 23:47 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, bfoster, linux-xfs On Sun, Dec 06, 2020 at 03:09:26PM -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_fc_fill_super before we add the needsrepair "feature", > which will prevent the kernel (but not xfsprogs) from mounting the > filesystem. This also reduces the diff between kernel and userspace > libxfs. looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner @ 2020-12-07 17:17 ` Brian Foster 2020-12-09 17:08 ` Eric Sandeen 2020-12-09 18:03 ` Christoph Hellwig 3 siblings, 0 replies; 21+ messages in thread From: Brian Foster @ 2020-12-07 17:17 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, david, linux-xfs On Sun, Dec 06, 2020 at 03:09:26PM -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_fc_fill_super before we add the needsrepair "feature", > which will prevent the kernel (but not xfsprogs) from mounting the > filesystem. 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 | 27 --------------------------- > fs/xfs/xfs_super.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+), 27 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 5aeafa59ed27..05359690aaed 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -382,17 +382,6 @@ 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; > - } > - > /* > * Currently only very few inode sizes are supported. > */ > @@ -408,22 +397,6 @@ xfs_validate_sb_common( > return -ENOSYS; > } > > - 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; > - } > - > - /* > - * 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/xfs_super.c b/fs/xfs/xfs_super.c > index e3e229e52512..599566c1a3b4 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1467,6 +1467,38 @@ xfs_fc_fill_super( > #endif > } > > + /* > + * 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 (mp->m_sb.sb_inprogress) { > + xfs_warn(mp, "Offline file system operation in progress!"); > + error = -EFSCORRUPTED; > + goto out_free_sb; > + } > + > + /* > + * Until this is fixed only page-sized or smaller data blocks work. > + */ > + if (mp->m_sb.sb_blocksize > PAGE_SIZE) { > + xfs_warn(mp, > + "File system with blocksize %d bytes. " > + "Only pagesize (%ld) or less will currently work.", > + mp->m_sb.sb_blocksize, PAGE_SIZE); > + error = -ENOSYS; > + goto out_free_sb; > + } > + > + /* Ensure this filesystem fits in the page cache limits */ > + if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) || > + xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) { > + xfs_warn(mp, > + "file system too large to be mounted on this system."); > + error = -EFBIG; > + goto out_free_sb; > + } > + > /* > * XFS block mappings use 54 bits to store the logical block offset. > * This should suffice to handle the maximum file size that the VFS > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner 2020-12-07 17:17 ` Brian Foster @ 2020-12-09 17:08 ` Eric Sandeen 2020-12-09 18:03 ` Christoph Hellwig 3 siblings, 0 replies; 21+ messages in thread From: Eric Sandeen @ 2020-12-09 17:08 UTC (permalink / raw) To: Darrick J. Wong, bfoster, david; +Cc: linux-xfs On 12/6/20 5:09 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_fc_fill_super before we add the needsrepair "feature", > which will prevent the kernel (but not xfsprogs) from mounting the > filesystem. This also reduces the diff between kernel and userspace > libxfs. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Nice, I like this version :) Reviewed-by: Eric Sandeen <sandeen@redhat.com> > fs/xfs/libxfs/xfs_sb.c | 27 --------------------------- > fs/xfs/xfs_super.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+), 27 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 5aeafa59ed27..05359690aaed 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -382,17 +382,6 @@ 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; > - } > - > /* > * Currently only very few inode sizes are supported. > */ > @@ -408,22 +397,6 @@ xfs_validate_sb_common( > return -ENOSYS; > } > > - 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; > - } > - > - /* > - * 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/xfs_super.c b/fs/xfs/xfs_super.c > index e3e229e52512..599566c1a3b4 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1467,6 +1467,38 @@ xfs_fc_fill_super( > #endif > } > > + /* > + * 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 (mp->m_sb.sb_inprogress) { > + xfs_warn(mp, "Offline file system operation in progress!"); > + error = -EFSCORRUPTED; > + goto out_free_sb; > + } > + > + /* > + * Until this is fixed only page-sized or smaller data blocks work. > + */ > + if (mp->m_sb.sb_blocksize > PAGE_SIZE) { > + xfs_warn(mp, > + "File system with blocksize %d bytes. " > + "Only pagesize (%ld) or less will currently work.", > + mp->m_sb.sb_blocksize, PAGE_SIZE); > + error = -ENOSYS; > + goto out_free_sb; > + } > + > + /* Ensure this filesystem fits in the page cache limits */ > + if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) || > + xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) { > + xfs_warn(mp, > + "file system too large to be mounted on this system."); > + error = -EFBIG; > + goto out_free_sb; > + } > + > /* > * XFS block mappings use 54 bits to store the logical block offset. > * This should suffice to handle the maximum file size that the VFS > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong ` (2 preceding siblings ...) 2020-12-09 17:08 ` Eric Sandeen @ 2020-12-09 18:03 ` Christoph Hellwig 3 siblings, 0 replies; 21+ messages in thread From: Christoph Hellwig @ 2020-12-09 18:03 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, bfoster, david, linux-xfs On Sun, Dec 06, 2020 at 03:09:26PM -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_fc_fill_super before we add the needsrepair "feature", > which will prevent the kernel (but not xfsprogs) from mounting the > filesystem. This also reduces the diff between kernel and userspace > libxfs. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-06 23:09 [PATCH v2 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong @ 2020-12-06 23:09 ` Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner ` (2 more replies) 2020-12-06 23:09 ` [PATCH 3/3] xfs: enable the needsrepair feature Darrick J. Wong 2 siblings, 3 replies; 21+ messages in thread From: Darrick J. Wong @ 2020-12-06 23:09 UTC (permalink / raw) To: darrick.wong, sandeen, bfoster, david; +Cc: linux-xfs From: Darrick J. Wong <darrick.wong@oracle.com> Define an incompat feature flag to indicate that the filesystem needs to be repaired. While libxfs will recognize this feature, the kernel will refuse to mount if the feature flag is set, and only xfs_repair will be able to clear the flag. The goal here is to force the admin to run xfs_repair to completion after upgrading the filesystem, or if we otherwise detect anomalies. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/libxfs/xfs_format.h | 7 +++++++ fs/xfs/xfs_super.c | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index dd764da08f6f..5d8ba609ac0b 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -468,6 +468,7 @@ xfs_sb_has_ro_compat_feature( #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ #define XFS_SB_FEAT_INCOMPAT_BIGTIME (1 << 3) /* large timestamps */ +#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4) /* needs xfs_repair */ #define XFS_SB_FEAT_INCOMPAT_ALL \ (XFS_SB_FEAT_INCOMPAT_FTYPE| \ XFS_SB_FEAT_INCOMPAT_SPINODES| \ @@ -584,6 +585,12 @@ static inline bool xfs_sb_version_hasinobtcounts(struct xfs_sb *sbp) (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_INOBTCNT); } +static inline bool xfs_sb_version_needsrepair(struct xfs_sb *sbp) +{ + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 && + (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR); +} + /* * end of superblock version macros */ diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 599566c1a3b4..36002f460d7c 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1467,6 +1467,13 @@ xfs_fc_fill_super( #endif } + /* 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."); + error = -EFSCORRUPTED; + goto out_free_sb; + } + /* * 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong @ 2020-12-06 23:47 ` Dave Chinner 2020-12-09 17:15 ` Eric Sandeen 2020-12-09 18:04 ` Christoph Hellwig 2 siblings, 0 replies; 21+ messages in thread From: Dave Chinner @ 2020-12-06 23:47 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, bfoster, linux-xfs On Sun, Dec 06, 2020 at 03:09:33PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Define an incompat feature flag to indicate that the filesystem needs to > be repaired. While libxfs will recognize this feature, the kernel will > refuse to mount if the feature flag is set, and only xfs_repair will be > able to clear the flag. The goal here is to force the admin to run > xfs_repair to completion after upgrading the filesystem, or if we > otherwise detect anomalies. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> lgtm Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner @ 2020-12-09 17:15 ` Eric Sandeen 2020-12-09 18:04 ` Christoph Hellwig 2 siblings, 0 replies; 21+ messages in thread From: Eric Sandeen @ 2020-12-09 17:15 UTC (permalink / raw) To: Darrick J. Wong, bfoster, david; +Cc: linux-xfs On 12/6/20 5:09 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Define an incompat feature flag to indicate that the filesystem needs to > be repaired. While libxfs will recognize this feature, the kernel will > refuse to mount if the feature flag is set, and only xfs_repair will be > able to clear the flag. The goal here is to force the admin to run > xfs_repair to completion after upgrading the filesystem, or if we > otherwise detect anomalies. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_format.h | 7 +++++++ > fs/xfs/xfs_super.c | 7 +++++++ > 2 files changed, 14 insertions(+) I'm curious, will this ever be used to make a filesystem unmountable if a verifier or scrub detects a problem? That might be some serious scope-creep; if not, I wonder if intent here should be in the commit log or in a comment. "if we otherwise detect anomalies" seems to leave that open. It doesn't change what we're doing here, I just wonder if we should indicate the current intent a bit more clearly to the code-and-commit reader. That said, for the change itself, Reviewed-by: Eric Sandeen <sandeen@redhat.com> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index dd764da08f6f..5d8ba609ac0b 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -468,6 +468,7 @@ xfs_sb_has_ro_compat_feature( > #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */ > #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */ > #define XFS_SB_FEAT_INCOMPAT_BIGTIME (1 << 3) /* large timestamps */ > +#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4) /* needs xfs_repair */ > #define XFS_SB_FEAT_INCOMPAT_ALL \ > (XFS_SB_FEAT_INCOMPAT_FTYPE| \ > XFS_SB_FEAT_INCOMPAT_SPINODES| \ > @@ -584,6 +585,12 @@ static inline bool xfs_sb_version_hasinobtcounts(struct xfs_sb *sbp) > (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_INOBTCNT); > } > > +static inline bool xfs_sb_version_needsrepair(struct xfs_sb *sbp) > +{ > + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 && > + (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR); > +} > + > /* > * end of superblock version macros > */ > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 599566c1a3b4..36002f460d7c 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1467,6 +1467,13 @@ xfs_fc_fill_super( > #endif > } > > + /* 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."); > + error = -EFSCORRUPTED; > + goto out_free_sb; > + } > + > /* > * 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner 2020-12-09 17:15 ` Eric Sandeen @ 2020-12-09 18:04 ` Christoph Hellwig 2020-12-09 18:10 ` Darrick J. Wong 2 siblings, 1 reply; 21+ messages in thread From: Christoph Hellwig @ 2020-12-09 18:04 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, bfoster, david, linux-xfs Who is going to set this flag? If the kernel ever sets it that is a good way to totally brick systems if it happens on the root file system. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-09 18:04 ` Christoph Hellwig @ 2020-12-09 18:10 ` Darrick J. Wong 2020-12-09 18:12 ` Christoph Hellwig 0 siblings, 1 reply; 21+ messages in thread From: Darrick J. Wong @ 2020-12-09 18:10 UTC (permalink / raw) To: Christoph Hellwig; +Cc: sandeen, bfoster, david, linux-xfs On Wed, Dec 09, 2020 at 06:04:00PM +0000, Christoph Hellwig wrote: > Who is going to set this flag? If the kernel ever sets it that is > a good way to totally brick systems if it happens on the root file > system. So far the only user is xfs_db, when xfs_admin tells it to upgrade a v5 filesystem and we want/need to force the fs through xfs_repair: https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mad4ee9c757051692f33a993a348f4e4e61ac098b https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mb6ef416f9626d87610625e069f516945783a5c13 I don't think there's ever going to be a use case for the kernel setting the feature flag on a mounted fs -- if some error is fixable we should just have online repair fix it; or if it's truly catastrophic we don't want to write the disk at all. --D ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] xfs: define a new "needrepair" feature 2020-12-09 18:10 ` Darrick J. Wong @ 2020-12-09 18:12 ` Christoph Hellwig 0 siblings, 0 replies; 21+ messages in thread From: Christoph Hellwig @ 2020-12-09 18:12 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Christoph Hellwig, sandeen, bfoster, david, linux-xfs On Wed, Dec 09, 2020 at 10:10:56AM -0800, Darrick J. Wong wrote: > On Wed, Dec 09, 2020 at 06:04:00PM +0000, Christoph Hellwig wrote: > > Who is going to set this flag? If the kernel ever sets it that is > > a good way to totally brick systems if it happens on the root file > > system. > > So far the only user is xfs_db, when xfs_admin tells it to upgrade a v5 > filesystem and we want/need to force the fs through xfs_repair: > > https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mad4ee9c757051692f33a993a348f4e4e61ac098b > https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mb6ef416f9626d87610625e069f516945783a5c13 > > I don't think there's ever going to be a use case for the kernel setting > the feature flag on a mounted fs -- if some error is fixable we should > just have online repair fix it; or if it's truly catastrophic we don't > want to write the disk at all. Maybe the definition wants a comment to explain this? ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] xfs: enable the needsrepair feature 2020-12-06 23:09 [PATCH v2 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong @ 2020-12-06 23:09 ` Darrick J. Wong 2020-12-06 23:48 ` Dave Chinner 2 siblings, 1 reply; 21+ messages in thread From: Darrick J. Wong @ 2020-12-06 23:09 UTC (permalink / raw) To: darrick.wong, sandeen, bfoster, david; +Cc: Eric Sandeen, linux-xfs From: Darrick J. Wong <darrick.wong@oracle.com> Make it so that libxfs recognizes the needsrepair feature. Note that the kernel will still refuse to mount these. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> --- fs/xfs/libxfs/xfs_format.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 5d8ba609ac0b..f64eed3ccfed 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -473,7 +473,8 @@ xfs_sb_has_ro_compat_feature( (XFS_SB_FEAT_INCOMPAT_FTYPE| \ XFS_SB_FEAT_INCOMPAT_SPINODES| \ XFS_SB_FEAT_INCOMPAT_META_UUID| \ - XFS_SB_FEAT_INCOMPAT_BIGTIME) + XFS_SB_FEAT_INCOMPAT_BIGTIME| \ + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) #define XFS_SB_FEAT_INCOMPAT_UNKNOWN ~XFS_SB_FEAT_INCOMPAT_ALL static inline bool ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] xfs: enable the needsrepair feature 2020-12-06 23:09 ` [PATCH 3/3] xfs: enable the needsrepair feature Darrick J. Wong @ 2020-12-06 23:48 ` Dave Chinner 0 siblings, 0 replies; 21+ messages in thread From: Dave Chinner @ 2020-12-06 23:48 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, bfoster, Eric Sandeen, linux-xfs On Sun, Dec 06, 2020 at 03:09:39PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Make it so that libxfs recognizes the needsrepair feature. Note that > the kernel will still refuse to mount these. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > Reviewed-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/xfs/libxfs/xfs_format.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 5d8ba609ac0b..f64eed3ccfed 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -473,7 +473,8 @@ xfs_sb_has_ro_compat_feature( > (XFS_SB_FEAT_INCOMPAT_FTYPE| \ > XFS_SB_FEAT_INCOMPAT_SPINODES| \ > XFS_SB_FEAT_INCOMPAT_META_UUID| \ > - XFS_SB_FEAT_INCOMPAT_BIGTIME) > + XFS_SB_FEAT_INCOMPAT_BIGTIME| \ > + XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) > > #define XFS_SB_FEAT_INCOMPAT_UNKNOWN ~XFS_SB_FEAT_INCOMPAT_ALL > static inline bool Looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/3] xfs: add the ability to flag a fs for repair @ 2020-12-01 3:37 Darrick J. Wong 2020-12-01 3:37 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 0 siblings, 1 reply; 21+ messages in thread From: Darrick J. Wong @ 2020-12-01 3:37 UTC (permalink / raw) To: darrick.wong, sandeen; +Cc: linux-xfs Hi all, This "new feature" adds a new incompat feature flag so that we can force a sysadmin to run xfs_repair on a filesystem before mounting. The intent for this code is to make it so that one can use xfs_db to upgrade a filesystem to support new V5 features (e.g. y2038 or inode btree counters). Because some upgrades may require xfs_repair to fix or add things before the filesystem goes back into use, this is the means for xfs_db to force that to happen. Note: xfs_admin will automatically run repair when required, so sysadmins won't have to issue the repair command directly. If you're going to start using this mess, you probably ought to just pull from my git trees, which are linked below. This is an extraordinary way to destroy everything. Enjoy! Comments and questions are, as always, welcome. --D kernel git tree: https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=needsrepair-5.11 --- fs/xfs/libxfs/xfs_format.h | 10 +++++++++- fs/xfs/libxfs/xfs_sb.c | 23 ++++------------------- fs/xfs/libxfs/xfs_sb.h | 3 +++ fs/xfs/xfs_mount.c | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 20 deletions(-) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-01 3:37 [PATCH 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong @ 2020-12-01 3:37 ` Darrick J. Wong 2020-12-01 16:17 ` Brian Foster 2020-12-04 20:35 ` Eric Sandeen 0 siblings, 2 replies; 21+ messages in thread From: Darrick J. Wong @ 2020-12-01 3:37 UTC (permalink / raw) To: darrick.wong, sandeen; +Cc: linux-xfs 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> --- 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 * ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-01 3:37 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong @ 2020-12-01 16:17 ` Brian Foster 2020-12-04 20:35 ` Eric Sandeen 1 sibling, 0 replies; 21+ messages in thread From: Brian Foster @ 2020-12-01 16:17 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, linux-xfs 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 > * > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-01 3:37 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-01 16:17 ` Brian Foster @ 2020-12-04 20:35 ` Eric Sandeen 2020-12-04 21:12 ` Darrick J. Wong 1 sibling, 1 reply; 21+ messages in thread From: Eric Sandeen @ 2020-12-04 20:35 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-04 20:35 ` Eric Sandeen @ 2020-12-04 21:12 ` Darrick J. Wong 2020-12-04 21:46 ` Eric Sandeen 0 siblings, 1 reply; 21+ messages in thread From: Darrick J. Wong @ 2020-12-04 21:12 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-04 21:12 ` Darrick J. Wong @ 2020-12-04 21:46 ` Eric Sandeen 2020-12-04 23:02 ` Darrick J. Wong 2020-12-04 23:29 ` Dave Chinner 0 siblings, 2 replies; 21+ messages in thread From: Eric Sandeen @ 2020-12-04 21:46 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-04 21:46 ` Eric Sandeen @ 2020-12-04 23:02 ` Darrick J. Wong 2020-12-04 23:29 ` Dave Chinner 1 sibling, 0 replies; 21+ messages in thread From: Darrick J. Wong @ 2020-12-04 23:02 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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 > >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs 2020-12-04 21:46 ` Eric Sandeen 2020-12-04 23:02 ` Darrick J. Wong @ 2020-12-04 23:29 ` Dave Chinner 1 sibling, 0 replies; 21+ messages in thread From: Dave Chinner @ 2020-12-04 23:29 UTC (permalink / raw) To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-12-09 18:13 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-06 23:09 [PATCH v2 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong 2020-12-06 23:09 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner 2020-12-07 17:17 ` Brian Foster 2020-12-09 17:08 ` Eric Sandeen 2020-12-09 18:03 ` Christoph Hellwig 2020-12-06 23:09 ` [PATCH 2/3] xfs: define a new "needrepair" feature Darrick J. Wong 2020-12-06 23:47 ` Dave Chinner 2020-12-09 17:15 ` Eric Sandeen 2020-12-09 18:04 ` Christoph Hellwig 2020-12-09 18:10 ` Darrick J. Wong 2020-12-09 18:12 ` Christoph Hellwig 2020-12-06 23:09 ` [PATCH 3/3] xfs: enable the needsrepair feature Darrick J. Wong 2020-12-06 23:48 ` Dave Chinner -- strict thread matches above, loose matches on Subject: below -- 2020-12-01 3:37 [PATCH 0/3] xfs: add the ability to flag a fs for repair Darrick J. Wong 2020-12-01 3:37 ` [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs Darrick J. Wong 2020-12-01 16:17 ` Brian Foster 2020-12-04 20:35 ` Eric Sandeen 2020-12-04 21:12 ` Darrick J. Wong 2020-12-04 21:46 ` Eric Sandeen 2020-12-04 23:02 ` Darrick J. Wong 2020-12-04 23:29 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).