On Wed, Apr 21, 2021 at 07:55:41AM +0200, Christoph Hellwig wrote: > On Tue, Apr 20, 2021 at 10:05:29AM -0700, Darrick J. Wong wrote: > > Hmm... so the behavior change here is that 32-bit kernels will start > > logging 16-byte xfs_extent structures (like 64-bit kernels)? > > Yes. > > > I see that > > xfs_extent_32 was added for 2.6.18; won't this break recovery on > > everything from before that? > > Where everything is a 32-bit kernel that doesn't align properly, yes. > > > Granted, 2.6.17 came out 15 years ago and the last 2.6.16 LTS kernel was > > released in 2008 so maybe we don't care, but this would seem to be a > > breaking change, right? This seems like a reasonable change for all V5 > > filesystems (since that format emerged well after 2.6.18), but not so > > good for V4. > > Err, why? Log recovery on those old kernels will choke on the 16-bit xfs_extent records, because they only know how to interpret a 12-bit xfs_extent. In 2.6.17, xlog_recover_do_efi_trans did this: efi_formatp = (xfs_efi_log_format_t *)item->ri_buf[0].i_addr; ASSERT(item->ri_buf[0].i_len == (sizeof(xfs_efi_log_format_t) + ((efi_formatp->efi_nextents - 1) * sizeof(xfs_extent_t)))); The ASSERT will trigger on the size being wrong due to the padding error, but on non-DEBUG kernels that won't interrupt log recovery, so we proceed on to this: memcpy((char *)&(efip->efi_format), (char *)efi_formatp, sizeof(xfs_efi_log_format_t) + ((efi_formatp->efi_nextents - 1) * sizeof(xfs_extent_t))); In this particular case, we fail to copy the all of the bytes from the recovered EFI into the new incore EFI log item. If there is more than 1 extent, the fields in the (N+1)th incore extent won't line up with the fields that were written to disk, which means we'll replay garbage. --D
