* [PATCH 0/6] xfs: accumulated fixes for 4.6
@ 2016-03-06 21:49 Dave Chinner
2016-03-06 21:49 ` [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery Dave Chinner
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw)
To: xfs
Hi folks,
This series is mostly fixes pulled from Darrick's rmap tree and one
for a growfs log recovery fix from me. I'm posting them mainly for a
final check before I merge them, and so everyone knows what is going
to be merged from Darrick's trees before the merge actually occurs.
Realistically, I'm not going to wait for days for people to
check/review these patch series before I merge them. If i don't hear
any objections within a couple of days, I'll simply push them out
into the for-next tree. Yes, I know this is a bit different to the
normal process, but I'm not posting these patches from Darrick's
tree until I've already reviewed (and therefore tested) them.
Cheers,
Dave.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-07 16:24 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 2/6] xfs: ioends require logically contiguous file offsets Dave Chinner ` (4 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> If a crash occurs immediately after a filesystem grow operation, the updated superblock geometry is found only in the log. After we recover the log, the superblock is reread and re-initialised and so has the new geometry in memory. If the new geometry has more AGs than prior to the grow operation, then the new AGs will not have in-memory xfs_perag structurea associated with them. This will result in an oops when the first metadata buffer from a new AG is looked up in the buffer cache, as the block lies within the new geometry but then fails to find a perag structure on lookup. This is easily fixed by simply re-initialising the perag structure after re-reading the superblock at the conclusion of the first pahse of log recovery. This, however, does not fix the case of log recovery requiring access to metadata in the newly grown space. Fortunately for us, because the in-core superblock has not been updated, this will result in detection of access beyond the end of the filesystem and so recovery will fail at that point. If this proves to be a problem, then we can address it separately to the current reported issue. Reported-by: Alex Lyakas <alex@zadarastorage.com> Tested-by: Alex Lyakas <alex@zadarastorage.com> Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_log_recover.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 1dc0e14..520471b 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -4898,6 +4898,7 @@ xlog_do_recover( xfs_daddr_t head_blk, xfs_daddr_t tail_blk) { + struct xfs_mount *mp = log->l_mp; int error; xfs_buf_t *bp; xfs_sb_t *sbp; @@ -4912,7 +4913,7 @@ xlog_do_recover( /* * If IO errors happened during recovery, bail out. */ - if (XFS_FORCED_SHUTDOWN(log->l_mp)) { + if (XFS_FORCED_SHUTDOWN(mp)) { return -EIO; } @@ -4925,13 +4926,13 @@ xlog_do_recover( * or iunlinks they will have some entries in the AIL; so we look at * the AIL to determine how to set the tail_lsn. */ - xlog_assign_tail_lsn(log->l_mp); + xlog_assign_tail_lsn(mp); /* * Now that we've finished replaying all buffer and inode * updates, re-read in the superblock and reverify it. */ - bp = xfs_getsb(log->l_mp, 0); + bp = xfs_getsb(mp, 0); bp->b_flags &= ~(XBF_DONE | XBF_ASYNC); ASSERT(!(bp->b_flags & XBF_WRITE)); bp->b_flags |= XBF_READ; @@ -4939,7 +4940,7 @@ xlog_do_recover( error = xfs_buf_submit_wait(bp); if (error) { - if (!XFS_FORCED_SHUTDOWN(log->l_mp)) { + if (!XFS_FORCED_SHUTDOWN(mp)) { xfs_buf_ioerror_alert(bp, __func__); ASSERT(0); } @@ -4948,14 +4949,17 @@ xlog_do_recover( } /* Convert superblock from on-disk format */ - sbp = &log->l_mp->m_sb; + sbp = &mp->m_sb; xfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp)); - ASSERT(sbp->sb_magicnum == XFS_SB_MAGIC); - ASSERT(xfs_sb_good_version(sbp)); - xfs_reinit_percpu_counters(log->l_mp); - xfs_buf_relse(bp); + /* re-initialise in-core superblock and geometry structures */ + xfs_reinit_percpu_counters(mp); + error = xfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi); + if (error) { + xfs_warn(mp, "Failed post-recovery per-ag init: %d", error); + return error; + } xlog_recover_check_summary(log); -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery 2016-03-06 21:49 ` [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery Dave Chinner @ 2016-03-07 16:24 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-07 16:24 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/6] xfs: ioends require logically contiguous file offsets 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner 2016-03-06 21:49 ` [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-07 16:26 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 3/6] xfs: fix computation of inode btree maxlevels Dave Chinner ` (3 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: "Darrick J. Wong" <darrick.wong@oracle.com> We need to create a new ioend if the current writepage call isn't logically contiguous with the range contained in the previous ioend. Hopefully writepage gets called in order of increasing file offset. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/xfs_aops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 7a467b3..75a39a8 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -526,7 +526,8 @@ xfs_add_to_ioend( struct list_head *iolist) { if (!wpc->ioend || wpc->io_type != wpc->ioend->io_type || - bh->b_blocknr != wpc->last_block + 1) { + bh->b_blocknr != wpc->last_block + 1 || + offset != wpc->ioend->io_offset + wpc->ioend->io_size) { struct xfs_ioend *new; if (wpc->ioend) -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] xfs: ioends require logically contiguous file offsets 2016-03-06 21:49 ` [PATCH 2/6] xfs: ioends require logically contiguous file offsets Dave Chinner @ 2016-03-07 16:26 ` Christoph Hellwig 2016-03-08 2:33 ` Darrick J. Wong 0 siblings, 1 reply; 17+ messages in thread From: Christoph Hellwig @ 2016-03-07 16:26 UTC (permalink / raw) To: Dave Chinner, darrick.wong; +Cc: xfs On Mon, Mar 07, 2016 at 08:49:46AM +1100, Dave Chinner wrote: > From: "Darrick J. Wong" <darrick.wong@oracle.com> > > We need to create a new ioend if the current writepage call isn't > logically contiguous with the range contained in the previous ioend. > Hopefully writepage gets called in order of increasing file offset. This looks reasonable, but how did we manage to get away without this for so long? I think as-is we do not actually require it - for setting i_size we just care about the highest offset, and for unwritten extent conversion we just need the lowest and highest offset, and we were making use of that fact in direct I/O extensively before I rewrote that code not to use ioends. So this looks fine to me, but the description could use some better wording. Signed-off-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] xfs: ioends require logically contiguous file offsets 2016-03-07 16:26 ` Christoph Hellwig @ 2016-03-08 2:33 ` Darrick J. Wong 2016-03-08 7:01 ` Christoph Hellwig 0 siblings, 1 reply; 17+ messages in thread From: Darrick J. Wong @ 2016-03-08 2:33 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Mon, Mar 07, 2016 at 08:26:44AM -0800, Christoph Hellwig wrote: > On Mon, Mar 07, 2016 at 08:49:46AM +1100, Dave Chinner wrote: > > From: "Darrick J. Wong" <darrick.wong@oracle.com> > > > > We need to create a new ioend if the current writepage call isn't > > logically contiguous with the range contained in the previous ioend. > > Hopefully writepage gets called in order of increasing file offset. > > This looks reasonable, but how did we manage to get away without this for > so long? I think as-is we do not actually require it - for setting > i_size we just care about the highest offset, and for unwritten extent > conversion we just need the lowest and highest offset, and we were > making use of that fact in direct I/O extensively before I rewrote that > code not to use ioends. The way I found this was by one of the cow tests failing (generic/139, I think?) -- if two non-adjacent file blocks were both CoW but mapped to adjacent physical blocks, the ioend check behaved as if the two file blocks were logically adjacent and combine them into one ioend. That sounds confusing even as I write it, so let's try an example: Say that /tmp/a block 10 and block 12 are both shared and dirty. Writepage comes along and allocates blocks 980 and 981 as replacements. This means that 10 -> 980 and 12 -> 981. The ioend combining code seems that 980 and 981 are adjacent and submits an ioend with offset 10 and length 2, instead of two ioends {10, 1} and {12, 1} like you'd expect, and as a result the CoW remapping is incorrect. I think we used to get away with this because _vm_writepage gets a mapping that only extends as far as the next io_type change, because XFS_IO_OVERWRITE and XFS_IO_UNWRITTEN only change at bmbt record boundaries and since for CoW we only deal with CoW fork bmbt records, that still holds true. Therefore, we performed only limited ioend chaining that ended at every extent boundary. Now that we collect the ioends in a struct writepage_ctx that can span several writepage calls, we can end up with ioends crossing bmbt record boundaries. --D > So this looks fine to me, but the description could use some better > wording. > > Signed-off-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/6] xfs: ioends require logically contiguous file offsets 2016-03-08 2:33 ` Darrick J. Wong @ 2016-03-08 7:01 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-08 7:01 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Christoph Hellwig, xfs Thanks Darrick, the explanation looks fine to me. I suspect it's still only really needed for COW, but together with my earlier direct I/O changes it makes the ioend rules sensible in general, so I'm all for it! _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/6] xfs: fix computation of inode btree maxlevels 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner 2016-03-06 21:49 ` [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery Dave Chinner 2016-03-06 21:49 ` [PATCH 2/6] xfs: ioends require logically contiguous file offsets Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-06 21:49 ` [PATCH 4/6] xfs: use named array initializers for log item dumping Dave Chinner ` (2 subsequent siblings) 5 siblings, 0 replies; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: "Darrick J. Wong" <darrick.wong@oracle.com> Commit 88740da18[1] introduced a function to compute the maximum height of the inode btree back in 1994. Back then, apparently, the freespace and inode btrees shared the same geometry; however, it has long since been the case that the inode and freespace btrees have different record and key sizes. Therefore, we must use m_inobt_mnr if we want a correct calculation/log reservation/etc. (Yes, this bug has been around for 21 years and ten months.) (Yes, I was in middle school when this bug was committed.) [1] http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=88740da18ddd9d7ba3ebaa9502fefc6ef2fd19cd Historical-research-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/libxfs/xfs_ialloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c index 66d702e..22297f9 100644 --- a/fs/xfs/libxfs/xfs_ialloc.c +++ b/fs/xfs/libxfs/xfs_ialloc.c @@ -2403,8 +2403,8 @@ xfs_ialloc_compute_maxlevels( maxleafents = (1LL << XFS_INO_AGINO_BITS(mp)) >> XFS_INODES_PER_CHUNK_LOG; - minleafrecs = mp->m_alloc_mnr[0]; - minnoderecs = mp->m_alloc_mnr[1]; + minleafrecs = mp->m_inobt_mnr[0]; + minnoderecs = mp->m_inobt_mnr[1]; maxblocks = (maxleafents + minleafrecs - 1) / minleafrecs; for (level = 1; maxblocks > 1; level++) maxblocks = (maxblocks + minnoderecs - 1) / minnoderecs; -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/6] xfs: use named array initializers for log item dumping 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner ` (2 preceding siblings ...) 2016-03-06 21:49 ` [PATCH 3/6] xfs: fix computation of inode btree maxlevels Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-07 16:27 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time Dave Chinner 2016-03-06 21:49 ` [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k Dave Chinner 5 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: "Darrick J. Wong" <darrick.wong@oracle.com> Use named array initializers for the string arrays used to dump log items, rather than depending on the order being maintained correctly. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/xfs_log.c | 132 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 68 insertions(+), 64 deletions(-) diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 40b700d..b49ccf5 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -2008,77 +2008,81 @@ xlog_print_tic_res( uint ophdr_spc = ticket->t_res_num_ophdrs * (uint)sizeof(xlog_op_header_t); /* match with XLOG_REG_TYPE_* in xfs_log.h */ - static char *res_type_str[XLOG_REG_TYPE_MAX] = { - "bformat", - "bchunk", - "efi_format", - "efd_format", - "iformat", - "icore", - "iext", - "ibroot", - "ilocal", - "iattr_ext", - "iattr_broot", - "iattr_local", - "qformat", - "dquot", - "quotaoff", - "LR header", - "unmount", - "commit", - "trans header" +#define REG_TYPE_STR(type, str) [XLOG_REG_TYPE_##type] = str + static char *res_type_str[XLOG_REG_TYPE_MAX + 1] = { + REG_TYPE_STR(BFORMAT, "bformat"), + REG_TYPE_STR(BCHUNK, "bchunk"), + REG_TYPE_STR(EFI_FORMAT, "efi_format"), + REG_TYPE_STR(EFD_FORMAT, "efd_format"), + REG_TYPE_STR(IFORMAT, "iformat"), + REG_TYPE_STR(ICORE, "icore"), + REG_TYPE_STR(IEXT, "iext"), + REG_TYPE_STR(IBROOT, "ibroot"), + REG_TYPE_STR(ILOCAL, "ilocal"), + REG_TYPE_STR(IATTR_EXT, "iattr_ext"), + REG_TYPE_STR(IATTR_BROOT, "iattr_broot"), + REG_TYPE_STR(IATTR_LOCAL, "iattr_local"), + REG_TYPE_STR(QFORMAT, "qformat"), + REG_TYPE_STR(DQUOT, "dquot"), + REG_TYPE_STR(QUOTAOFF, "quotaoff"), + REG_TYPE_STR(LRHEADER, "LR header"), + REG_TYPE_STR(UNMOUNT, "unmount"), + REG_TYPE_STR(COMMIT, "commit"), + REG_TYPE_STR(TRANSHDR, "trans header"), + REG_TYPE_STR(ICREATE, "inode create") }; +#undef REG_TYPE_STR +#define TRANS_TYPE_STR(type) [XFS_TRANS_##type] = #type static char *trans_type_str[XFS_TRANS_TYPE_MAX] = { - "SETATTR_NOT_SIZE", - "SETATTR_SIZE", - "INACTIVE", - "CREATE", - "CREATE_TRUNC", - "TRUNCATE_FILE", - "REMOVE", - "LINK", - "RENAME", - "MKDIR", - "RMDIR", - "SYMLINK", - "SET_DMATTRS", - "GROWFS", - "STRAT_WRITE", - "DIOSTRAT", - "WRITE_SYNC", - "WRITEID", - "ADDAFORK", - "ATTRINVAL", - "ATRUNCATE", - "ATTR_SET", - "ATTR_RM", - "ATTR_FLAG", - "CLEAR_AGI_BUCKET", - "QM_SBCHANGE", - "DUMMY1", - "DUMMY2", - "QM_QUOTAOFF", - "QM_DQALLOC", - "QM_SETQLIM", - "QM_DQCLUSTER", - "QM_QINOCREATE", - "QM_QUOTAOFF_END", - "FSYNC_TS", - "GROWFSRT_ALLOC", - "GROWFSRT_ZERO", - "GROWFSRT_FREE", - "SWAPEXT", - "CHECKPOINT", - "ICREATE", - "CREATE_TMPFILE" + TRANS_TYPE_STR(SETATTR_NOT_SIZE), + TRANS_TYPE_STR(SETATTR_SIZE), + TRANS_TYPE_STR(INACTIVE), + TRANS_TYPE_STR(CREATE), + TRANS_TYPE_STR(CREATE_TRUNC), + TRANS_TYPE_STR(TRUNCATE_FILE), + TRANS_TYPE_STR(REMOVE), + TRANS_TYPE_STR(LINK), + TRANS_TYPE_STR(RENAME), + TRANS_TYPE_STR(MKDIR), + TRANS_TYPE_STR(RMDIR), + TRANS_TYPE_STR(SYMLINK), + TRANS_TYPE_STR(SET_DMATTRS), + TRANS_TYPE_STR(GROWFS), + TRANS_TYPE_STR(STRAT_WRITE), + TRANS_TYPE_STR(DIOSTRAT), + TRANS_TYPE_STR(WRITEID), + TRANS_TYPE_STR(ADDAFORK), + TRANS_TYPE_STR(ATTRINVAL), + TRANS_TYPE_STR(ATRUNCATE), + TRANS_TYPE_STR(ATTR_SET), + TRANS_TYPE_STR(ATTR_RM), + TRANS_TYPE_STR(ATTR_FLAG), + TRANS_TYPE_STR(CLEAR_AGI_BUCKET), + TRANS_TYPE_STR(SB_CHANGE), + TRANS_TYPE_STR(DUMMY1), + TRANS_TYPE_STR(DUMMY2), + TRANS_TYPE_STR(QM_QUOTAOFF), + TRANS_TYPE_STR(QM_DQALLOC), + TRANS_TYPE_STR(QM_SETQLIM), + TRANS_TYPE_STR(QM_DQCLUSTER), + TRANS_TYPE_STR(QM_QINOCREATE), + TRANS_TYPE_STR(QM_QUOTAOFF_END), + TRANS_TYPE_STR(FSYNC_TS), + TRANS_TYPE_STR(GROWFSRT_ALLOC), + TRANS_TYPE_STR(GROWFSRT_ZERO), + TRANS_TYPE_STR(GROWFSRT_FREE), + TRANS_TYPE_STR(SWAPEXT), + TRANS_TYPE_STR(CHECKPOINT), + TRANS_TYPE_STR(ICREATE), + TRANS_TYPE_STR(CREATE_TMPFILE) }; +#undef TRANS_TYPE_STR xfs_warn(mp, "xlog_write: reservation summary:"); xfs_warn(mp, " trans type = %s (%u)", ((ticket->t_trans_type <= 0 || ticket->t_trans_type > XFS_TRANS_TYPE_MAX) ? - "bad-trans-type" : trans_type_str[ticket->t_trans_type-1]), + "bad-trans-type" : trans_type_str[ticket->t_trans_type]), ticket->t_trans_type); xfs_warn(mp, " unit res = %d bytes", ticket->t_unit_res); @@ -2097,7 +2101,7 @@ xlog_print_tic_res( uint r_type = ticket->t_res_arr[i].r_type; xfs_warn(mp, "region[%u]: %s - %u bytes", i, ((r_type <= 0 || r_type > XLOG_REG_TYPE_MAX) ? - "bad-rtype" : res_type_str[r_type-1]), + "bad-rtype" : res_type_str[r_type]), ticket->t_res_arr[i].r_len); } -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/6] xfs: use named array initializers for log item dumping 2016-03-06 21:49 ` [PATCH 4/6] xfs: use named array initializers for log item dumping Dave Chinner @ 2016-03-07 16:27 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-07 16:27 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner ` (3 preceding siblings ...) 2016-03-06 21:49 ` [PATCH 4/6] xfs: use named array initializers for log item dumping Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-07 16:27 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k Dave Chinner 5 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: "Darrick J. Wong" <darrick.wong@oracle.com> Check the sizes of XFS on-disk structures when compiling the kernel. Use this to catch inadvertent changes in structure size due to padding and alignment issues, etc. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Brian Foster <bfoster@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/xfs_ondisk.h | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_super.c | 3 ++ 2 files changed, 111 insertions(+) create mode 100644 fs/xfs/xfs_ondisk.h diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h new file mode 100644 index 0000000..9a78408 --- /dev/null +++ b/fs/xfs/xfs_ondisk.h @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2016 Oracle. + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ +#ifndef __XFS_ONDISK_H +#define __XFS_ONDISK_H + +#define XFS_CHECK_STRUCT_SIZE(structname, size) \ + BUILD_BUG_ON_MSG(sizeof(structname) != (size), "XFS: sizeof(" \ + #structname ") is wrong, expected " #size) + +static inline void __init +xfs_check_ondisk_structs(void) +{ + /* ag/file structures */ + XFS_CHECK_STRUCT_SIZE(struct xfs_acl, 4); + XFS_CHECK_STRUCT_SIZE(struct xfs_acl_entry, 12); + XFS_CHECK_STRUCT_SIZE(struct xfs_agf, 224); + XFS_CHECK_STRUCT_SIZE(struct xfs_agfl, 36); + XFS_CHECK_STRUCT_SIZE(struct xfs_agi, 336); + XFS_CHECK_STRUCT_SIZE(struct xfs_bmbt_key, 8); + XFS_CHECK_STRUCT_SIZE(struct xfs_bmbt_rec, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_bmdr_block, 4); + XFS_CHECK_STRUCT_SIZE(struct xfs_btree_block, 72); + XFS_CHECK_STRUCT_SIZE(struct xfs_dinode, 176); + XFS_CHECK_STRUCT_SIZE(struct xfs_disk_dquot, 104); + XFS_CHECK_STRUCT_SIZE(struct xfs_dqblk, 136); + XFS_CHECK_STRUCT_SIZE(struct xfs_dsb, 264); + XFS_CHECK_STRUCT_SIZE(struct xfs_dsymlink_hdr, 56); + XFS_CHECK_STRUCT_SIZE(struct xfs_inobt_key, 4); + XFS_CHECK_STRUCT_SIZE(struct xfs_inobt_rec, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_timestamp, 8); + XFS_CHECK_STRUCT_SIZE(xfs_alloc_key_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_alloc_ptr_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_alloc_rec_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_inobt_ptr_t, 4); + + /* dir/attr trees */ + XFS_CHECK_STRUCT_SIZE(struct xfs_attr3_leaf_hdr, 80); + XFS_CHECK_STRUCT_SIZE(struct xfs_attr3_leafblock, 88); + XFS_CHECK_STRUCT_SIZE(struct xfs_attr3_rmt_hdr, 56); + XFS_CHECK_STRUCT_SIZE(struct xfs_da3_blkinfo, 56); + XFS_CHECK_STRUCT_SIZE(struct xfs_da3_intnode, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_da3_node_hdr, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_blk_hdr, 48); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_data_hdr, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_free, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_free_hdr, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_leaf, 64); + XFS_CHECK_STRUCT_SIZE(struct xfs_dir3_leaf_hdr, 64); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_entry_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_hdr_t, 32); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_map_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_name_local_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leaf_name_remote_t, 12); + XFS_CHECK_STRUCT_SIZE(xfs_attr_leafblock_t, 40); + XFS_CHECK_STRUCT_SIZE(xfs_attr_shortform_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_da_blkinfo_t, 12); + XFS_CHECK_STRUCT_SIZE(xfs_da_intnode_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_da_node_entry_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_da_node_hdr_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_data_free_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_data_hdr_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_data_unused_t, 6); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_free_hdr_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_free_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_ino4_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_ino8_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_inou_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_entry_t, 8); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_hdr_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_t, 16); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_leaf_tail_t, 4); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_entry_t, 3); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_hdr_t, 10); + XFS_CHECK_STRUCT_SIZE(xfs_dir2_sf_off_t, 2); + + /* log structures */ + XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat, 24); + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32, 28); + XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64, 32); + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32, 28); + XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64, 32); + XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32, 12); + XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64, 16); + XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode, 176); + XFS_CHECK_STRUCT_SIZE(struct xfs_icreate_log, 28); + XFS_CHECK_STRUCT_SIZE(struct xfs_ictimestamp, 8); + XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format_32, 52); + XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format_64, 56); + XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat, 20); + XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header, 16); +} + +#endif /* __XFS_ONDISK_H */ diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 59c9b7b..3bd2027 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -45,6 +45,7 @@ #include "xfs_filestream.h" #include "xfs_quota.h" #include "xfs_sysfs.h" +#include "xfs_ondisk.h" #include <linux/namei.h> #include <linux/init.h> @@ -1817,6 +1818,8 @@ init_xfs_fs(void) { int error; + xfs_check_ondisk_structs(); + printk(KERN_INFO XFS_VERSION_STRING " with " XFS_BUILD_OPTIONS " enabled\n"); -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time 2016-03-06 21:49 ` [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time Dave Chinner @ 2016-03-07 16:27 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-07 16:27 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs Looks okay-ish. My minor complain will be in the next patch instead.. Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner ` (4 preceding siblings ...) 2016-03-06 21:49 ` [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time Dave Chinner @ 2016-03-06 21:49 ` Dave Chinner 2016-03-07 16:28 ` Christoph Hellwig 5 siblings, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-06 21:49 UTC (permalink / raw) To: xfs From: "Darrick J. Wong" <darrick.wong@oracle.com> Pad the xfs_attr_leaf_name_remote so that we don't trip the structure size checker on m68k. [dchinner: add comment, XFS_ATTR_LEAF_NAME_BYTES constant and make sure xfs_attr_leaf_entsize_remote() does the right thing. ] Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/libxfs/xfs_da_format.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h index 8d4d8bc..b6d58f6 100644 --- a/fs/xfs/libxfs/xfs_da_format.h +++ b/fs/xfs/libxfs/xfs_da_format.h @@ -686,11 +686,19 @@ typedef struct xfs_attr_leaf_name_local { __u8 nameval[1]; /* name/value bytes */ } xfs_attr_leaf_name_local_t; +/* + * some platforms (like m68k) don't like non-word sized structures and pad them + * out inappropriately. To work around this, we set the name array to the size + * that pads out to a 4 byte word size. We need to take this away from the size + * of the structure when calculating the length of the entry in + * xfs_attr_leaf_entsize_remote(), hence we define a constant for this value. + */ +#define XFS_ATTR_LEAF_NAME_BYTES 3 typedef struct xfs_attr_leaf_name_remote { __be32 valueblk; /* block number of value bytes */ __be32 valuelen; /* number of bytes in value */ __u8 namelen; /* length of name bytes */ - __u8 name[1]; /* name bytes */ + __u8 name[XFS_ATTR_LEAF_NAME_BYTES]; /* name bytes */ } xfs_attr_leaf_name_remote_t; typedef struct xfs_attr_leafblock { @@ -847,8 +855,10 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx) */ static inline int xfs_attr_leaf_entsize_remote(int nlen) { - return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) + \ - XFS_ATTR_LEAF_NAME_ALIGN - 1) & ~(XFS_ATTR_LEAF_NAME_ALIGN - 1); + return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - + XFS_ATTR_LEAF_NAME_BYTES + nlen + + XFS_ATTR_LEAF_NAME_ALIGN - 1) & + ~(XFS_ATTR_LEAF_NAME_ALIGN - 1); } static inline int xfs_attr_leaf_entsize_local(int nlen, int vlen) -- 2.7.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k 2016-03-06 21:49 ` [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k Dave Chinner @ 2016-03-07 16:28 ` Christoph Hellwig 2016-03-08 3:56 ` Dave Chinner 2016-03-08 17:34 ` Christoph Hellwig 0 siblings, 2 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-07 16:28 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On Mon, Mar 07, 2016 at 08:49:50AM +1100, Dave Chinner wrote: > From: "Darrick J. Wong" <darrick.wong@oracle.com> > > Pad the xfs_attr_leaf_name_remote so that we don't trip the structure > size checker on m68k. > > [dchinner: add comment, XFS_ATTR_LEAF_NAME_BYTES constant and make sure > xfs_attr_leaf_entsize_remote() does the right thing. ] I think using a small fixed size array as a variable sized array is not a good idea, especially with increasinly "smart" optimizing compilers. I'd rather take this structure out the size checking, and then move it to a C99 VLA instead of the size 1 hack in the long run. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k 2016-03-07 16:28 ` Christoph Hellwig @ 2016-03-08 3:56 ` Dave Chinner 2016-03-08 7:02 ` Christoph Hellwig 2016-03-08 17:34 ` Christoph Hellwig 1 sibling, 1 reply; 17+ messages in thread From: Dave Chinner @ 2016-03-08 3:56 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Mon, Mar 07, 2016 at 08:28:58AM -0800, Christoph Hellwig wrote: > On Mon, Mar 07, 2016 at 08:49:50AM +1100, Dave Chinner wrote: > > From: "Darrick J. Wong" <darrick.wong@oracle.com> > > > > Pad the xfs_attr_leaf_name_remote so that we don't trip the structure > > size checker on m68k. > > > > [dchinner: add comment, XFS_ATTR_LEAF_NAME_BYTES constant and make sure > > xfs_attr_leaf_entsize_remote() does the right thing. ] > > I think using a small fixed size array as a variable sized array > is not a good idea, especially with increasinly "smart" optimizing > compilers. I'd rather take this structure out the size checking, > and then move it to a C99 VLA instead of the size 1 hack in the long > run. I don't have the time right now to do this, so I'm just going to drop it - I'd guess the overlap between m68k and XFS users is so close to zero that it just doesn't matter. I'll just drop this patch for now. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k 2016-03-08 3:56 ` Dave Chinner @ 2016-03-08 7:02 ` Christoph Hellwig 0 siblings, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-08 7:02 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs On Tue, Mar 08, 2016 at 02:56:48PM +1100, Dave Chinner wrote: > > I think using a small fixed size array as a variable sized array > > is not a good idea, especially with increasinly "smart" optimizing > > compilers. I'd rather take this structure out the size checking, > > and then move it to a C99 VLA instead of the size 1 hack in the long > > run. > > I don't have the time right now to do this, so I'm just going to > drop it - I'd guess the overlap between m68k and XFS users is so > close to zero that it just doesn't matter. I'll just drop this > patch for now. I'll send you a patch for it. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k 2016-03-07 16:28 ` Christoph Hellwig 2016-03-08 3:56 ` Dave Chinner @ 2016-03-08 17:34 ` Christoph Hellwig 1 sibling, 0 replies; 17+ messages in thread From: Christoph Hellwig @ 2016-03-08 17:34 UTC (permalink / raw) To: Dave Chinner, darrick.wong; +Cc: xfs So I looked into this, and it seems we really don't care about the size - xfs_attr_leaf_entsize_remote pad it to the next multiple of 4 anyway. So I think we really should simply remove the size check here. Assuming all architectures pad up a structure that isn't word aligned the same way just isn't a sensible assumption. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-03-08 17:34 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-06 21:49 [PATCH 0/6] xfs: accumulated fixes for 4.6 Dave Chinner 2016-03-06 21:49 ` [PATCH 1/6] xfs: reinitialise per-AG structures if geometry changes during recovery Dave Chinner 2016-03-07 16:24 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 2/6] xfs: ioends require logically contiguous file offsets Dave Chinner 2016-03-07 16:26 ` Christoph Hellwig 2016-03-08 2:33 ` Darrick J. Wong 2016-03-08 7:01 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 3/6] xfs: fix computation of inode btree maxlevels Dave Chinner 2016-03-06 21:49 ` [PATCH 4/6] xfs: use named array initializers for log item dumping Dave Chinner 2016-03-07 16:27 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 5/6] xfs: check sizes of XFS on-disk structures at compile time Dave Chinner 2016-03-07 16:27 ` Christoph Hellwig 2016-03-06 21:49 ` [PATCH 6/6] xfs: pad xfs_attr_leaf_name_remote to avoid tripping on m68k Dave Chinner 2016-03-07 16:28 ` Christoph Hellwig 2016-03-08 3:56 ` Dave Chinner 2016-03-08 7:02 ` Christoph Hellwig 2016-03-08 17:34 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox