[PATCH 0/2] xfsprogs/db: fix up broken multi-record inode chunk support

[PATCH 0/2] xfsprogs/db: fix up broken multi-record inode chunk support

[Brian Foster]

xfs_db/xfs_check doesn't currently handle filesystems with multi-record
inode chunks correctly. For example, do the following on a 64k page size
arch such as ppc64:

# mkfs.xfs -f -b size=64k <dev>
# xfs_db -c check <dev>
bad magic number 0 for inode 1152
bad magic number 0 for inode 1153
bad magic number 0 for inode 1154
bad magic number 0 for inode 1155
bad magic number 0 for inode 1156
bad magic number 0 for inode 1157
...

This boils down to a regression in the inode record processing code
(scanfunc_ino()) in db/check.c. Specifically, the cblocks value can end
up being zero after it is shifted by mp->m_sb.sb_inopblog (i.e., 64 >> 7
== 0 for an -isize=512 -bsize=64k fs).

The xfs_check sparse inode processing code takes a unique approach from
similar code in other areas such as metadump and repair. This was
sufficiently confusing for me that I decided to start with a revert of
the patch that introduced the regression and follow up with a patch that
updates the inode record processing code to take a similar approach as
used in metadump, for example. This approach processes inode chunks a
cluster at a time on sparse inode enabled filesystems and skips the
regions that are sparse according to the record holemask. The two
patches could probably be squashed into one if that is desired, but I'm
posting separately as it describes my workflow.

This survives my testing thus far on sparse inode filesystems as well as
multi-record chunk configurations. These multi-rec configurations don't
exactly have a high test pass rate ;P but I haven't hit any regressions.
BTW, I have noticed that xfs_check seems to also have memory issues with
quotas on multi-inode-rec fs', but that seems to go much farther back
and may just be a limitation of check.

Brian

Brian Foster (2):
  Revert "xfs_db: make check work for sparse inodes"
  xfs_check: process sparse inode chunks correctly

 db/check.c | 274 +++++++++++++++++++++----------------------------------------
 1 file changed, 92 insertions(+), 182 deletions(-)

-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] Revert "xfs_db: make check work for sparse inodes"

[Brian Foster]

This reverts commit bb2f98b78f20f4abbfbbd442162d9f535c84888a.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 db/check.c | 249 ++++++++++++-------------------------------------------------
 1 file changed, 49 insertions(+), 200 deletions(-)

diff --git a/db/check.c b/db/check.c
index 0871ed7..750ecc1 100644
--- a/db/check.c
+++ b/db/check.c
@@ -4311,51 +4311,6 @@ scanfunc_cnt(
 		scan_sbtree(agf, be32_to_cpu(pp[i]), level, 0, scanfunc_cnt, TYP_CNTBT);
 }
 
-static bool
-ino_issparse(
-	struct xfs_inobt_rec	*rp,
-	int			offset)
-{
-	if (!xfs_sb_version_hassparseinodes(&mp->m_sb))
-		return false;
-
-	return xfs_inobt_is_sparse_disk(rp, offset);
-}
-
-static int
-find_one_ino_bit(
-	__u16		mask,
-	int		startino)
-{
-	int		n;
-	int		b;
-
-	startino /= XFS_INODES_PER_HOLEMASK_BIT;
-	b = startino;
-	mask >>= startino;
-	for (n = startino; n < sizeof(mask) * NBBY && !(mask & 1); n++, mask >>= 1)
-		b++;
-
-	return b * XFS_INODES_PER_HOLEMASK_BIT;
-}
-
-static int
-find_zero_ino_bit(
-	__u16		mask,
-	int		startino)
-{
-	int		n;
-	int		b;
-
-	startino /= XFS_INODES_PER_HOLEMASK_BIT;
-	b = startino;
-	mask >>= startino;
-	for (n = startino; n < sizeof(mask) * NBBY && (mask & 1); n++, mask >>= 1)
-		b++;
-
-	return b * XFS_INODES_PER_HOLEMASK_BIT;
-}
-
 static void
 scanfunc_ino(
 	struct xfs_btree_block	*block,
@@ -4373,13 +4328,6 @@ scanfunc_ino(
 	int			off;
 	xfs_inobt_ptr_t		*pp;
 	xfs_inobt_rec_t		*rp;
-	bool			sparse, crc;
-	int			inodes_per_chunk;
-	int			freecount;
-	int			startidx, endidx;
-	__u16			holemask;
-	xfs_agino_t		rino;
-	xfs_extlen_t		cblocks;
 
 	if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
 	    be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
@@ -4407,111 +4355,59 @@ scanfunc_ino(
 			return;
 		}
 		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
-		sparse = xfs_sb_version_hassparseinodes(&mp->m_sb);
-		crc = xfs_sb_version_hascrc(&mp->m_sb);
 		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
-			nfree = 0;
-
-			/* First let's look at the inode chunk alignment */
 			agino = be32_to_cpu(rp[i].ir_startino);
 			off = XFS_INO_TO_OFFSET(mp, agino);
-			if (off == 0 &&
-			    (sbversion & XFS_SB_VERSION_ALIGNBIT) &&
-			    mp->m_sb.sb_inoalignmt &&
-			    (XFS_INO_TO_AGBNO(mp, agino) %
-			     mp->m_sb.sb_inoalignmt)) {
-				if (sparse || crc) {
-					dbprintf(_("incorrect record %u/%u "
-						 "alignment in inobt block "
-						 "%u/%u\n"),
-						 seqno, agino, seqno, bno);
-					error++;
-				} else
+			if (off == 0) {
+				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
+				    mp->m_sb.sb_inoalignmt &&
+				    (XFS_INO_TO_AGBNO(mp, agino) %
+				     mp->m_sb.sb_inoalignmt))
 					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
+				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
+					(xfs_extlen_t)MAX(1,
+						XFS_INODES_PER_CHUNK >>
+						mp->m_sb.sb_inopblog),
+					DBM_INODE, seqno, bno);
 			}
-
-			/* Move on to examining the inode chunks */
-			if (sparse) {
-				inodes_per_chunk = rp[i].ir_u.sp.ir_count;
-				freecount = rp[i].ir_u.sp.ir_freecount;
-				holemask = be16_to_cpu(rp[i].ir_u.sp.ir_holemask);
-				startidx = find_zero_ino_bit(holemask, 0);
-			} else {
-				inodes_per_chunk = XFS_INODES_PER_CHUNK;
-				freecount = be32_to_cpu(rp[i].ir_u.f.ir_freecount);
-				holemask = 0;
-				startidx = 0;
-			}
-
-			/* For each allocated chunk, look at each inode. */
-			endidx = find_one_ino_bit(holemask, startidx);
-			do {
-				rino = agino + startidx;
-				cblocks = (endidx - startidx) >>
-						mp->m_sb.sb_inopblog;
-
-				/* Check the sparse chunk alignment */
-				if (sparse &&
-				    (XFS_INO_TO_AGBNO(mp, rino) %
-				     mp->m_sb.sb_spino_align)) {
-					dbprintf(_("incorrect chunk %u/%u "
-						 "alignment in inobt block "
+			icount += XFS_INODES_PER_CHUNK;
+			agicount += XFS_INODES_PER_CHUNK;
+			ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
+			agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
+			push_cur();
+			set_cur(&typtab[TYP_INODE],
+				XFS_AGB_TO_DADDR(mp, seqno,
+						 XFS_AGINO_TO_AGBNO(mp, agino)),
+				(int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
+				DB_RING_IGN, NULL);
+			if (iocur_top->data == NULL) {
+				if (!sflag)
+					dbprintf(_("can't read inode block "
 						 "%u/%u\n"),
-						 seqno, rino, seqno, bno);
-					error++;
-				}
-
-				/* Check the block map */
-				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, rino),
-					cblocks, DBM_INODE, seqno, bno);
-
-				push_cur();
-				set_cur(&typtab[TYP_INODE],
-					XFS_AGB_TO_DADDR(mp, seqno,
-							 XFS_AGINO_TO_AGBNO(mp, rino)),
-					(int)XFS_FSB_TO_BB(mp, cblocks),
-					DB_RING_IGN, NULL);
-				if (iocur_top->data == NULL) {
-					if (!sflag)
-						dbprintf(_("can't read inode block "
-							 "%u/%u\n"),
-							seqno,
-							XFS_AGINO_TO_AGBNO(mp, agino));
-					error++;
-					pop_cur();
-					continue;
-				}
-
-				/* Examine each inode in this chunk */
-				for (j = startidx; j < endidx; j++) {
-					if (ino_issparse(&rp[i], j))
-						continue;
-					isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
-					if (isfree)
-						nfree++;
-					process_inode(agf, agino + j,
-						(xfs_dinode_t *)((char *)iocur_top->data + ((j - startidx) << mp->m_sb.sb_inodelog)),
-							isfree);
-				}
+						seqno,
+						XFS_AGINO_TO_AGBNO(mp, agino));
+				error++;
 				pop_cur();
-
-				startidx = find_zero_ino_bit(holemask, endidx);
-				endidx = find_one_ino_bit(holemask, startidx);
-			} while (endidx < XFS_INODES_PER_CHUNK);
-			icount += inodes_per_chunk;
-			agicount += inodes_per_chunk;
-			ifree += freecount;
-			agifreecount += freecount;
-
-			if (nfree != freecount) {
+				continue;
+			}
+			for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
+				isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
+				if (isfree)
+					nfree++;
+				process_inode(agf, agino + j,
+					(xfs_dinode_t *)((char *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
+						isfree);
+			}
+			if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
 				if (!sflag)
 					dbprintf(_("ir_freecount/free mismatch, "
 						 "inode chunk %u/%u, freecount "
 						 "%d nfree %d\n"),
 						seqno, agino,
-						freecount, nfree);
+						be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
 				error++;
 			}
+			pop_cur();
 		}
 		return;
 	}
@@ -4543,11 +4439,6 @@ scanfunc_fino(
 	int			off;
 	xfs_inobt_ptr_t		*pp;
 	struct xfs_inobt_rec	*rp;
-	bool			sparse, crc;
-	int			startidx, endidx;
-	__u16			holemask;
-	xfs_agino_t		rino;
-	xfs_extlen_t		cblocks;
 
 	if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
 	    be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
@@ -4575,63 +4466,21 @@ scanfunc_fino(
 			return;
 		}
 		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
-		sparse = xfs_sb_version_hassparseinodes(&mp->m_sb);
-		crc = xfs_sb_version_hascrc(&mp->m_sb);
 		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
-			/* First let's look at the inode chunk alignment */
 			agino = be32_to_cpu(rp[i].ir_startino);
 			off = XFS_INO_TO_OFFSET(mp, agino);
-			if (off == 0 &&
-			    (sbversion & XFS_SB_VERSION_ALIGNBIT) &&
-			    mp->m_sb.sb_inoalignmt &&
-			    (XFS_INO_TO_AGBNO(mp, agino) %
-			     mp->m_sb.sb_inoalignmt)) {
-				if (sparse || crc) {
-					dbprintf(_("incorrect record %u/%u "
-						 "alignment in finobt block "
-						 "%u/%u\n"),
-						 seqno, agino, seqno, bno);
-					error++;
-				} else
+			if (off == 0) {
+				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
+				    mp->m_sb.sb_inoalignmt &&
+				    (XFS_INO_TO_AGBNO(mp, agino) %
+				     mp->m_sb.sb_inoalignmt))
 					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
+				check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
+					(xfs_extlen_t)MAX(1,
+						XFS_INODES_PER_CHUNK >>
+						mp->m_sb.sb_inopblog),
+					DBM_INODE, DBM_INODE, seqno, bno);
 			}
-
-			/* Move on to examining the inode chunks */
-			if (sparse) {
-				holemask = be16_to_cpu(rp[i].ir_u.sp.ir_holemask);
-				startidx = find_zero_ino_bit(holemask, 0);
-			} else {
-				holemask = 0;
-				startidx = 0;
-			}
-
-			/* For each allocated chunk... */
-			endidx = find_one_ino_bit(holemask, startidx);
-			do {
-				rino = agino + startidx;
-				cblocks = (endidx - startidx) >>
-						mp->m_sb.sb_inopblog;
-
-				/* Check the sparse chunk alignment */
-				if (sparse &&
-				    (XFS_INO_TO_AGBNO(mp, rino) %
-				     mp->m_sb.sb_spino_align)) {
-					dbprintf(_("incorrect chunk %u/%u "
-						 "alignment in finobt block "
-						 "%u/%u\n"),
-						 seqno, rino, seqno, bno);
-					error++;
-				}
-
-				/* Check the block map */
-				check_set_dbmap(seqno,
-					XFS_AGINO_TO_AGBNO(mp, rino),
-					cblocks, DBM_INODE, DBM_INODE,
-					seqno, bno);
-
-				startidx = find_zero_ino_bit(holemask, endidx);
-				endidx = find_one_ino_bit(holemask, startidx);
-			} while (endidx < XFS_INODES_PER_CHUNK);
 		}
 		return;
 	}
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Brian Foster]

Update the inode btree scanning functions to process sparse inode chunks
correctly. For filesystems with sparse inode support enabled, process
each chunk a cluster at a time. Each cluster is checked against the
inobt record to determine if it is a hole and skipped if so.

Note that since xfs_check is deprecated in favor of xfs_repair,  this
adds the minimum support necessary to process sparse inode enabled
filesystems. In other words, this adds no sparse inode specific checks
or verifications. We only update the inobt scanning functions to extend
the existing level of verification to sparse inode enabled filesystems
(e.g., avoid incorrectly tracking sparse regions as inodes). Problems
or corruptions associated with sparse inode records must be detected and
recovered via xfs_repair.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 db/check.c | 143 +++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 102 insertions(+), 41 deletions(-)

diff --git a/db/check.c b/db/check.c
index 750ecc1..25146e5 100644
--- a/db/check.c
+++ b/db/check.c
@@ -4324,10 +4324,24 @@ scanfunc_ino(
 	int			i;
 	int			isfree;
 	int			j;
+	int			freecount;
 	int			nfree;
 	int			off;
 	xfs_inobt_ptr_t		*pp;
 	xfs_inobt_rec_t		*rp;
+	xfs_agblock_t		agbno;
+	xfs_agblock_t		end_agbno;
+	struct xfs_dinode	*dip;
+	int			blks_per_buf;
+	int			inodes_per_buf;
+	int			ioff;
+
+	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
+		blks_per_buf = xfs_icluster_size_fsb(mp);
+	else
+		blks_per_buf = mp->m_ialloc_blks;
+	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
+			     XFS_INODES_PER_CHUNK);
 
 	if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
 	    be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
@@ -4357,54 +4371,74 @@ scanfunc_ino(
 		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
 		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
 			agino = be32_to_cpu(rp[i].ir_startino);
-			off = XFS_INO_TO_OFFSET(mp, agino);
+			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
+			off = XFS_AGINO_TO_OFFSET(mp, agino);
+			end_agbno = agbno + mp->m_ialloc_blks;
 			if (off == 0) {
 				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
 				    mp->m_sb.sb_inoalignmt &&
 				    (XFS_INO_TO_AGBNO(mp, agino) %
 				     mp->m_sb.sb_inoalignmt))
 					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
-				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
-					(xfs_extlen_t)MAX(1,
-						XFS_INODES_PER_CHUNK >>
-						mp->m_sb.sb_inopblog),
-					DBM_INODE, seqno, bno);
 			}
-			icount += XFS_INODES_PER_CHUNK;
-			agicount += XFS_INODES_PER_CHUNK;
-			ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
-			agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
+
 			push_cur();
-			set_cur(&typtab[TYP_INODE],
-				XFS_AGB_TO_DADDR(mp, seqno,
-						 XFS_AGINO_TO_AGBNO(mp, agino)),
-				(int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
-				DB_RING_IGN, NULL);
-			if (iocur_top->data == NULL) {
-				if (!sflag)
-					dbprintf(_("can't read inode block "
-						 "%u/%u\n"),
-						seqno,
-						XFS_AGINO_TO_AGBNO(mp, agino));
-				error++;
-				pop_cur();
-				continue;
-			}
-			for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
-				isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
-				if (isfree)
-					nfree++;
-				process_inode(agf, agino + j,
-					(xfs_dinode_t *)((char *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
-						isfree);
+
+			ioff = 0;
+			nfree = 0;
+			while (agbno < end_agbno &&
+			       ioff < XFS_INODES_PER_CHUNK) {
+				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
+					goto next_buf;
+
+				if (off < XFS_INODES_PER_CHUNK)
+					set_dbmap(seqno, agbno, blks_per_buf,
+						  DBM_INODE, seqno, bno);
+
+				icount += inodes_per_buf;
+				agicount += inodes_per_buf;
+
+				set_cur(&typtab[TYP_INODE],
+					XFS_AGB_TO_DADDR(mp, seqno, agbno),
+					XFS_FSB_TO_BB(mp, blks_per_buf),
+					DB_RING_IGN, NULL);
+				if (iocur_top->data == NULL) {
+					if (!sflag)
+						dbprintf(_("can't read inode block "
+							   "%u/%u\n"), seqno,
+							 agbno);
+					error++;
+					goto next_buf;
+				}
+
+				for (j = 0; j < inodes_per_buf; j++) {
+					isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], ioff + j);
+					if (isfree)
+						nfree++;
+					dip = (xfs_dinode_t *)((char *)iocur_top->data +
+						((off + j) << mp->m_sb.sb_inodelog));
+					process_inode(agf, agino + ioff + j, dip, isfree);
+				}
+
+next_buf:
+				agbno += blks_per_buf;
+				ioff += inodes_per_buf;
 			}
-			if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
+
+			if (xfs_sb_version_hassparseinodes(&mp->m_sb))
+				freecount = rp[i].ir_u.sp.ir_freecount;
+			else
+				freecount = be32_to_cpu(rp[i].ir_u.f.ir_freecount);
+
+			ifree += freecount;
+			agifreecount += freecount;
+
+			if (nfree != freecount) {
 				if (!sflag)
 					dbprintf(_("ir_freecount/free mismatch, "
 						 "inode chunk %u/%u, freecount "
 						 "%d nfree %d\n"),
-						seqno, agino,
-						be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
+						seqno, agino, freecount, nfree);
 				error++;
 			}
 			pop_cur();
@@ -4439,6 +4473,18 @@ scanfunc_fino(
 	int			off;
 	xfs_inobt_ptr_t		*pp;
 	struct xfs_inobt_rec	*rp;
+	xfs_agblock_t		agbno;
+	xfs_agblock_t		end_agbno;
+	int			blks_per_buf;
+	int			inodes_per_buf;
+	int			ioff;
+
+	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
+		blks_per_buf = xfs_icluster_size_fsb(mp);
+	else
+		blks_per_buf = mp->m_ialloc_blks;
+	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
+			     XFS_INODES_PER_CHUNK);
 
 	if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
 	    be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
@@ -4468,19 +4514,34 @@ scanfunc_fino(
 		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
 		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
 			agino = be32_to_cpu(rp[i].ir_startino);
-			off = XFS_INO_TO_OFFSET(mp, agino);
+			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
+			off = XFS_AGINO_TO_OFFSET(mp, agino);
+			end_agbno = agbno + mp->m_ialloc_blks;
 			if (off == 0) {
 				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
 				    mp->m_sb.sb_inoalignmt &&
 				    (XFS_INO_TO_AGBNO(mp, agino) %
 				     mp->m_sb.sb_inoalignmt))
 					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
-				check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
-					(xfs_extlen_t)MAX(1,
-						XFS_INODES_PER_CHUNK >>
-						mp->m_sb.sb_inopblog),
-					DBM_INODE, DBM_INODE, seqno, bno);
 			}
+
+			ioff = 0;
+			while (agbno < end_agbno &&
+			       ioff < XFS_INODES_PER_CHUNK) {
+				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
+					goto next_buf;
+
+                                check_set_dbmap(seqno, agbno,
+                                        (xfs_extlen_t)MAX(1,
+                                                inodes_per_buf >>
+                                                mp->m_sb.sb_inopblog),
+                                        DBM_INODE, DBM_INODE, seqno, bno);
+
+next_buf:
+				agbno += blks_per_buf;
+				ioff += inodes_per_buf;
+			}
+
 		}
 		return;
 	}
-- 
2.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Carlos Maiolino]

On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> Update the inode btree scanning functions to process sparse inode chunks
> correctly. For filesystems with sparse inode support enabled, process
> each chunk a cluster at a time. Each cluster is checked against the
> inobt record to determine if it is a hole and skipped if so.
> 
> Note that since xfs_check is deprecated in favor of xfs_repair,  this
> adds the minimum support necessary to process sparse inode enabled
> filesystems. In other words, this adds no sparse inode specific checks
> or verifications. We only update the inobt scanning functions to extend
> the existing level of verification to sparse inode enabled filesystems
> (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> or corruptions associated with sparse inode records must be detected and
> recovered via xfs_repair.
> 

Hi,

I'm not quite sure, but a while ago, I thought I've heard some rumors about
deprecating xfs_check, is this true or something that my mind made up for some
weird reason?


> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  db/check.c | 143 +++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 102 insertions(+), 41 deletions(-)
> 
> diff --git a/db/check.c b/db/check.c
> index 750ecc1..25146e5 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -4324,10 +4324,24 @@ scanfunc_ino(
>  	int			i;
>  	int			isfree;
>  	int			j;
> +	int			freecount;
>  	int			nfree;
>  	int			off;
>  	xfs_inobt_ptr_t		*pp;
>  	xfs_inobt_rec_t		*rp;
> +	xfs_agblock_t		agbno;
> +	xfs_agblock_t		end_agbno;
> +	struct xfs_dinode	*dip;
> +	int			blks_per_buf;
> +	int			inodes_per_buf;
> +	int			ioff;
> +
> +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> +		blks_per_buf = xfs_icluster_size_fsb(mp);
> +	else
> +		blks_per_buf = mp->m_ialloc_blks;
> +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> +			     XFS_INODES_PER_CHUNK);
>  
>  	if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
>  	    be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
> @@ -4357,54 +4371,74 @@ scanfunc_ino(
>  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
>  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
>  			agino = be32_to_cpu(rp[i].ir_startino);
> -			off = XFS_INO_TO_OFFSET(mp, agino);
> +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> +			end_agbno = agbno + mp->m_ialloc_blks;
>  			if (off == 0) {
>  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
>  				    mp->m_sb.sb_inoalignmt &&
>  				    (XFS_INO_TO_AGBNO(mp, agino) %
>  				     mp->m_sb.sb_inoalignmt))
>  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> -				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> -					(xfs_extlen_t)MAX(1,
> -						XFS_INODES_PER_CHUNK >>
> -						mp->m_sb.sb_inopblog),
> -					DBM_INODE, seqno, bno);
>  			}
> -			icount += XFS_INODES_PER_CHUNK;
> -			agicount += XFS_INODES_PER_CHUNK;
> -			ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> -			agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> +
>  			push_cur();
> -			set_cur(&typtab[TYP_INODE],
> -				XFS_AGB_TO_DADDR(mp, seqno,
> -						 XFS_AGINO_TO_AGBNO(mp, agino)),
> -				(int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
> -				DB_RING_IGN, NULL);
> -			if (iocur_top->data == NULL) {
> -				if (!sflag)
> -					dbprintf(_("can't read inode block "
> -						 "%u/%u\n"),
> -						seqno,
> -						XFS_AGINO_TO_AGBNO(mp, agino));
> -				error++;
> -				pop_cur();
> -				continue;
> -			}
> -			for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
> -				isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
> -				if (isfree)
> -					nfree++;
> -				process_inode(agf, agino + j,
> -					(xfs_dinode_t *)((char *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
> -						isfree);
> +
> +			ioff = 0;
> +			nfree = 0;
> +			while (agbno < end_agbno &&
> +			       ioff < XFS_INODES_PER_CHUNK) {
> +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> +					goto next_buf;
> +
> +				if (off < XFS_INODES_PER_CHUNK)
> +					set_dbmap(seqno, agbno, blks_per_buf,
> +						  DBM_INODE, seqno, bno);
> +
> +				icount += inodes_per_buf;
> +				agicount += inodes_per_buf;
> +
> +				set_cur(&typtab[TYP_INODE],
> +					XFS_AGB_TO_DADDR(mp, seqno, agbno),
> +					XFS_FSB_TO_BB(mp, blks_per_buf),
> +					DB_RING_IGN, NULL);
> +				if (iocur_top->data == NULL) {
> +					if (!sflag)
> +						dbprintf(_("can't read inode block "
> +							   "%u/%u\n"), seqno,
> +							 agbno);
> +					error++;
> +					goto next_buf;
> +				}
> +
> +				for (j = 0; j < inodes_per_buf; j++) {
> +					isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], ioff + j);
> +					if (isfree)
> +						nfree++;
> +					dip = (xfs_dinode_t *)((char *)iocur_top->data +
> +						((off + j) << mp->m_sb.sb_inodelog));
> +					process_inode(agf, agino + ioff + j, dip, isfree);
> +				}
> +
> +next_buf:
> +				agbno += blks_per_buf;
> +				ioff += inodes_per_buf;
>  			}
> -			if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
> +
> +			if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> +				freecount = rp[i].ir_u.sp.ir_freecount;
> +			else
> +				freecount = be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> +
> +			ifree += freecount;
> +			agifreecount += freecount;
> +
> +			if (nfree != freecount) {
>  				if (!sflag)
>  					dbprintf(_("ir_freecount/free mismatch, "
>  						 "inode chunk %u/%u, freecount "
>  						 "%d nfree %d\n"),
> -						seqno, agino,
> -						be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
> +						seqno, agino, freecount, nfree);
>  				error++;
>  			}
>  			pop_cur();
> @@ -4439,6 +4473,18 @@ scanfunc_fino(
>  	int			off;
>  	xfs_inobt_ptr_t		*pp;
>  	struct xfs_inobt_rec	*rp;
> +	xfs_agblock_t		agbno;
> +	xfs_agblock_t		end_agbno;
> +	int			blks_per_buf;
> +	int			inodes_per_buf;
> +	int			ioff;
> +
> +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> +		blks_per_buf = xfs_icluster_size_fsb(mp);
> +	else
> +		blks_per_buf = mp->m_ialloc_blks;
> +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> +			     XFS_INODES_PER_CHUNK);
>  
>  	if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
>  	    be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
> @@ -4468,19 +4514,34 @@ scanfunc_fino(
>  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
>  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
>  			agino = be32_to_cpu(rp[i].ir_startino);
> -			off = XFS_INO_TO_OFFSET(mp, agino);
> +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> +			end_agbno = agbno + mp->m_ialloc_blks;
>  			if (off == 0) {
>  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
>  				    mp->m_sb.sb_inoalignmt &&
>  				    (XFS_INO_TO_AGBNO(mp, agino) %
>  				     mp->m_sb.sb_inoalignmt))
>  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> -				check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> -					(xfs_extlen_t)MAX(1,
> -						XFS_INODES_PER_CHUNK >>
> -						mp->m_sb.sb_inopblog),
> -					DBM_INODE, DBM_INODE, seqno, bno);
>  			}
> +
> +			ioff = 0;
> +			while (agbno < end_agbno &&
> +			       ioff < XFS_INODES_PER_CHUNK) {
> +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> +					goto next_buf;
> +
> +                                check_set_dbmap(seqno, agbno,
> +                                        (xfs_extlen_t)MAX(1,
> +                                                inodes_per_buf >>
> +                                                mp->m_sb.sb_inopblog),
> +                                        DBM_INODE, DBM_INODE, seqno, bno);
> +
> +next_buf:
> +				agbno += blks_per_buf;
> +				ioff += inodes_per_buf;
> +			}
> +
>  		}
>  		return;
>  	}
> -- 
> 2.5.5
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Brian Foster]

On Tue, Jun 21, 2016 at 11:01:53AM +0200, Carlos Maiolino wrote:
> On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> > Update the inode btree scanning functions to process sparse inode chunks
> > correctly. For filesystems with sparse inode support enabled, process
> > each chunk a cluster at a time. Each cluster is checked against the
> > inobt record to determine if it is a hole and skipped if so.
> > 
> > Note that since xfs_check is deprecated in favor of xfs_repair,  this
> > adds the minimum support necessary to process sparse inode enabled
> > filesystems. In other words, this adds no sparse inode specific checks
> > or verifications. We only update the inobt scanning functions to extend
> > the existing level of verification to sparse inode enabled filesystems
> > (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> > or corruptions associated with sparse inode records must be detected and
> > recovered via xfs_repair.
> > 
> 
> Hi,
> 
> I'm not quite sure, but a while ago, I thought I've heard some rumors about
> deprecating xfs_check, is this true or something that my mind made up for some
> weird reason?
> 

I actually thought it already was. :) xfstests still runs check in
certain cases, however, and these patches just fix a regression.
Personally, I'd be fine with just dumping an "fs has sparse inodes, use
repair" message from xfs_check, but the basic sparse inode support had
already been added.

Brian

> 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  db/check.c | 143 +++++++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 102 insertions(+), 41 deletions(-)
> > 
> > diff --git a/db/check.c b/db/check.c
> > index 750ecc1..25146e5 100644
> > --- a/db/check.c
> > +++ b/db/check.c
> > @@ -4324,10 +4324,24 @@ scanfunc_ino(
> >  	int			i;
> >  	int			isfree;
> >  	int			j;
> > +	int			freecount;
> >  	int			nfree;
> >  	int			off;
> >  	xfs_inobt_ptr_t		*pp;
> >  	xfs_inobt_rec_t		*rp;
> > +	xfs_agblock_t		agbno;
> > +	xfs_agblock_t		end_agbno;
> > +	struct xfs_dinode	*dip;
> > +	int			blks_per_buf;
> > +	int			inodes_per_buf;
> > +	int			ioff;
> > +
> > +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > +		blks_per_buf = xfs_icluster_size_fsb(mp);
> > +	else
> > +		blks_per_buf = mp->m_ialloc_blks;
> > +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > +			     XFS_INODES_PER_CHUNK);
> >  
> >  	if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
> >  	    be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
> > @@ -4357,54 +4371,74 @@ scanfunc_ino(
> >  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> >  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> >  			agino = be32_to_cpu(rp[i].ir_startino);
> > -			off = XFS_INO_TO_OFFSET(mp, agino);
> > +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> > +			end_agbno = agbno + mp->m_ialloc_blks;
> >  			if (off == 0) {
> >  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> >  				    mp->m_sb.sb_inoalignmt &&
> >  				    (XFS_INO_TO_AGBNO(mp, agino) %
> >  				     mp->m_sb.sb_inoalignmt))
> >  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > -				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> > -					(xfs_extlen_t)MAX(1,
> > -						XFS_INODES_PER_CHUNK >>
> > -						mp->m_sb.sb_inopblog),
> > -					DBM_INODE, seqno, bno);
> >  			}
> > -			icount += XFS_INODES_PER_CHUNK;
> > -			agicount += XFS_INODES_PER_CHUNK;
> > -			ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > -			agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > +
> >  			push_cur();
> > -			set_cur(&typtab[TYP_INODE],
> > -				XFS_AGB_TO_DADDR(mp, seqno,
> > -						 XFS_AGINO_TO_AGBNO(mp, agino)),
> > -				(int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
> > -				DB_RING_IGN, NULL);
> > -			if (iocur_top->data == NULL) {
> > -				if (!sflag)
> > -					dbprintf(_("can't read inode block "
> > -						 "%u/%u\n"),
> > -						seqno,
> > -						XFS_AGINO_TO_AGBNO(mp, agino));
> > -				error++;
> > -				pop_cur();
> > -				continue;
> > -			}
> > -			for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
> > -				isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
> > -				if (isfree)
> > -					nfree++;
> > -				process_inode(agf, agino + j,
> > -					(xfs_dinode_t *)((char *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
> > -						isfree);
> > +
> > +			ioff = 0;
> > +			nfree = 0;
> > +			while (agbno < end_agbno &&
> > +			       ioff < XFS_INODES_PER_CHUNK) {
> > +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > +					goto next_buf;
> > +
> > +				if (off < XFS_INODES_PER_CHUNK)
> > +					set_dbmap(seqno, agbno, blks_per_buf,
> > +						  DBM_INODE, seqno, bno);
> > +
> > +				icount += inodes_per_buf;
> > +				agicount += inodes_per_buf;
> > +
> > +				set_cur(&typtab[TYP_INODE],
> > +					XFS_AGB_TO_DADDR(mp, seqno, agbno),
> > +					XFS_FSB_TO_BB(mp, blks_per_buf),
> > +					DB_RING_IGN, NULL);
> > +				if (iocur_top->data == NULL) {
> > +					if (!sflag)
> > +						dbprintf(_("can't read inode block "
> > +							   "%u/%u\n"), seqno,
> > +							 agbno);
> > +					error++;
> > +					goto next_buf;
> > +				}
> > +
> > +				for (j = 0; j < inodes_per_buf; j++) {
> > +					isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], ioff + j);
> > +					if (isfree)
> > +						nfree++;
> > +					dip = (xfs_dinode_t *)((char *)iocur_top->data +
> > +						((off + j) << mp->m_sb.sb_inodelog));
> > +					process_inode(agf, agino + ioff + j, dip, isfree);
> > +				}
> > +
> > +next_buf:
> > +				agbno += blks_per_buf;
> > +				ioff += inodes_per_buf;
> >  			}
> > -			if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
> > +
> > +			if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > +				freecount = rp[i].ir_u.sp.ir_freecount;
> > +			else
> > +				freecount = be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > +
> > +			ifree += freecount;
> > +			agifreecount += freecount;
> > +
> > +			if (nfree != freecount) {
> >  				if (!sflag)
> >  					dbprintf(_("ir_freecount/free mismatch, "
> >  						 "inode chunk %u/%u, freecount "
> >  						 "%d nfree %d\n"),
> > -						seqno, agino,
> > -						be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
> > +						seqno, agino, freecount, nfree);
> >  				error++;
> >  			}
> >  			pop_cur();
> > @@ -4439,6 +4473,18 @@ scanfunc_fino(
> >  	int			off;
> >  	xfs_inobt_ptr_t		*pp;
> >  	struct xfs_inobt_rec	*rp;
> > +	xfs_agblock_t		agbno;
> > +	xfs_agblock_t		end_agbno;
> > +	int			blks_per_buf;
> > +	int			inodes_per_buf;
> > +	int			ioff;
> > +
> > +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > +		blks_per_buf = xfs_icluster_size_fsb(mp);
> > +	else
> > +		blks_per_buf = mp->m_ialloc_blks;
> > +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > +			     XFS_INODES_PER_CHUNK);
> >  
> >  	if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
> >  	    be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
> > @@ -4468,19 +4514,34 @@ scanfunc_fino(
> >  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> >  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> >  			agino = be32_to_cpu(rp[i].ir_startino);
> > -			off = XFS_INO_TO_OFFSET(mp, agino);
> > +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> > +			end_agbno = agbno + mp->m_ialloc_blks;
> >  			if (off == 0) {
> >  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> >  				    mp->m_sb.sb_inoalignmt &&
> >  				    (XFS_INO_TO_AGBNO(mp, agino) %
> >  				     mp->m_sb.sb_inoalignmt))
> >  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > -				check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> > -					(xfs_extlen_t)MAX(1,
> > -						XFS_INODES_PER_CHUNK >>
> > -						mp->m_sb.sb_inopblog),
> > -					DBM_INODE, DBM_INODE, seqno, bno);
> >  			}
> > +
> > +			ioff = 0;
> > +			while (agbno < end_agbno &&
> > +			       ioff < XFS_INODES_PER_CHUNK) {
> > +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > +					goto next_buf;
> > +
> > +                                check_set_dbmap(seqno, agbno,
> > +                                        (xfs_extlen_t)MAX(1,
> > +                                                inodes_per_buf >>
> > +                                                mp->m_sb.sb_inopblog),
> > +                                        DBM_INODE, DBM_INODE, seqno, bno);
> > +
> > +next_buf:
> > +				agbno += blks_per_buf;
> > +				ioff += inodes_per_buf;
> > +			}
> > +
> >  		}
> >  		return;
> >  	}
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> -- 
> Carlos
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Carlos Maiolino]

On Tue, Jun 21, 2016 at 06:48:15AM -0400, Brian Foster wrote:
> On Tue, Jun 21, 2016 at 11:01:53AM +0200, Carlos Maiolino wrote:
> > On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> > > Update the inode btree scanning functions to process sparse inode chunks
> > > correctly. For filesystems with sparse inode support enabled, process
> > > each chunk a cluster at a time. Each cluster is checked against the
> > > inobt record to determine if it is a hole and skipped if so.
> > > 
> > > Note that since xfs_check is deprecated in favor of xfs_repair,  this
> > > adds the minimum support necessary to process sparse inode enabled
> > > filesystems. In other words, this adds no sparse inode specific checks
> > > or verifications. We only update the inobt scanning functions to extend
> > > the existing level of verification to sparse inode enabled filesystems
> > > (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> > > or corruptions associated with sparse inode records must be detected and
> > > recovered via xfs_repair.
> > > 
> > 
> > Hi,
> > 
> > I'm not quite sure, but a while ago, I thought I've heard some rumors about
> > deprecating xfs_check, is this true or something that my mind made up for some
> > weird reason?
> > 
> 
> I actually thought it already was. :) xfstests still runs check in
> certain cases, however, and these patches just fix a regression.
> Personally, I'd be fine with just dumping an "fs has sparse inodes, use
> repair" message from xfs_check, but the basic sparse inode support had
> already been added.
> 

:) I had no objection about the patch, just to make it clear. I was just curious
about how much we still care about xfs_check :)

> Brian
> 
> > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  db/check.c | 143 +++++++++++++++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 102 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/db/check.c b/db/check.c
> > > index 750ecc1..25146e5 100644
> > > --- a/db/check.c
> > > +++ b/db/check.c
> > > @@ -4324,10 +4324,24 @@ scanfunc_ino(
> > >  	int			i;
> > >  	int			isfree;
> > >  	int			j;
> > > +	int			freecount;
> > >  	int			nfree;
> > >  	int			off;
> > >  	xfs_inobt_ptr_t		*pp;
> > >  	xfs_inobt_rec_t		*rp;
> > > +	xfs_agblock_t		agbno;
> > > +	xfs_agblock_t		end_agbno;
> > > +	struct xfs_dinode	*dip;
> > > +	int			blks_per_buf;
> > > +	int			inodes_per_buf;
> > > +	int			ioff;
> > > +
> > > +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +		blks_per_buf = xfs_icluster_size_fsb(mp);
> > > +	else
> > > +		blks_per_buf = mp->m_ialloc_blks;
> > > +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > > +			     XFS_INODES_PER_CHUNK);
> > >  
> > >  	if (be32_to_cpu(block->bb_magic) != XFS_IBT_MAGIC &&
> > >  	    be32_to_cpu(block->bb_magic) != XFS_IBT_CRC_MAGIC) {
> > > @@ -4357,54 +4371,74 @@ scanfunc_ino(
> > >  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> > >  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> > >  			agino = be32_to_cpu(rp[i].ir_startino);
> > > -			off = XFS_INO_TO_OFFSET(mp, agino);
> > > +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > > +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> > > +			end_agbno = agbno + mp->m_ialloc_blks;
> > >  			if (off == 0) {
> > >  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> > >  				    mp->m_sb.sb_inoalignmt &&
> > >  				    (XFS_INO_TO_AGBNO(mp, agino) %
> > >  				     mp->m_sb.sb_inoalignmt))
> > >  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > > -				set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> > > -					(xfs_extlen_t)MAX(1,
> > > -						XFS_INODES_PER_CHUNK >>
> > > -						mp->m_sb.sb_inopblog),
> > > -					DBM_INODE, seqno, bno);
> > >  			}
> > > -			icount += XFS_INODES_PER_CHUNK;
> > > -			agicount += XFS_INODES_PER_CHUNK;
> > > -			ifree += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > -			agifreecount += be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > +
> > >  			push_cur();
> > > -			set_cur(&typtab[TYP_INODE],
> > > -				XFS_AGB_TO_DADDR(mp, seqno,
> > > -						 XFS_AGINO_TO_AGBNO(mp, agino)),
> > > -				(int)XFS_FSB_TO_BB(mp, mp->m_ialloc_blks),
> > > -				DB_RING_IGN, NULL);
> > > -			if (iocur_top->data == NULL) {
> > > -				if (!sflag)
> > > -					dbprintf(_("can't read inode block "
> > > -						 "%u/%u\n"),
> > > -						seqno,
> > > -						XFS_AGINO_TO_AGBNO(mp, agino));
> > > -				error++;
> > > -				pop_cur();
> > > -				continue;
> > > -			}
> > > -			for (j = 0, nfree = 0; j < XFS_INODES_PER_CHUNK; j++) {
> > > -				isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], j);
> > > -				if (isfree)
> > > -					nfree++;
> > > -				process_inode(agf, agino + j,
> > > -					(xfs_dinode_t *)((char *)iocur_top->data + ((off + j) << mp->m_sb.sb_inodelog)),
> > > -						isfree);
> > > +
> > > +			ioff = 0;
> > > +			nfree = 0;
> > > +			while (agbno < end_agbno &&
> > > +			       ioff < XFS_INODES_PER_CHUNK) {
> > > +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > > +					goto next_buf;
> > > +
> > > +				if (off < XFS_INODES_PER_CHUNK)
> > > +					set_dbmap(seqno, agbno, blks_per_buf,
> > > +						  DBM_INODE, seqno, bno);
> > > +
> > > +				icount += inodes_per_buf;
> > > +				agicount += inodes_per_buf;
> > > +
> > > +				set_cur(&typtab[TYP_INODE],
> > > +					XFS_AGB_TO_DADDR(mp, seqno, agbno),
> > > +					XFS_FSB_TO_BB(mp, blks_per_buf),
> > > +					DB_RING_IGN, NULL);
> > > +				if (iocur_top->data == NULL) {
> > > +					if (!sflag)
> > > +						dbprintf(_("can't read inode block "
> > > +							   "%u/%u\n"), seqno,
> > > +							 agbno);
> > > +					error++;
> > > +					goto next_buf;
> > > +				}
> > > +
> > > +				for (j = 0; j < inodes_per_buf; j++) {
> > > +					isfree = XFS_INOBT_IS_FREE_DISK(&rp[i], ioff + j);
> > > +					if (isfree)
> > > +						nfree++;
> > > +					dip = (xfs_dinode_t *)((char *)iocur_top->data +
> > > +						((off + j) << mp->m_sb.sb_inodelog));
> > > +					process_inode(agf, agino + ioff + j, dip, isfree);
> > > +				}
> > > +
> > > +next_buf:
> > > +				agbno += blks_per_buf;
> > > +				ioff += inodes_per_buf;
> > >  			}
> > > -			if (nfree != be32_to_cpu(rp[i].ir_u.f.ir_freecount)) {
> > > +
> > > +			if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +				freecount = rp[i].ir_u.sp.ir_freecount;
> > > +			else
> > > +				freecount = be32_to_cpu(rp[i].ir_u.f.ir_freecount);
> > > +
> > > +			ifree += freecount;
> > > +			agifreecount += freecount;
> > > +
> > > +			if (nfree != freecount) {
> > >  				if (!sflag)
> > >  					dbprintf(_("ir_freecount/free mismatch, "
> > >  						 "inode chunk %u/%u, freecount "
> > >  						 "%d nfree %d\n"),
> > > -						seqno, agino,
> > > -						be32_to_cpu(rp[i].ir_u.f.ir_freecount), nfree);
> > > +						seqno, agino, freecount, nfree);
> > >  				error++;
> > >  			}
> > >  			pop_cur();
> > > @@ -4439,6 +4473,18 @@ scanfunc_fino(
> > >  	int			off;
> > >  	xfs_inobt_ptr_t		*pp;
> > >  	struct xfs_inobt_rec	*rp;
> > > +	xfs_agblock_t		agbno;
> > > +	xfs_agblock_t		end_agbno;
> > > +	int			blks_per_buf;
> > > +	int			inodes_per_buf;
> > > +	int			ioff;
> > > +
> > > +	if (xfs_sb_version_hassparseinodes(&mp->m_sb))
> > > +		blks_per_buf = xfs_icluster_size_fsb(mp);
> > > +	else
> > > +		blks_per_buf = mp->m_ialloc_blks;
> > > +	inodes_per_buf = min(blks_per_buf << mp->m_sb.sb_inopblog,
> > > +			     XFS_INODES_PER_CHUNK);
> > >  
> > >  	if (be32_to_cpu(block->bb_magic) != XFS_FIBT_MAGIC &&
> > >  	    be32_to_cpu(block->bb_magic) != XFS_FIBT_CRC_MAGIC) {
> > > @@ -4468,19 +4514,34 @@ scanfunc_fino(
> > >  		rp = XFS_INOBT_REC_ADDR(mp, block, 1);
> > >  		for (i = 0; i < be16_to_cpu(block->bb_numrecs); i++) {
> > >  			agino = be32_to_cpu(rp[i].ir_startino);
> > > -			off = XFS_INO_TO_OFFSET(mp, agino);
> > > +			agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> > > +			off = XFS_AGINO_TO_OFFSET(mp, agino);
> > > +			end_agbno = agbno + mp->m_ialloc_blks;
> > >  			if (off == 0) {
> > >  				if ((sbversion & XFS_SB_VERSION_ALIGNBIT) &&
> > >  				    mp->m_sb.sb_inoalignmt &&
> > >  				    (XFS_INO_TO_AGBNO(mp, agino) %
> > >  				     mp->m_sb.sb_inoalignmt))
> > >  					sbversion &= ~XFS_SB_VERSION_ALIGNBIT;
> > > -				check_set_dbmap(seqno, XFS_AGINO_TO_AGBNO(mp, agino),
> > > -					(xfs_extlen_t)MAX(1,
> > > -						XFS_INODES_PER_CHUNK >>
> > > -						mp->m_sb.sb_inopblog),
> > > -					DBM_INODE, DBM_INODE, seqno, bno);
> > >  			}
> > > +
> > > +			ioff = 0;
> > > +			while (agbno < end_agbno &&
> > > +			       ioff < XFS_INODES_PER_CHUNK) {
> > > +				if (xfs_inobt_is_sparse_disk(&rp[i], ioff))
> > > +					goto next_buf;
> > > +
> > > +                                check_set_dbmap(seqno, agbno,
> > > +                                        (xfs_extlen_t)MAX(1,
> > > +                                                inodes_per_buf >>
> > > +                                                mp->m_sb.sb_inopblog),
> > > +                                        DBM_INODE, DBM_INODE, seqno, bno);
> > > +
> > > +next_buf:
> > > +				agbno += blks_per_buf;
> > > +				ioff += inodes_per_buf;
> > > +			}
> > > +
> > >  		}
> > >  		return;
> > >  	}
> > > -- 
> > > 2.5.5
> > > 
> > > _______________________________________________
> > > xfs mailing list
> > > xfs@oss.sgi.com
> > > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> > -- 
> > Carlos
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Eric Sandeen]

On 6/21/16 4:01 AM, Carlos Maiolino wrote:
> On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
>> Update the inode btree scanning functions to process sparse inode chunks
>> correctly. For filesystems with sparse inode support enabled, process
>> each chunk a cluster at a time. Each cluster is checked against the
>> inobt record to determine if it is a hole and skipped if so.
>>
>> Note that since xfs_check is deprecated in favor of xfs_repair,  this
>> adds the minimum support necessary to process sparse inode enabled
>> filesystems. In other words, this adds no sparse inode specific checks
>> or verifications. We only update the inobt scanning functions to extend
>> the existing level of verification to sparse inode enabled filesystems
>> (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
>> or corruptions associated with sparse inode records must be detected and
>> recovered via xfs_repair.
>>
> 
> Hi,
> 
> I'm not quite sure, but a while ago, I thought I've heard some rumors about
> deprecating xfs_check, is this true or something that my mind made up for some
> weird reason?

Yes, like Brian said above.  ;)

bfc541e xfsprogs: remove xfs_check
12a48f5 xfsprogs: remove xfs_check references from fsck.xfs script & manpage

However, we still run check inside xfs_db in xfstests as an independent
verification step:

187bccd xfstests: Remove dependence of xfs_check script

+# xfs_check script is planned to be deprecated. But, we want to
+# be able to invoke "xfs_check" behavior in xfstests in order to
+# maintain the current verification levels.
+_xfs_check()

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Dave Chinner]

On Tue, Jun 21, 2016 at 08:54:04AM -0500, Eric Sandeen wrote:
> 
> 
> On 6/21/16 4:01 AM, Carlos Maiolino wrote:
> > On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> >> Update the inode btree scanning functions to process sparse inode chunks
> >> correctly. For filesystems with sparse inode support enabled, process
> >> each chunk a cluster at a time. Each cluster is checked against the
> >> inobt record to determine if it is a hole and skipped if so.
> >>
> >> Note that since xfs_check is deprecated in favor of xfs_repair,  this
> >> adds the minimum support necessary to process sparse inode enabled
> >> filesystems. In other words, this adds no sparse inode specific checks
> >> or verifications. We only update the inobt scanning functions to extend
> >> the existing level of verification to sparse inode enabled filesystems
> >> (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> >> or corruptions associated with sparse inode records must be detected and
> >> recovered via xfs_repair.
> >>
> > 
> > Hi,
> > 
> > I'm not quite sure, but a while ago, I thought I've heard some rumors about
> > deprecating xfs_check, is this true or something that my mind made up for some
> > weird reason?
> 
> Yes, like Brian said above.  ;)
> 
> bfc541e xfsprogs: remove xfs_check
> 12a48f5 xfsprogs: remove xfs_check references from fsck.xfs script & manpage
> 
> However, we still run check inside xfs_db in xfstests as an independent
> verification step:
> 
> 187bccd xfstests: Remove dependence of xfs_check script
> 
> +# xfs_check script is planned to be deprecated. But, we want to
> +# be able to invoke "xfs_check" behavior in xfstests in order to
> +# maintain the current verification levels.
> +_xfs_check()

Right - it's a secondary set of code that effectively tells us
whether repair is detecting all the problems it should. i.e. if
check fails and repair doesn't, then we've got a bug in repair that
needs fixing.

Also, the check code is really just an "addon" to other
functionality in xfs_db (e.g. blockget) that we have to keep, so
removing check doesn't really gain us all that much in terms of
reduced maintenance overhead. So long as we can easily keep check up
to date with new features I think we shoul dbe keeping it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Carlos Maiolino]

On Wed, Jun 22, 2016 at 09:29:40AM +1000, Dave Chinner wrote:
> On Tue, Jun 21, 2016 at 08:54:04AM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 6/21/16 4:01 AM, Carlos Maiolino wrote:
> > > On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> > >> Update the inode btree scanning functions to process sparse inode chunks
> > >> correctly. For filesystems with sparse inode support enabled, process
> > >> each chunk a cluster at a time. Each cluster is checked against the
> > >> inobt record to determine if it is a hole and skipped if so.
> > >>
> > >> Note that since xfs_check is deprecated in favor of xfs_repair,  this
> > >> adds the minimum support necessary to process sparse inode enabled
> > >> filesystems. In other words, this adds no sparse inode specific checks
> > >> or verifications. We only update the inobt scanning functions to extend
> > >> the existing level of verification to sparse inode enabled filesystems
> > >> (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> > >> or corruptions associated with sparse inode records must be detected and
> > >> recovered via xfs_repair.
> > >>
> > > 
> > > Hi,
> > > 
> > > I'm not quite sure, but a while ago, I thought I've heard some rumors about
> > > deprecating xfs_check, is this true or something that my mind made up for some
> > > weird reason?
> > 
> > Yes, like Brian said above.  ;)
> > 
> > bfc541e xfsprogs: remove xfs_check
> > 12a48f5 xfsprogs: remove xfs_check references from fsck.xfs script & manpage
> > 
> > However, we still run check inside xfs_db in xfstests as an independent
> > verification step:
> > 
> > 187bccd xfstests: Remove dependence of xfs_check script
> > 
> > +# xfs_check script is planned to be deprecated. But, we want to
> > +# be able to invoke "xfs_check" behavior in xfstests in order to
> > +# maintain the current verification levels.
> > +_xfs_check()
> 
> Right - it's a secondary set of code that effectively tells us
> whether repair is detecting all the problems it should. i.e. if
> check fails and repair doesn't, then we've got a bug in repair that
> needs fixing.
> 

Just out of curiosity here, have we hit any case like that for the past few
versions?

> Also, the check code is really just an "addon" to other
> functionality in xfs_db (e.g. blockget) that we have to keep, so
> removing check doesn't really gain us all that much in terms of
> reduced maintenance overhead. So long as we can easily keep check up
> to date with new features I think we shoul dbe keeping it...
> 

Good to know.

Cheers o/

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_check: process sparse inode chunks correctly

[Darrick J. Wong]

On Wed, Jun 22, 2016 at 10:03:24AM +0200, Carlos Maiolino wrote:
> On Wed, Jun 22, 2016 at 09:29:40AM +1000, Dave Chinner wrote:
> > On Tue, Jun 21, 2016 at 08:54:04AM -0500, Eric Sandeen wrote:
> > > 
> > > 
> > > On 6/21/16 4:01 AM, Carlos Maiolino wrote:
> > > > On Mon, Jun 20, 2016 at 12:52:42PM -0400, Brian Foster wrote:
> > > >> Update the inode btree scanning functions to process sparse inode chunks
> > > >> correctly. For filesystems with sparse inode support enabled, process
> > > >> each chunk a cluster at a time. Each cluster is checked against the
> > > >> inobt record to determine if it is a hole and skipped if so.
> > > >>
> > > >> Note that since xfs_check is deprecated in favor of xfs_repair,  this
> > > >> adds the minimum support necessary to process sparse inode enabled
> > > >> filesystems. In other words, this adds no sparse inode specific checks
> > > >> or verifications. We only update the inobt scanning functions to extend
> > > >> the existing level of verification to sparse inode enabled filesystems
> > > >> (e.g., avoid incorrectly tracking sparse regions as inodes). Problems
> > > >> or corruptions associated with sparse inode records must be detected and
> > > >> recovered via xfs_repair.
> > > >>
> > > > 
> > > > Hi,
> > > > 
> > > > I'm not quite sure, but a while ago, I thought I've heard some rumors about
> > > > deprecating xfs_check, is this true or something that my mind made up for some
> > > > weird reason?
> > > 
> > > Yes, like Brian said above.  ;)
> > > 
> > > bfc541e xfsprogs: remove xfs_check
> > > 12a48f5 xfsprogs: remove xfs_check references from fsck.xfs script & manpage
> > > 
> > > However, we still run check inside xfs_db in xfstests as an independent
> > > verification step:
> > > 
> > > 187bccd xfstests: Remove dependence of xfs_check script
> > > 
> > > +# xfs_check script is planned to be deprecated. But, we want to
> > > +# be able to invoke "xfs_check" behavior in xfstests in order to
> > > +# maintain the current verification levels.
> > > +_xfs_check()
> > 
> > Right - it's a secondary set of code that effectively tells us
> > whether repair is detecting all the problems it should. i.e. if
> > check fails and repair doesn't, then we've got a bug in repair that
> > needs fixing.
> > 
> 
> Just out of curiosity here, have we hit any case like that for the past few
> versions?

I have, while writing reflink.  Granted, that only happened /once/.

While we're talking about repair/xfstests, I also started running
xfs_repair and then xfs_repair -n to _check_xfs_filesystem, which
helped me find some cases where the rmapbt rebuild wasn't quite right.

--D

> 
> > Also, the check code is really just an "addon" to other
> > functionality in xfs_db (e.g. blockget) that we have to keep, so
> > removing check doesn't really gain us all that much in terms of
> > reduced maintenance overhead. So long as we can easily keep check up
> > to date with new features I think we shoul dbe keeping it...
> > 
> 
> Good to know.
> 
> Cheers o/
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> 
> -- 
> Carlos
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs