On 29 Sep 2021 at 04:38, Dave Chinner wrote:

On Tue, Sep 28, 2021 at 03:17:59PM +0530, Chandan Babu R wrote:

quoted

On 28 Sep 2021 at 06:17, Dave Chinner wrote:

quoted

On Thu, Sep 16, 2021 at 03:36:43PM +0530, Chandan Babu R wrote:

quoted

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 <redacted>

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:

quoted

-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.

quoted

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