On 1/29/20 12:28 PM, Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 12:18:16PM -0600, Eric Sandeen wrote:
>>>
>>> But I absolutely do not see the point. If agfl_bno was unalgined
>>> so is adding the offsetoff. The warnings makes no sense, and there is
>>> a good reason the kernel build turned it off.
>>
>> Why do the warnings make no sense?
>
> Because taking the address of a member of a packed struct itself doesn't
> mean it is misaligned. Taking the address of misaligned member does
> that. And adding a non-aligned offset to a pointer will make it just
> as misaligned.
>
>> TBH, the above construction actually makes a lot more intuitive sense to
>> me, alignment concerns or not.
>
> Using offsetoff to take the address of a struct member is really
> strange.
>
> If we want to stop taking the address of agfl_bno we should just remove
> the field entirely:
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index fc93fd88ec89..d91177c4a1e4 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -585,11 +585,12 @@ xfs_alloc_fixup_trees(
>
> static xfs_failaddr_t
> xfs_agfl_verify(
> - struct xfs_buf *bp)
> + struct xfs_buf *bp)
> {
> - struct xfs_mount *mp = bp->b_mount;
> - struct xfs_agfl *agfl = XFS_BUF_TO_AGFL(bp);
> - int i;
> + struct xfs_mount *mp = bp->b_mount;
> + struct xfs_agfl *agfl = XFS_BUF_TO_AGFL(bp);
> + __be32 *agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp);
> + int i;
>
> /*
> * There is no verification of non-crc AGFLs because mkfs does not
> @@ -614,8 +615,8 @@ xfs_agfl_verify(
> return __this_address;
>
> for (i = 0; i < xfs_agfl_size(mp); i++) {
> - if (be32_to_cpu(agfl->agfl_bno[i]) != NULLAGBLOCK &&
> - be32_to_cpu(agfl->agfl_bno[i]) >= mp->m_sb.sb_agblocks)
> + if (be32_to_cpu(agfl_bno[i]) != NULLAGBLOCK &&
> + be32_to_cpu(agfl_bno[i]) >= mp->m_sb.sb_agblocks)
> return __this_address;
> }
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 77e9fa385980..0d0a6616e129 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -783,21 +783,21 @@ typedef struct xfs_agi {
> */
> #define XFS_AGFL_DADDR(mp) ((xfs_daddr_t)(3 << (mp)->m_sectbb_log))
> #define XFS_AGFL_BLOCK(mp) XFS_HDR_BLOCK(mp, XFS_AGFL_DADDR(mp))
> -#define XFS_BUF_TO_AGFL(bp) ((xfs_agfl_t *)((bp)->b_addr))
> +#define XFS_BUF_TO_AGFL(bp) ((struct xfs_agfl *)((bp)->b_addr))
>
> -#define XFS_BUF_TO_AGFL_BNO(mp, bp) \
> - (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
> - &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
> - (__be32 *)(bp)->b_addr)
> +#define XFS_BUF_TO_AGFL_BNO(mp, bp) \
> + ((__be32 *) \
> + (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
> + (bp)->b_addr : \
> + ((bp)->b_addr + sizeof(struct xfs_agfl))))
This is backwards
>
> -typedef struct xfs_agfl {
> +struct xfs_agfl {
> __be32 agfl_magicnum;
> __be32 agfl_seqno;
> uuid_t agfl_uuid;
> __be64 agfl_lsn;
> __be32 agfl_crc;
> - __be32 agfl_bno[]; /* actually xfs_agfl_size(mp) */
> -} __attribute__((packed)) xfs_agfl_t;
> +} __attribute__((packed));
>
> #define XFS_AGFL_CRC_OFF offsetof(struct xfs_agfl, agfl_crc)
<peers past whitespace changes> ;)
other than that, this approach seems reasonable too.
-Eric
