* [PATCH 0/2] xfs: don't take addresses of packed structures @ 2020-01-29 17:41 Eric Sandeen 2020-01-29 17:43 ` [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member Eric Sandeen 2020-01-29 17:45 ` [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key Eric Sandeen 0 siblings, 2 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 17:41 UTC (permalink / raw) To: linux-xfs 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member 2020-01-29 17:41 [PATCH 0/2] xfs: don't take addresses of packed structures Eric Sandeen @ 2020-01-29 17:43 ` Eric Sandeen 2020-01-29 18:09 ` Christoph Hellwig 2020-01-29 17:45 ` [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key Eric Sandeen 1 sibling, 1 reply; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 17:43 UTC (permalink / raw) To: linux-xfs 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 { ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member 2020-01-29 17:43 ` [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member Eric Sandeen @ 2020-01-29 18:09 ` Christoph Hellwig 2020-01-29 18:18 ` Eric Sandeen 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-01-29 18:09 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member 2020-01-29 18:09 ` Christoph Hellwig @ 2020-01-29 18:18 ` Eric Sandeen 2020-01-29 18:28 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:18 UTC (permalink / raw) To: Christoph Hellwig, Eric Sandeen; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member 2020-01-29 18:18 ` Eric Sandeen @ 2020-01-29 18:28 ` Christoph Hellwig 2020-01-29 18:46 ` Eric Sandeen 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-01-29 18:28 UTC (permalink / raw) To: Eric Sandeen; +Cc: Christoph Hellwig, Eric Sandeen, linux-xfs 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) ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member 2020-01-29 18:28 ` Christoph Hellwig @ 2020-01-29 18:46 ` Eric Sandeen 0 siblings, 0 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:46 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Eric Sandeen, linux-xfs 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key 2020-01-29 17:41 [PATCH 0/2] xfs: don't take addresses of packed structures Eric Sandeen 2020-01-29 17:43 ` [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member Eric Sandeen @ 2020-01-29 17:45 ` Eric Sandeen 2020-01-29 17:56 ` Darrick J. Wong 2020-01-29 18:15 ` [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member Eric Sandeen 1 sibling, 2 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 17:45 UTC (permalink / raw) To: linux-xfs 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)) || ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key 2020-01-29 17:45 ` [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key Eric Sandeen @ 2020-01-29 17:56 ` Darrick J. Wong 2020-01-29 18:01 ` Eric Sandeen 2020-01-29 18:15 ` [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member Eric Sandeen 1 sibling, 1 reply; 17+ messages in thread From: Darrick J. Wong @ 2020-01-29 17:56 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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)) || > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key 2020-01-29 17:56 ` Darrick J. Wong @ 2020-01-29 18:01 ` Eric Sandeen 0 siblings, 0 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:01 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 17:45 ` [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key Eric Sandeen 2020-01-29 17:56 ` Darrick J. Wong @ 2020-01-29 18:15 ` Eric Sandeen 2020-01-29 18:29 ` Christoph Hellwig 2020-01-29 18:35 ` [PATCH 2/2 V3] " Eric Sandeen 1 sibling, 2 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:15 UTC (permalink / raw) To: linux-xfs 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)) || ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 18:15 ` [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member Eric Sandeen @ 2020-01-29 18:29 ` Christoph Hellwig 2020-01-29 18:31 ` Eric Sandeen 2020-01-29 18:35 ` [PATCH 2/2 V3] " Eric Sandeen 1 sibling, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2020-01-29 18:29 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 18:29 ` Christoph Hellwig @ 2020-01-29 18:31 ` Eric Sandeen 0 siblings, 0 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:31 UTC (permalink / raw) To: Christoph Hellwig, Eric Sandeen; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 18:15 ` [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member Eric Sandeen 2020-01-29 18:29 ` Christoph Hellwig @ 2020-01-29 18:35 ` Eric Sandeen 2020-01-29 18:36 ` Christoph Hellwig 2020-02-26 18:06 ` Darrick J. Wong 1 sibling, 2 replies; 17+ messages in thread From: Eric Sandeen @ 2020-01-29 18:35 UTC (permalink / raw) To: linux-xfs 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)) || ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 18:35 ` [PATCH 2/2 V3] " Eric Sandeen @ 2020-01-29 18:36 ` Christoph Hellwig 2020-02-26 18:06 ` Darrick J. Wong 1 sibling, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2020-01-29 18:36 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member 2020-01-29 18:35 ` [PATCH 2/2 V3] " Eric Sandeen 2020-01-29 18:36 ` Christoph Hellwig @ 2020-02-26 18:06 ` Darrick J. Wong 2020-02-26 18:21 ` Eric Sandeen 1 sibling, 1 reply; 17+ messages in thread From: Darrick J. Wong @ 2020-02-26 18:06 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs 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)) || > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member 2020-02-26 18:06 ` Darrick J. Wong @ 2020-02-26 18:21 ` Eric Sandeen 2020-02-26 18:45 ` Eric Sandeen 0 siblings, 1 reply; 17+ messages in thread From: Eric Sandeen @ 2020-02-26 18:21 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2 V3] xfs: don't take addresses of packed xfs_rmap_key member 2020-02-26 18:21 ` Eric Sandeen @ 2020-02-26 18:45 ` Eric Sandeen 0 siblings, 0 replies; 17+ messages in thread From: Eric Sandeen @ 2020-02-26 18:45 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-02-26 18:45 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-29 17:41 [PATCH 0/2] xfs: don't take addresses of packed structures Eric Sandeen 2020-01-29 17:43 ` [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member Eric Sandeen 2020-01-29 18:09 ` Christoph Hellwig 2020-01-29 18:18 ` Eric Sandeen 2020-01-29 18:28 ` Christoph Hellwig 2020-01-29 18:46 ` Eric Sandeen 2020-01-29 17:45 ` [PATCH 2/2] xfs: don't take addresses of packed xfs_rmap_key Eric Sandeen 2020-01-29 17:56 ` Darrick J. Wong 2020-01-29 18:01 ` Eric Sandeen 2020-01-29 18:15 ` [PATCH 2/2 V2] xfs: don't take addresses of packed xfs_rmap_key member Eric Sandeen 2020-01-29 18:29 ` Christoph Hellwig 2020-01-29 18:31 ` Eric Sandeen 2020-01-29 18:35 ` [PATCH 2/2 V3] " Eric Sandeen 2020-01-29 18:36 ` Christoph Hellwig 2020-02-26 18:06 ` Darrick J. Wong 2020-02-26 18:21 ` Eric Sandeen 2020-02-26 18:45 ` Eric Sandeen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.