On Fri, Nov 10, 2023 at 01:03:49PM +1100, Dave Chinner wrote:
> On Fri, Nov 10, 2023 at 09:36:51AM +0800, Zorro Lang wrote:
> > The g/047 still fails with this 2nd patch. So I did below steps [1],
> > and get the trace output as [2], those dump_inodes() messages you
> > added have been printed, please check.
>
> And that points me at the bug.
>
> dump_inodes: disk ino 0x83: init nblocks 0x8 nextents 0x0/0x0 anextents 0x0/0x0 v3_pad 0x0 nrext64_pad 0x0 di_flags2 0x18
> dump_inodes: log ino 0x83: init nblocks 0x8 nextents 0x0/0x1 anextents 0x0/0x0 v3_pad 0x1 nrext64_pad 0x0 di_flags2 0x18 big
> ^^^^^^^
> The initial log inode is correct.
>
> dump_inodes: disk ino 0x83: pre nblocks 0x8 nextents 0x0/0x0 anextents 0x0/0x0 v3_pad 0x0 nrext64_pad 0x0 di_flags2 0x18
> dump_inodes: log ino 0x83: pre nblocks 0x8 nextents 0x0/0x0 anextents 0x0/0x0 v3_pad 0x0 nrext64_pad 0x0 di_flags2 0x18 big
> ^^^^^^^
>
> .... but on the second sample, it's been modified and the extent
> count has been zeroed? Huh, that is unexpected - what did that?
>
> Oh.
>
> Can you test the patch below and see if it fixes the issue? Keep
> the first verifier patch I sent, then apply the patch below. You can
> drop the debug traceprintk patch - the patch below should fix it.
>
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> xfs: recovery should not clear di_flushiter unconditionally
>
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Because on v3 inodes, di_flushiter doesn't exist. It overlaps with
> zero padding in the inode, except when NREXT64=1 configurations are
> in use and the zero padding is no longer padding but holds the 64
> bit extent counter.
>
> This manifests obviously on big endian platforms (e.g. s390) because
> the log dinode is in host order and the overlap is the LSBs of the
> extent count field. It is not noticed on little endian machines
> because the overlap is at the MSB end of the extent count field and
> we need to get more than 2^^48 extents in the inode before it
> manifests. i.e. the heat death of the universe will occur before we
> see the problem in little endian machines.
>
> This is a zero-day issue for NREXT64=1 configuraitons on big endian
> machines. Fix it by only clearing di_flushiter on v2 inodes during
> recovery.
>
> Fixes: 9b7d16e34bbe ("xfs: Introduce XFS_DIFLAG2_NREXT64 and associated helpers")
> cc: stable@xxxxxxxxxx # 5.19+
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_inode_item_recover.c | 32 +++++++++++++++++---------------
> 1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> index f4c31c2b60d5..dbdab4ce7c44 100644
> --- a/fs/xfs/xfs_inode_item_recover.c
> +++ b/fs/xfs/xfs_inode_item_recover.c
> @@ -371,24 +371,26 @@ xlog_recover_inode_commit_pass2(
> * superblock flag to determine whether we need to look at di_flushiter
> * to skip replay when the on disk inode is newer than the log one
> */
> - if (!xfs_has_v3inodes(mp) &&
> - ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> - /*
> - * Deal with the wrap case, DI_MAX_FLUSH is less
> - * than smaller numbers
> - */
> - if (be16_to_cpu(dip->di_flushiter) == DI_MAX_FLUSH &&
> - ldip->di_flushiter < (DI_MAX_FLUSH >> 1)) {
> - /* do nothing */
> - } else {
> - trace_xfs_log_recover_inode_skip(log, in_f);
> - error = 0;
> - goto out_release;
> + if (!xfs_has_v3inodes(mp)) {
> + if (ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> + /*
> + * Deal with the wrap case, DI_MAX_FLUSH is less
> + * than smaller numbers
> + */
> + if (be16_to_cpu(dip->di_flushiter) == DI_MAX_FLUSH &&
> + ldip->di_flushiter < (DI_MAX_FLUSH >> 1)) {
> + /* do nothing */
> + } else {
> + trace_xfs_log_recover_inode_skip(log, in_f);
> + error = 0;
> + goto out_release;
> + }
> }
> +
> + /* Take the opportunity to reset the flush iteration count */
> + ldip->di_flushiter = 0;
> }
>
> - /* Take the opportunity to reset the flush iteration count */
> - ldip->di_flushiter = 0;
That's an unfortunate logic bomb left over from the V5 introduction. I
guess it was benign until we finally reused that part of the xfs_dinode.
If this fixes zorro's machine, then:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
I wonder, if we made XFS_SUPPORT_V4=n turn xfs_has_v3inodes and
xfs_has_crc into #define'd true symbols, could we then rename all the
V4-only fields to see what stops compiling?
(Probably not, gcc will still want to parse it all even if the source
code itself is dead...)
--D
>
> if (unlikely(S_ISREG(ldip->di_mode))) {
> if ((ldip->di_format != XFS_DINODE_FMT_EXTENTS) &&
