On 29 Sep 2021 at 04:38, Dave Chinner wrote:
> On Tue, Sep 28, 2021 at 03:17:59PM +0530, Chandan Babu R wrote:
>> 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.
Sorry, The previous patch is the one which changes the data type of the extent
counter fields in "struct xfs_log_dinode".
>
> Arggh.
>
> Perhaps now you might see why I really don't like naming things by
> size and having the contents of those fields based on context? It
> is so easy to miss things like when the wrong variable or type is
> used for a given context because the code itself gives you no hint
> as to what the correct usage it.
I agree. I will go with the "Increment inode version" suggestion.
>
> I suspect part of the problem I'm had here is that the change of
> the type in the xfs_log_dinode is done in a -variable rename- patch
> that names variables by size, not in the patch that -actually
> changes the variable size-.
>
> IOWs, the type change in the xfs_log_dinode should
> either be in this patch where the log_dinode structure shape would
> change, or in it's own standalone patch with a description that says
> "we need to avoid changing the on-disk structure shape".
I think I will put the data type change in a separate patch to make it much
easier to spot. Thanks for suggesting that.
>
> Making sure that the on-disk format changes (or things that avoid
> them!) are clear and explicit in a patchset is critical as these are
> things we really need to get right.
>
> I missed the per-inode extent size flag for a similar reason - it
> was buried in a larger patch that made lots of different
> modifications to support the on-disk extent count format change, so
> it wasn't clearly defined/called out as a separate on-disk format
> change necessary for correct functioning.
>
You are right. I will pull out critical parts of the "xfs: Extend per-inode
extent counter widths" into as many separate patches as possible.
>> 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.
>
> I think that's correct, because the superblock bit will prevent
> mount on old kernels that don't support the 64 bit extent counter
> and so the zeroes in di_pad2 won't get overwritten incorrectly.
>
> Cheers,
>
> Dave.
--
chandan
