| Message ID | 165644936573.1089996.11135224585697421312.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: random fixes | expand |
On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64 > feature enabled in the superblock. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > repair/dinode.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > > diff --git a/repair/dinode.c b/repair/dinode.c > index 00de31fb..547c5833 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > } > } > > + if (xfs_dinode_has_large_extent_counts(dino) && > + !xfs_has_large_extent_counts(mp)) { > + if (!uncertain) { > + do_warn( > + _("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"), > + lino); > + } > + flags2 &= ~XFS_DIFLAG2_NREXT64; > + > + if (no_modify) { > + do_warn(_("would zero extent counts.\n")); > + } else { > + do_warn(_("zeroing extent counts.\n")); > + dino->di_nextents = 0; > + dino->di_anextents = 0; > + *dirty = 1; Is that necessary? If the existing extent counts are within the bounds of the old extent fields, then shouldn't we just rewrite the current values into the old format rather than trashing all the data/xattrs on the inode? Cheers, Dave.
On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote: > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64 > > feature enabled in the superblock. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > repair/dinode.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/repair/dinode.c b/repair/dinode.c > > index 00de31fb..547c5833 100644 > > --- a/repair/dinode.c > > +++ b/repair/dinode.c > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > > } > > } > > > > + if (xfs_dinode_has_large_extent_counts(dino) && > > + !xfs_has_large_extent_counts(mp)) { > > + if (!uncertain) { > > + do_warn( > > + _("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"), > > + lino); > > + } > > + flags2 &= ~XFS_DIFLAG2_NREXT64; > > + > > + if (no_modify) { > > + do_warn(_("would zero extent counts.\n")); > > + } else { > > + do_warn(_("zeroing extent counts.\n")); > > + dino->di_nextents = 0; > > + dino->di_anextents = 0; > > + *dirty = 1; > > Is that necessary? If the existing extent counts are within the > bounds of the old extent fields, then shouldn't we just rewrite the > current values into the old format rather than trashing all the > data/xattrs on the inode? It's hard to know what to do here -- we haven't actually checked the forks yet, so we don't know if the dinode flag was set but the !nrext64 extent count fields are ok so all we have to do is clear the dinode flag; or if the dinode flag was set, the nrext64 extent count fields are correct and must be moved to the !nrext64 fields; or what? I guess I could just leave the extent count fields as-is and let the process_*_fork functions deal with it. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, Jun 29, 2022 at 04:10:47PM -0700, Darrick J. Wong wrote: > On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote: > > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64 > > > feature enabled in the superblock. > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > repair/dinode.c | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > > > > diff --git a/repair/dinode.c b/repair/dinode.c > > > index 00de31fb..547c5833 100644 > > > --- a/repair/dinode.c > > > +++ b/repair/dinode.c > > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > > > } > > > } > > > > > > + if (xfs_dinode_has_large_extent_counts(dino) && > > > + !xfs_has_large_extent_counts(mp)) { > > > + if (!uncertain) { > > > + do_warn( > > > + _("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"), > > > + lino); > > > + } > > > + flags2 &= ~XFS_DIFLAG2_NREXT64; > > > + > > > + if (no_modify) { > > > + do_warn(_("would zero extent counts.\n")); > > > + } else { > > > + do_warn(_("zeroing extent counts.\n")); > > > + dino->di_nextents = 0; > > > + dino->di_anextents = 0; > > > + *dirty = 1; > > > > Is that necessary? If the existing extent counts are within the > > bounds of the old extent fields, then shouldn't we just rewrite the > > current values into the old format rather than trashing all the > > data/xattrs on the inode? > > It's hard to know what to do here -- we haven't actually checked the > forks yet, so we don't know if the dinode flag was set but the !nrext64 > extent count fields are ok so all we have to do is clear the dinode > flag; or if the dinode flag was set, the nrext64 extent count fields are > correct and must be moved to the !nrext64 fields; or what? > > I guess I could just leave the extent count fields as-is and let the > process_*_fork functions deal with it. Can we check it -after- the inode has otherwise been validated and do the conversion then? Cheers, Dave.
On Fri, Jul 01, 2022 at 08:51:45AM +1000, Dave Chinner wrote: > On Wed, Jun 29, 2022 at 04:10:47PM -0700, Darrick J. Wong wrote: > > On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote: > > > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64 > > > > feature enabled in the superblock. > > > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > --- > > > > repair/dinode.c | 19 +++++++++++++++++++ > > > > 1 file changed, 19 insertions(+) > > > > > > > > > > > > diff --git a/repair/dinode.c b/repair/dinode.c > > > > index 00de31fb..547c5833 100644 > > > > --- a/repair/dinode.c > > > > +++ b/repair/dinode.c > > > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), > > > > } > > > > } > > > > > > > > + if (xfs_dinode_has_large_extent_counts(dino) && > > > > + !xfs_has_large_extent_counts(mp)) { > > > > + if (!uncertain) { > > > > + do_warn( > > > > + _("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"), > > > > + lino); > > > > + } > > > > + flags2 &= ~XFS_DIFLAG2_NREXT64; > > > > + > > > > + if (no_modify) { > > > > + do_warn(_("would zero extent counts.\n")); > > > > + } else { > > > > + do_warn(_("zeroing extent counts.\n")); > > > > + dino->di_nextents = 0; > > > > + dino->di_anextents = 0; > > > > + *dirty = 1; > > > > > > Is that necessary? If the existing extent counts are within the > > > bounds of the old extent fields, then shouldn't we just rewrite the > > > current values into the old format rather than trashing all the > > > data/xattrs on the inode? > > > > It's hard to know what to do here -- we haven't actually checked the > > forks yet, so we don't know if the dinode flag was set but the !nrext64 > > extent count fields are ok so all we have to do is clear the dinode > > flag; or if the dinode flag was set, the nrext64 extent count fields are > > correct and must be moved to the !nrext64 fields; or what? > > > > I guess I could just leave the extent count fields as-is and let the > > process_*_fork functions deal with it. > > Can we check it -after- the inode has otherwise been validated > and do the conversion then? process_inode_{data,attr}_fork compute the correct extent counts and process_inode_blocks_and_extents writes them into the appropriate fields in the xfs_dinode structure, so there's no need to convert anything. This chunk comes before all that, so all we have to do here is set the DIFLAG2 state as desired. Zeroing the extent fields was unnecessary. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/repair/dinode.c b/repair/dinode.c index 00de31fb..547c5833 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), } } + if (xfs_dinode_has_large_extent_counts(dino) && + !xfs_has_large_extent_counts(mp)) { + if (!uncertain) { + do_warn( + _("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"), + lino); + } + flags2 &= ~XFS_DIFLAG2_NREXT64; + + if (no_modify) { + do_warn(_("would zero extent counts.\n")); + } else { + do_warn(_("zeroing extent counts.\n")); + dino->di_nextents = 0; + dino->di_anextents = 0; + *dirty = 1; + } + } + if (!verify_mode && flags2 != be64_to_cpu(dino->di_flags2)) { if (!no_modify) { do_warn(_("fixing bad flags2.\n"));