| Message ID | 20200130133343.225818-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] xfs: remove the agfl_bno member from struct xfs_agfl | expand |
On 1/30/20 7:33 AM, Christoph Hellwig wrote: > struct xfs_agfl is a header in front of the AGFL entries that exists > for CRC enabled file systems. For not CRC enabled file systems the AGFL > is simply a list of agbno. Make the CRC case similar to that by just > using the list behind the new header. This indirectly solves a problem > with modern gcc versions that warn about taking addresses of packed > structures (and we have to pack the AGFL given that gcc rounds up > structure sizes). Also replace the helper macro to get from a buffer > with an inline function in xfs_alloc.h to make the code easier to > read. > > Signed-off-by: Christoph Hellwig <hch@lst.de> I like it. Giving it an RVB but are we 100% sure that there won't ever be any padding after the xfs_agfl structure before the bno? I don't understand gcc alignment magic. Reviewed-by: Eric Sandeen <sandeen@redhat.com>
On Mon, Feb 03, 2020 at 11:46:11AM -0600, Eric Sandeen wrote: > On 1/30/20 7:33 AM, Christoph Hellwig wrote: > > struct xfs_agfl is a header in front of the AGFL entries that exists > > for CRC enabled file systems. For not CRC enabled file systems the AGFL > > is simply a list of agbno. Make the CRC case similar to that by just > > using the list behind the new header. This indirectly solves a problem > > with modern gcc versions that warn about taking addresses of packed > > structures (and we have to pack the AGFL given that gcc rounds up > > structure sizes). Also replace the helper macro to get from a buffer > > with an inline function in xfs_alloc.h to make the code easier to > > read. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > I like it. > > Giving it an RVB but are we 100% sure that there won't ever be any padding > after the xfs_agfl structure before the bno? I don't understand gcc > alignment magic. That is at least the definition of __attribute__((packed))..
On Thu, Jan 30, 2020 at 02:33:38PM +0100, Christoph Hellwig wrote: > struct xfs_agfl is a header in front of the AGFL entries that exists > for CRC enabled file systems. For not CRC enabled file systems the AGFL > is simply a list of agbno. Make the CRC case similar to that by just > using the list behind the new header. This indirectly solves a problem > with modern gcc versions that warn about taking addresses of packed > structures (and we have to pack the AGFL given that gcc rounds up > structure sizes). Also replace the helper macro to get from a buffer > with an inline function in xfs_alloc.h to make the code easier to > read. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Any chance we can pick this up for 5.6 to unbreak arm OABI?
On Mon, Feb 24, 2020 at 02:02:56PM -0800, Christoph Hellwig wrote: > On Thu, Jan 30, 2020 at 02:33:38PM +0100, Christoph Hellwig wrote: > > struct xfs_agfl is a header in front of the AGFL entries that exists > > for CRC enabled file systems. For not CRC enabled file systems the AGFL > > is simply a list of agbno. Make the CRC case similar to that by just > > using the list behind the new header. This indirectly solves a problem > > with modern gcc versions that warn about taking addresses of packed > > structures (and we have to pack the AGFL given that gcc rounds up > > structure sizes). Also replace the helper macro to get from a buffer > > with an inline function in xfs_alloc.h to make the code easier to > > read. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Any chance we can pick this up for 5.6 to unbreak arm OABI? Yeah, I can do that. Is there a Fixes: tag that goes with this? Also, will you have a chance to respin the last patch for 5.7? --D
On Mon, Feb 24, 2020 at 02:19:31PM -0800, Darrick J. Wong wrote: > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > Any chance we can pick this up for 5.6 to unbreak arm OABI? > > Yeah, I can do that. Is there a Fixes: tag that goes with this? I'm not sure what to add. I think the problem itself has actually always been around since adding v5 fs support. But the build break was only caused by the addition of the BUILD_BUG_ON. > Also, will you have a chance to respin the last patch for 5.7? Last patch in this series?
On Mon, Feb 24, 2020 at 02:21:18PM -0800, Christoph Hellwig wrote: > On Mon, Feb 24, 2020 at 02:19:31PM -0800, Darrick J. Wong wrote: > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > > Any chance we can pick this up for 5.6 to unbreak arm OABI? > > > > Yeah, I can do that. Is there a Fixes: tag that goes with this? > > I'm not sure what to add. I think the problem itself has actually > always been around since adding v5 fs support. But the build break > was only caused by the addition of the BUILD_BUG_ON. Hmm. That's tricky, since in theory this should go all the way back to the introduction of the v5 format in 3.x, but that's going to require explicit backporting to get past all the reorganization and other things that have happened. We might just have to hand-backport it to the stable kernels seeing how the macro name change will probably cause all sorts of problems with AI backports. :/ > > Also, will you have a chance to respin the last patch for 5.7? > > Last patch in this series? Yes. From the discussion of patch 6/6, "+ __xfs_sb_from_disk(&sb, bp->b_addr, false); "why not dsb here "Yes, this should just pass dsb." --D
On 2/24/20 2:02 PM, Christoph Hellwig wrote: > On Thu, Jan 30, 2020 at 02:33:38PM +0100, Christoph Hellwig wrote: >> struct xfs_agfl is a header in front of the AGFL entries that exists >> for CRC enabled file systems. For not CRC enabled file systems the AGFL >> is simply a list of agbno. Make the CRC case similar to that by just >> using the list behind the new header. This indirectly solves a problem >> with modern gcc versions that warn about taking addresses of packed >> structures (and we have to pack the AGFL given that gcc rounds up >> structure sizes). Also replace the helper macro to get from a buffer >> with an inline function in xfs_alloc.h to make the code easier to >> read. >> >> Signed-off-by: Christoph Hellwig <hch@lst.de> > > Any chance we can pick this up for 5.6 to unbreak arm OABI? > What did I miss, where's the report of actual breakage vs. (I thought) harmless GCC complaints? -Eric
On Mon, Feb 24, 2020 at 02:27:49PM -0800, Eric Sandeen wrote: > On 2/24/20 2:02 PM, Christoph Hellwig wrote: > > On Thu, Jan 30, 2020 at 02:33:38PM +0100, Christoph Hellwig wrote: > >> struct xfs_agfl is a header in front of the AGFL entries that exists > >> for CRC enabled file systems. For not CRC enabled file systems the AGFL > >> is simply a list of agbno. Make the CRC case similar to that by just > >> using the list behind the new header. This indirectly solves a problem > >> with modern gcc versions that warn about taking addresses of packed > >> structures (and we have to pack the AGFL given that gcc rounds up > >> structure sizes). Also replace the helper macro to get from a buffer > >> with an inline function in xfs_alloc.h to make the code easier to > >> read. > >> > >> Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > Any chance we can pick this up for 5.6 to unbreak arm OABI? > > > > What did I miss, where's the report of actual breakage vs. > (I thought) harmless GCC complaints? The "harmless" gcc complaint is that the kernel build errors out as soon as XFS is enabled on arm OABI. Which is a good thing, as the file system would not be interoperable with other architectures if it didn't.
On 2/24/20 2:30 PM, Christoph Hellwig wrote: > On Mon, Feb 24, 2020 at 02:27:49PM -0800, Eric Sandeen wrote: >> On 2/24/20 2:02 PM, Christoph Hellwig wrote: >>> On Thu, Jan 30, 2020 at 02:33:38PM +0100, Christoph Hellwig wrote: >>>> struct xfs_agfl is a header in front of the AGFL entries that exists >>>> for CRC enabled file systems. For not CRC enabled file systems the AGFL >>>> is simply a list of agbno. Make the CRC case similar to that by just >>>> using the list behind the new header. This indirectly solves a problem >>>> with modern gcc versions that warn about taking addresses of packed >>>> structures (and we have to pack the AGFL given that gcc rounds up >>>> structure sizes). Also replace the helper macro to get from a buffer >>>> with an inline function in xfs_alloc.h to make the code easier to >>>> read. >>>> >>>> Signed-off-by: Christoph Hellwig <hch@lst.de> >>> >>> Any chance we can pick this up for 5.6 to unbreak arm OABI? >>> >> >> What did I miss, where's the report of actual breakage vs. >> (I thought) harmless GCC complaints? > > The "harmless" gcc complaint is that the kernel build errors out as > soon as XFS is enabled on arm OABI. Which is a good thing, as the > file system would not be interoperable with other architectures if it > didn't. Not just on latest GCC? Ok, I just hadn't seen that reported (but, I miss lots). And the commit log doesn't mention any actual breakage; it sounds like patch to work around a new gcc warning, not an actual fix for anything real. -Eric
On Mon, Feb 24, 2020 at 02:27:37PM -0800, Darrick J. Wong wrote: > On Mon, Feb 24, 2020 at 02:21:18PM -0800, Christoph Hellwig wrote: > > On Mon, Feb 24, 2020 at 02:19:31PM -0800, Darrick J. Wong wrote: > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > > > > > Any chance we can pick this up for 5.6 to unbreak arm OABI? > > > > > > Yeah, I can do that. Is there a Fixes: tag that goes with this? > > > > I'm not sure what to add. I think the problem itself has actually > > always been around since adding v5 fs support. But the build break > > was only caused by the addition of the BUILD_BUG_ON. > > Hmm. That's tricky, since in theory this should go all the way back to > the introduction of the v5 format in 3.x, but that's going to require > explicit backporting to get past all the reorganization and other things > that have happened. We might just have to hand-backport it to the > stable kernels seeing how the macro name change will probably cause all > sorts of problems with AI backports. :/ So which fixes tag do you want? Or feel free to just add the one you feel fits best. > > > Also, will you have a chance to respin the last patch for 5.7? > > > > Last patch in this series? > > Yes. From the discussion of patch 6/6, > > "+ __xfs_sb_from_disk(&sb, bp->b_addr, false); > > "why not dsb here > > "Yes, this should just pass dsb." Oh. I've actually had the respun branch on my box since a day after that comment. But I think it doesn't make sense until the fix in patch one is in the baseline tree, given how many outstanding patch series we have.
On Mon, Feb 24, 2020 at 02:35:08PM -0800, Eric Sandeen wrote: > > The "harmless" gcc complaint is that the kernel build errors out as > > soon as XFS is enabled on arm OABI. Which is a good thing, as the > > file system would not be interoperable with other architectures if it > > didn't. > > Not just on latest GCC? AFAIK all versions of gcc, as that is the intent of BUILD_BUG_ON.
On 2/24/20 2:46 PM, Christoph Hellwig wrote: > On Mon, Feb 24, 2020 at 02:35:08PM -0800, Eric Sandeen wrote: >>> The "harmless" gcc complaint is that the kernel build errors out as >>> soon as XFS is enabled on arm OABI. Which is a good thing, as the >>> file system would not be interoperable with other architectures if it >>> didn't. >> >> Not just on latest GCC? > > AFAIK all versions of gcc, as that is the intent of BUILD_BUG_ON. Right, makes sense. So let's please commit a changelog that makes it clear that this fixes an actual alignment problem and build breakage, ok? Thanks, -Eric
So actually I think I was confused. The BUILD_BUG_ON is what we had before adding the __packed. What we fix here іs just a really silly warning from new gcc, which in fact only happens in xfsprogs as the kernel decided that the warning is so damn stupid that we won't enable it. So no need for urgent treatment or a new commit message, sorry for the noise. I'll just resend the whole series with the review comments addressed.
diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c index 08d6beb54f8c..831bdd035900 100644 --- a/fs/xfs/libxfs/xfs_ag.c +++ b/fs/xfs/libxfs/xfs_ag.c @@ -301,7 +301,7 @@ xfs_agflblock_init( uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid); } - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp); + agfl_bno = xfs_buf_to_agfl_bno(bp); for (bucket = 0; bucket < xfs_agfl_size(mp); bucket++) agfl_bno[bucket] = cpu_to_be32(NULLAGBLOCK); } diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index d8053bc96c4d..b95a688e9f87 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -589,6 +589,7 @@ xfs_agfl_verify( { struct xfs_mount *mp = bp->b_mount; struct xfs_agfl *agfl = XFS_BUF_TO_AGFL(bp); + __be32 *agfl_bno = xfs_buf_to_agfl_bno(bp); int i; /* @@ -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; } @@ -2684,7 +2685,7 @@ xfs_alloc_get_freelist( /* * Get the block number and update the data structures. */ - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp); + agfl_bno = xfs_buf_to_agfl_bno(agflbp); bno = be32_to_cpu(agfl_bno[be32_to_cpu(agf->agf_flfirst)]); be32_add_cpu(&agf->agf_flfirst, 1); xfs_trans_brelse(tp, agflbp); @@ -2820,7 +2821,7 @@ xfs_alloc_put_freelist( ASSERT(be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp)); - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp); + agfl_bno = xfs_buf_to_agfl_bno(agflbp); blockp = &agfl_bno[be32_to_cpu(agf->agf_fllast)]; *blockp = cpu_to_be32(bno); startoff = (char *)blockp - (char *)agflbp->b_addr; @@ -3408,7 +3409,7 @@ xfs_agfl_walk( unsigned int i; int error; - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp); + agfl_bno = xfs_buf_to_agfl_bno(agflbp); i = be32_to_cpu(agf->agf_flfirst); /* Nothing to walk in an empty AGFL. */ diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h index 7380fbe4a3ff..a851bf77f17b 100644 --- a/fs/xfs/libxfs/xfs_alloc.h +++ b/fs/xfs/libxfs/xfs_alloc.h @@ -236,4 +236,13 @@ typedef int (*xfs_agfl_walk_fn)(struct xfs_mount *mp, xfs_agblock_t bno, int xfs_agfl_walk(struct xfs_mount *mp, struct xfs_agf *agf, struct xfs_buf *agflbp, xfs_agfl_walk_fn walk_fn, void *priv); +static inline __be32 * +xfs_buf_to_agfl_bno( + struct xfs_buf *bp) +{ + if (xfs_sb_version_hascrc(&bp->b_mount->m_sb)) + return bp->b_addr + sizeof(struct xfs_agfl); + return bp->b_addr; +} + #endif /* __XFS_ALLOC_H__ */ diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 77e9fa385980..8c7aea7795da 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -785,18 +785,12 @@ typedef struct xfs_agi { #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_BNO(mp, bp) \ - (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \ - &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \ - (__be32 *)(bp)->b_addr) - typedef 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; #define XFS_AGFL_CRC_OFF offsetof(struct xfs_agfl, agfl_crc) diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c index d5e6db9af434..68ee1ce1ae36 100644 --- a/fs/xfs/scrub/agheader_repair.c +++ b/fs/xfs/scrub/agheader_repair.c @@ -602,7 +602,7 @@ xrep_agfl_init_header( * step. */ fl_off = 0; - agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agfl_bp); + agfl_bno = xfs_buf_to_agfl_bno(agfl_bp); for_each_xfs_bitmap_extent(br, n, agfl_extents) { agbno = XFS_FSB_TO_AGBNO(mp, br->start);
struct xfs_agfl is a header in front of the AGFL entries that exists for CRC enabled file systems. For not CRC enabled file systems the AGFL is simply a list of agbno. Make the CRC case similar to that by just using the list behind the new header. This indirectly solves a problem with modern gcc versions that warn about taking addresses of packed structures (and we have to pack the AGFL given that gcc rounds up structure sizes). Also replace the helper macro to get from a buffer with an inline function in xfs_alloc.h to make the code easier to read. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_ag.c | 2 +- fs/xfs/libxfs/xfs_alloc.c | 11 ++++++----- fs/xfs/libxfs/xfs_alloc.h | 9 +++++++++ fs/xfs/libxfs/xfs_format.h | 6 ------ fs/xfs/scrub/agheader_repair.c | 2 +- 5 files changed, 17 insertions(+), 13 deletions(-)