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@xxxxxxxxxx>
>> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
>
> 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
