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
