Re: [PATCH V3 08/12] xfs: Promote xfs_extnum_t and xfs_aextnum_t to 64 and 32-bits respectively

[Chandan Babu R <chandan.babu@oracle.com>]

On 28 Sep 2021 at 06:17, Dave Chinner wrote:
> On Thu, Sep 16, 2021 at 03:36:43PM +0530, Chandan Babu R wrote:
>> A future commit will introduce a 64-bit on-disk data extent counter and a
>> 32-bit on-disk attr extent counter. This commit promotes xfs_extnum_t and
>> xfs_aextnum_t to 64 and 32-bits in order to correctly handle in-core versions
>> of these quantities.
>> 
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>
> So while I was auditing extent lengths w.r.t. the last patch f the
> series, I noticed that xfs_extnum_t is used in the struct
> xfs_log_dinode and so changing the size of these types changes the
> layout of this structure:
>
> /*
>  * Define the format of the inode core that is logged. This structure must be
>  * kept identical to struct xfs_dinode except for the endianness annotations.
>  */
> struct xfs_log_dinode {
> ....
>         xfs_rfsblock_t  di_nblocks;     /* # of direct & btree blocks used */
>         xfs_extlen_t    di_extsize;     /* basic/minimum extent size for file */
>         xfs_extnum_t    di_nextents;    /* number of extents in data fork */
>         xfs_aextnum_t   di_anextents;   /* number of extents in attribute fork*/
> ....
>
> Which means this:
>
>> -typedef int32_t		xfs_extnum_t;	/* # of extents in a file */
>> -typedef int16_t		xfs_aextnum_t;	/* # extents in an attribute fork */
>> +typedef uint64_t	xfs_extnum_t;	/* # of extents in a file */
>> +typedef uint32_t	xfs_aextnum_t;	/* # extents in an attribute fork */
>
> creates an incompatible log format change that will cause silent
> inode corruption during log recovery if inodes logged with this
> change are replayed on an older kernel without this change. It's not
> just the type size change that matters here - it also changes the
> implicit padding in this structure because xfs_extlen_t is a 32 bit
> object and so:
>
> Old					New
> 64 bit object (di_nblocks)		64 bit object (di_nblocks)
> 32 bit object (di_extsize)		32 bit object (di_extsize)
> 					32 bit pad (implicit)
> 32 bit object (di_nextents)		64 bit object (di_nextents)
> 16 bit object (di_anextents)		32 bit ojecct (di_anextents
> 8 bit object (di_forkoff)		8 bit object (di_forkoff)
> 8 bit object (di_aformat)		8 bit object (di_aformat)
> 					16 bit pad (implicit)
> 32 bit object (di_dmevmask)		32 bit object (di_dmevmask)
>
>
> That's quite the layout change, and that's something we must not do
> without a feature bit being set. hence I think we need to rev the
> struct xfs_log_dinode version for large extent count support, too,
> so that the struct xfs_log_dinode does not change size for
> filesystems without the large extent count feature.

Actually, the current patch replaces the data types xfs_extnum_t and
xfs_aextnum_t inside "struct xfs_log_dinode" with the basic integral types
uint32_t and uint16_t respectively. The patch "xfs: Extend per-inode extent
counter widths" which arrives later in the series adds the new field
di_nextents64 to "struct xfs_log_dinode" and uint64_t is used as its data
type.

So in a scenario where we have a filesystem which does not have support for
64-bit extent counters and a kernel which does not support 64-bit extent
counters is replaying a log created by a kernel supporting 64-bit extent
counters, the contents of the 16-bit and 32-bit extent counter fields should
be replayed correctly into xfs_inode's attr and data fork extent counters
respectively. The contents of the 64-bit extent counter (whose value will be
zero) in the logged inode will be replayed back into di_pad2[] field of the
inode.

Please do let me know if my explaination is incorrect.

-- 
chandan