[PATCH 0/2] xfs: don't take addresses of packed structures

[PATCH 0/2] xfs: don't take addresses of packed structures

[Eric Sandeen]

newer gcc complains when we try to get the address of a packed structure:

xfs_format.h:790:3: warning: taking address of packed member of ‘struct xfs_agfl’ may result in an unaligned pointer value [-Waddress-of-packed-member]

xfs_rmap_btree.c:188:15: warning: taking address of packed member of ‘struct xfs_rmap_key’ may result in an unaligned pointer value [-Waddress-of-packed-member]


Dave had sent a patch to turn the warning off globally, but that seems
like a big hammer, I dunno.  Here are 2 patches to work around it instead.

Not tested yet, just seeing how loud people may scream at the ideas.

[PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

[Eric Sandeen]

gcc now warns about taking an address of a packed structure member.

Work around this by using offsetof() instead.

Thanks to bfoster for the suggestion and djwong for reiterating it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 1b7dcbae051c..7bfc8e2437e9 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -787,7 +787,8 @@ typedef struct xfs_agi {
 
 #define XFS_BUF_TO_AGFL_BNO(mp, bp) \
 	(xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
-		&(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
+		(__be32 *)((char *)(bp)->b_addr + \
+			offsetof(struct xfs_agfl, agfl_bno)) : \
 		(__be32 *)(bp)->b_addr)
 
 typedef struct xfs_agfl {

[PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key

[Eric Sandeen]

gcc now warns about taking an address of a packed structure member.

This happens here because of how be32_add_cpu() works; just open-code
the modification using a temporary variable instead.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index fc78efa52c94..ad5ead62c992 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -182,12 +182,14 @@ xfs_rmapbt_init_high_key_from_rec(
 	union xfs_btree_rec	*rec)
 {
 	uint64_t		off;
+	xfs_agblock_t		start;
 	int			adj;
 
 	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
 
 	key->rmap.rm_startblock = rec->rmap.rm_startblock;
-	be32_add_cpu(&key->rmap.rm_startblock, adj);
+	start = be32_to_cpu(rec->rmap.rm_startblock) - adj;
+	rec->rmap.rm_startblock = cpu_to_be32(start);
 	key->rmap.rm_owner = rec->rmap.rm_owner;
 	key->rmap.rm_offset = rec->rmap.rm_offset;
 	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||

Re: [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key

[Darrick J. Wong]

On Wed, Jan 29, 2020 at 11:45:05AM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> This happens here because of how be32_add_cpu() works; just open-code
> the modification using a temporary variable instead.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index fc78efa52c94..ad5ead62c992 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -182,12 +182,14 @@ xfs_rmapbt_init_high_key_from_rec(
>  	union xfs_btree_rec	*rec)
>  {
>  	uint64_t		off;
> +	xfs_agblock_t		start;
>  	int			adj;
>  
>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>  
>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;

I was gonna say to kill this statement since you set it again two lines
later, but then spotted a bug two lines down...

> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
> +	start = be32_to_cpu(rec->rmap.rm_startblock) - adj;
> +	rec->rmap.rm_startblock = cpu_to_be32(start);

...this should be setting key->rmap.rm_startblock.

--D

>  	key->rmap.rm_owner = rec->rmap.rm_owner;
>  	key->rmap.rm_offset = rec->rmap.rm_offset;
>  	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||
> 
> 

Re: [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key

[Eric Sandeen]

On 1/29/20 11:56 AM, Darrick J. Wong wrote:
> On Wed, Jan 29, 2020 at 11:45:05AM -0600, Eric Sandeen wrote:
>> gcc now warns about taking an address of a packed structure member.
>>
>> This happens here because of how be32_add_cpu() works; just open-code
>> the modification using a temporary variable instead.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
>> index fc78efa52c94..ad5ead62c992 100644
>> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
>> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
>> @@ -182,12 +182,14 @@ xfs_rmapbt_init_high_key_from_rec(
>>  	union xfs_btree_rec	*rec)
>>  {
>>  	uint64_t		off;
>> +	xfs_agblock_t		start;
>>  	int			adj;
>>  
>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>  
>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
> 
> I was gonna say to kill this statement since you set it again two lines
> later, but then spotted a bug two lines down...
> 
>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>> +	start = be32_to_cpu(rec->rmap.rm_startblock) - adj;
>> +	rec->rmap.rm_startblock = cpu_to_be32(start);

/searches for that brown paper bag

thanks

-Eric

Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

[Christoph Hellwig]

On Wed, Jan 29, 2020 at 11:43:13AM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> Work around this by using offsetof() instead.
> 
> Thanks to bfoster for the suggestion and djwong for reiterating it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 1b7dcbae051c..7bfc8e2437e9 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -787,7 +787,8 @@ typedef struct xfs_agi {
>  
>  #define XFS_BUF_TO_AGFL_BNO(mp, bp) \
>  	(xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
> -		&(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
> +		(__be32 *)((char *)(bp)->b_addr + \
> +			offsetof(struct xfs_agfl, agfl_bno)) : \
>  		(__be32 *)(bp)->b_addr)

Yikes.  If we want to go down this route this really needs to become
an inline function (and fiven that it touches buffer is has no business
in xfs_format.h).

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.

[PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member

[Eric Sandeen]

gcc now warns about taking an address of a packed structure member.

This happens here because of how be32_add_cpu() works; just open-code
the modification using a temporary variable instead.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: fix key-> vs rec-> derp derp thinko

diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index fc78efa52c94..9e5ac773fc9e 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -182,12 +182,14 @@ xfs_rmapbt_init_high_key_from_rec(
 	union xfs_btree_rec	*rec)
 {
 	uint64_t		off;
+	xfs_agblock_t		start;
 	int			adj;
 
 	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
 
 	key->rmap.rm_startblock = rec->rmap.rm_startblock;
-	be32_add_cpu(&key->rmap.rm_startblock, adj);
+	start = be32_to_cpu(key->rmap.rm_startblock) - adj;
+	key->rmap.rm_startblock = cpu_to_be32(start);
 	key->rmap.rm_owner = rec->rmap.rm_owner;
 	key->rmap.rm_offset = rec->rmap.rm_offset;
 	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||

Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

[Eric Sandeen]

On 1/29/20 12:09 PM, Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 11:43:13AM -0600, Eric Sandeen wrote:
>> gcc now warns about taking an address of a packed structure member.
>>
>> Work around this by using offsetof() instead.
>>
>> Thanks to bfoster for the suggestion and djwong for reiterating it.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 1b7dcbae051c..7bfc8e2437e9 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -787,7 +787,8 @@ typedef struct xfs_agi {
>>  
>>  #define XFS_BUF_TO_AGFL_BNO(mp, bp) \
>>  	(xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
>> -		&(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
>> +		(__be32 *)((char *)(bp)->b_addr + \
>> +			offsetof(struct xfs_agfl, agfl_bno)) : \
>>  		(__be32 *)(bp)->b_addr)
> 
> Yikes.  If we want to go down this route this really needs to become
> an inline function (and fiven that it touches buffer is has no business
> in xfs_format.h).

fair point

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

TBH, the above construction actually makes a lot more intuitive sense to
me, alignment concerns or not.

-Eric

Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

[Christoph Hellwig]

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

Re: [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member

[Christoph Hellwig]

On Wed, Jan 29, 2020 at 12:15:06PM -0600, Eric Sandeen wrote:
>  {
>  	uint64_t		off;
> +	xfs_agblock_t		start;
>  	int			adj;
>  
>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>  
>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
> +	start = be32_to_cpu(key->rmap.rm_startblock) - adj;
> +	key->rmap.rm_startblock = cpu_to_be32(start);

Do we really need the local variable?  Why not:

	key->rmap.rm_startblock =
		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);

Re: [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member

[Eric Sandeen]

On 1/29/20 12:29 PM, Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 12:15:06PM -0600, Eric Sandeen wrote:
>>  {
>>  	uint64_t		off;
>> +	xfs_agblock_t		start;
>>  	int			adj;
>>  
>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>  
>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>> +	start = be32_to_cpu(key->rmap.rm_startblock) - adj;
>> +	key->rmap.rm_startblock = cpu_to_be32(start);
> 
> Do we really need the local variable?  Why not:
> 
> 	key->rmap.rm_startblock =
> 		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
> 

Sure

-Eric

[PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member

[Eric Sandeen]

gcc now warns about taking an address of a packed structure member.

This happens here because of how be32_add_cpu() works; just open-code
the modification instead.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: fix key-> vs rec-> derp derp thinko
V3: drop local temp variable, add comment

diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index fc78efa52c94..c6f6a7ec6121 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
 	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
 
 	key->rmap.rm_startblock = rec->rmap.rm_startblock;
-	be32_add_cpu(&key->rmap.rm_startblock, adj);
+	/* do this manually to avoid gcc warning about alignment */
+	key->rmap.rm_startblock =
+		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
 	key->rmap.rm_owner = rec->rmap.rm_owner;
 	key->rmap.rm_offset = rec->rmap.rm_offset;
 	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||

Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member

[Christoph Hellwig]

On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> This happens here because of how be32_add_cpu() works; just open-code
> the modification instead.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

[Eric Sandeen]

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
 

Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member

[Darrick J. Wong]

On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> gcc now warns about taking an address of a packed structure member.
> 
> This happens here because of how be32_add_cpu() works; just open-code
> the modification instead.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> V2: fix key-> vs rec-> derp derp thinko
> V3: drop local temp variable, add comment
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index fc78efa52c94..c6f6a7ec6121 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>  
>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
> +	/* do this manually to avoid gcc warning about alignment */
> +	key->rmap.rm_startblock =
> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);

<blink>

This should be getting the value from rec->rmap, not key->rmap.

This should be adding adj, not subtracting it, since that's what the
original code did.

And finally, there's no need to set the value twice.

--D

>  	key->rmap.rm_owner = rec->rmap.rm_owner;
>  	key->rmap.rm_offset = rec->rmap.rm_offset;
>  	if (XFS_RMAP_NON_INODE_OWNER(be64_to_cpu(rec->rmap.rm_owner)) ||
> 
> 

Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member

[Eric Sandeen]

On 2/26/20 10:06 AM, Darrick J. Wong wrote:
> On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:

...

>> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>  
>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>> +	/* do this manually to avoid gcc warning about alignment */
>> +	key->rmap.rm_startblock =
>> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
> 
> <blink>
> 
> This should be getting the value from rec->rmap, not key->rmap.
> 
> This should be adding adj, not subtracting it, since that's what the
> original code did.

*sigh* I got nothin' here tbh.

Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member

[Eric Sandeen]

On 2/26/20 10:21 AM, Eric Sandeen wrote:
> On 2/26/20 10:06 AM, Darrick J. Wong wrote:
>> On Wed, Jan 29, 2020 at 12:35:21PM -0600, Eric Sandeen wrote:
> 
> ...
> 
>>> @@ -187,7 +187,9 @@ xfs_rmapbt_init_high_key_from_rec(
>>>  	adj = be32_to_cpu(rec->rmap.rm_blockcount) - 1;
>>>  
>>>  	key->rmap.rm_startblock = rec->rmap.rm_startblock;
>>> -	be32_add_cpu(&key->rmap.rm_startblock, adj);
>>> +	/* do this manually to avoid gcc warning about alignment */
>>> +	key->rmap.rm_startblock =
>>> +		cpu_to_be32(be32_to_cpu(key->rmap.rm_startblock) - adj);
>>
>> <blink>
>>
>> This should be getting the value from rec->rmap, not key->rmap.
>>
>> This should be adding adj, not subtracting it, since that's what the
>> original code did.
> 
> *sigh* I got nothin' here tbh.
> 

Let's just drop this patch.  The first patch in this series is obviated by
hch's work and AFAIK the kernel just turned off this warning, we can do
the same in userspace and make this 2nd issue go away.

-Eric