Re: [PATCH V3 07/12] xfs: Rename inode's extent counter fields based on their width

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

On 13 Oct 2021 at 20:14, Chandan Babu R wrote:
> On 11 Oct 2021 at 03:19, Dave Chinner wrote:
>> On Thu, Oct 07, 2021 at 04:22:25PM +0530, Chandan Babu R wrote:
>>> On 01 Oct 2021 at 04:25, Dave Chinner wrote:
>>> > On Thu, Sep 30, 2021 at 01:00:00PM +0530, Chandan Babu R wrote:
>>> >> On 30 Sep 2021 at 10:01, Dave Chinner wrote:
>>> >> > On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote:
>>> >> >
>>> >> 
[...]
>>> >> > FWIW, I also think doing something like this would help make the
>>> >> > code be easier to read and confirm that it is obviously correct when
>>> >> > reading it:
>>> >> >
>>> >> > 	__be32          di_gid;         /* owner's group id */
>>> >> > 	__be32          di_nlink;       /* number of links to file */
>>> >> > 	__be16          di_projid_lo;   /* lower part of owner's project id */
>>> >> > 	__be16          di_projid_hi;   /* higher part owner's project id */
>>> >> > 	union {
>>> >> > 		__be64	di_big_dextcnt;	/* NREXT64 data extents */
>>> >> > 		__u8	di_v3_pad[8];	/* !NREXT64 V3 inode zeroed space */
>>> >> > 		struct {
>>> >> > 			__u8	di_v2_pad[6];	/* V2 inode zeroed space */
>>> >> > 			__be16	di_flushiter;	/* V2 inode incremented on flush */
>>> >> > 		};
>>> >> > 	};
>>> >> > 	xfs_timestamp_t di_atime;       /* time last accessed */
>>> >> > 	xfs_timestamp_t di_mtime;       /* time last modified */
>>> >> > 	xfs_timestamp_t di_ctime;       /* time created/inode modified */
>>> >> > 	__be64          di_size;        /* number of bytes in file */
>>> >> > 	__be64          di_nblocks;     /* # of direct & btree blocks used */
>>> >> > 	__be32          di_extsize;     /* basic/minimum extent size for file */
>>> >> > 	union {
>>> >> > 		struct {
>>> >> > 			__be32	di_big_aextcnt; /* NREXT64 attr extents */
>>> >> > 			__be16	di_nrext64_pad;	/* NREXT64 unused, zero */
>>> >> > 		};
>>> >> > 		struct {
>>> >> > 			__be32	di_nextents;    /* !NREXT64 data extents */
>>> >> > 			__be16	di_anextents;   /* !NREXT64 attr extents */
>>> >> > 		}
>>> >> > 	}
>>> 
>>> The two structures above result in padding and hence result in a hole being
>>> introduced. The entire union above can be replaced with the following,
>>> 
>>>         union {
>>>                 __be32  di_big_aextcnt; /* NREXT64 attr extents */
>>>                 __be32  di_nextents;    /* !NREXT64 data extents */
>>>         };
>>>         union {
>>>                 __be16  di_nrext64_pad; /* NREXT64 unused, zero */
>>>                 __be16  di_anextents;   /* !NREXT64 attr extents */
>>>         };
>>
>> I don't think this makes sense. This groups by field rather than
>> by feature layout. It doesn't make it clear at all that these
>> varaibles both change definition at the same time - they are either
>> {di_nexts, di_anexts} pair or a {di_big_aexts, pad} pair. That's the
>> whole point of using anonymous structs here - it defines and
>> documents the relationship between the layouts when certain features
>> are set rather than relying on people to parse the comments
>> correctly to determine the relationship....
>
> Ok. I will need to check if there are alternative ways of arranging the fields
> to accomplish the goal stated above. I will think about this and get back as
> soon as possible.

The padding that results from the following structure layout,

typedef struct xfs_dinode {
        __be16          di_magic;       /* inode magic # = XFS_DINODE_MAGIC */
        __be16          di_mode;        /* mode and type of file */
        __u8            di_version;     /* inode version */
        __u8            di_format;      /* format of di_c data */
        __be16          di_onlink;      /* old number of links to file */
        __be32          di_uid;         /* owner's user id */
        __be32          di_gid;         /* owner's group id */
        __be32          di_nlink;       /* number of links to file */
        __be16          di_projid_lo;   /* lower part of owner's project id */
        __be16          di_projid_hi;   /* higher part owner's project id */
        __u8            di_pad[6];      /* unused, zeroed space */
        __be16          di_flushiter;   /* incremented on flush */
        xfs_timestamp_t di_atime;       /* time last accessed */
        xfs_timestamp_t di_mtime;       /* time last modified */
        xfs_timestamp_t di_ctime;       /* time created/inode modified */
        __be64          di_size;        /* number of bytes in file */
        __be64          di_nblocks;     /* # of direct & btree blocks used */
        __be32          di_extsize;     /* basic/minimum extent size for file */
        union {
                struct {
                        __be32  di_big_aextcnt; /* NREXT64 attr extents */
                        __be16  di_nrext64_pad; /* NREXT64 unused, zero */
                };
                struct {
                        __be32  di_nextents;    /* !NREXT64 data extents */
                        __be16  di_anextents;   /* !NREXT64 attr extents */
                };
        };
        __u8            di_forkoff;     /* attr fork offs, <<3 for 64b align */
        __s8            di_aformat;     /* format of attr fork's data */

... can be solved by packing the two structures contained within the union i.e.

        union {
                struct {
                        __be32  di_big_aextcnt; /* NREXT64 attr extents */
                        __be16  di_nrext64_pad; /* NREXT64 unused, zero */
                } __packed;
                struct {
                        __be32  di_nextents;    /* !NREXT64 data extents */
                        __be16  di_anextents;   /* !NREXT64 attr extents */
                } __packed;
        };
        __u8            di_forkoff;     /* attr fork offs, <<3 for 64b align */
        __s8            di_aformat;     /* format of attr fork's data */

Each of the two structures start at an 8-byte offset and the two 1-byte fields
(di_forkoff & di_aformat) defined later in the structure, prevent introduction
of holes inside dinode structure.

Also, An exception shouldn't be generated if the address of any of the packed
structure members is assigned to another pointer variable and later the
pointer variable is dereferenced. This is because such an address would still
be a 4-byte aligned address (in the case of di_big_aextcnt/di_nextents) or a
2-byte aligned address (in the case of di_nrext64_pad/di_anextents).

-- 
chandan