review: fix bulkstat error detection logic

review: fix bulkstat error detection logic

[Nathan Scott]

Hi,

An earlier patch to remove bulkstat verbosity in debug kernels
changed the xfs_itobp logic slightly, such that we no longer do
the first stage of buffer checking there.  Turns out this isn't
the right fix, we need to do that check still, and pass out the
error code to bulkstat still, otherwise we either trip up logic
in later debug code, or pass out success incorrectly.

This fixes that up, and is the last bulkstat change I have for
awhile. :)  Its error handling is correct once more, its a fair
bit quicker, and no more debug-kernel console spam - hooray!

cheers.

-- 
Nathan


Index: xfs-linux/xfs_inode.c
===================================================================
--- xfs-linux.orig/xfs_inode.c	2006-07-25 12:02:05.787585750 +1000
+++ xfs-linux/xfs_inode.c	2006-07-25 13:04:58.941286750 +1000
@@ -335,10 +335,9 @@ xfs_itobp(
 #if !defined(__KERNEL__)
 	ni = 0;
 #elif defined(DEBUG)
-	ni = (imap_flags & XFS_IMAP_BULKSTAT) ? 0 :
-		(BBTOB(imap.im_len) >> mp->m_sb.sb_inodelog);
+	ni = BBTOB(imap.im_len) >> mp->m_sb.sb_inodelog;
 #else	/* usual case */
-	ni = (imap_flags & XFS_IMAP_BULKSTAT) ? 0 : 1;
+	ni = 1;
 #endif
 
 	for (i = 0; i < ni; i++) {
@@ -349,11 +348,15 @@ xfs_itobp(
 					(i << mp->m_sb.sb_inodelog));
 		di_ok = INT_GET(dip->di_core.di_magic, ARCH_CONVERT) == XFS_DINODE_MAGIC &&
 			    XFS_DINODE_GOOD_VERSION(INT_GET(dip->di_core.di_version, ARCH_CONVERT));
-		if (unlikely(XFS_TEST_ERROR(!di_ok, mp, XFS_ERRTAG_ITOBP_INOTOBP,
-				 XFS_RANDOM_ITOBP_INOTOBP))) {
+		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
+						XFS_ERRTAG_ITOBP_INOTOBP,
+						XFS_RANDOM_ITOBP_INOTOBP))) {
+			if (imap_flags & XFS_IMAP_BULKSTAT) {
+				xfs_trans_brelse(tp, bp);
+				return XFS_ERROR(EINVAL);
+			}
 #ifdef DEBUG
-			if (!(imap_flags & XFS_IMAP_BULKSTAT))
-				cmn_err(CE_ALERT,
+			cmn_err(CE_ALERT,
 					"Device %s - bad inode magic/vsn "
 					"daddr %lld #%d (magic=%x)",
 				XFS_BUFTARG_NAME(mp->m_ddev_targp),

Re: review: fix bulkstat error detection logic

[Timothy Shimmin]

Nathan Scott wrote:
> Hi,
> 
> An earlier patch to remove bulkstat verbosity in debug kernels
> changed the xfs_itobp logic slightly, such that we no longer do
> the first stage of buffer checking there.  Turns out this isn't
> the right fix, we need to do that check still, and pass out the
> error code to bulkstat still, otherwise we either trip up logic
> in later debug code, or pass out success incorrectly.
> 
> This fixes that up, and is the last bulkstat change I have for
> awhile. :)  Its error handling is correct once more, its a fair
> bit quicker, and no more debug-kernel console spam - hooray!
> 
> cheers.
> 

Looks reasonable.
So you still do the inode buffer validation but we don't print out
a corruption error msg and we return EINVAL instead of EFSCORRUPTED.
Can we not be bulkstat'ing over inodes with reasonable numbers/locations but
the inode data on disk is just corrupted?
(I missed looking at the original patch)

(Interesting how previously you set ni to 0 in the XFS_IMAP_BULKSTAT
case and yet in the loop which would then never iterate (0<0=F),
you test for XFS_IMAP_BULKSTAT :)

--Tim

Re: review: fix bulkstat error detection logic

[Nathan Scott]

On Fri, Jul 28, 2006 at 03:40:35PM +1000, Timothy Shimmin wrote:
> Looks reasonable.
> So you still do the inode buffer validation but we don't print out
> a corruption error msg and we return EINVAL instead of EFSCORRUPTED.

Right.

> Can we not be bulkstat'ing over inodes with reasonable numbers/locations but
> the inode data on disk is just corrupted?

Yes, we can, but we don't want bulkstat to shutdown the filesystem
in that situation.

cheers.

-- 
Nathan