Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling

["Darrick J. Wong" <djwong@kernel.org>]

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