review: fix infinite loop in bulkstat

review: fix infinite loop in bulkstat

[Lachlan McIlroy]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

This fix prevents bulkstat from spinning in an infinite loop.

Here 'agino' increments through the inodes in an allocation group.
At the end of the innermost 'for' loop it will hold the value of the
next inode to look at (ie the first inode in the next cluster/chunk).
Assigning 'lastino' to 'agino' resets it to the last inode in the
last inode cluster we just looked at.  This causes us to look up
the very same cluster and examine all the inodes all over again,
and again, and again...

We also want to set 'lastino' for the cases when we're not interested
in the inode so that the next call to bulkstat wont re-examine the
same uninteresting inodes.

[-- Attachment #2: bulkstat.diff --]
[-- Type: text/x-patch, Size: 1014 bytes --]

--- fs/xfs/xfs_itable.c_1.155	2007-10-03 13:05:22.000000000 +1000
+++ fs/xfs/xfs_itable.c	2007-10-04 12:16:46.000000000 +1000
@@ -622,8 +622,10 @@ xfs_bulkstat(
 				/*
 				 * Skip if this inode is free.
 				 */
-				if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free)
+				if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
+					lastino = ino;
 					continue;
+				}
 				/*
 				 * Count used inodes as free so we can tell
 				 * when the chunk is used up.
@@ -632,8 +634,10 @@ xfs_bulkstat(
 				ino = XFS_AGINO_TO_INO(mp, agno, agino);
 				bno = XFS_AGB_TO_DADDR(mp, agno, agbno);
 				if (!xfs_bulkstat_use_dinode(mp, flags, bp,
-							     clustidx, &dip))
+							     clustidx, &dip)) {
+					lastino = ino;
 					continue;
+				}
 				/*
 				 * If we need to do an iget, cannot hold bp.
 				 * Drop it, until starting the next cluster.
@@ -694,8 +698,7 @@ xfs_bulkstat(
 			if (end_of_ag) {
 				agno++;
 				agino = 0;
-			} else
-				agino = XFS_INO_TO_AGINO(mp, lastino);
+			}
 		} else
 			break;
 	}

Re: review: fix infinite loop in bulkstat

[David Chinner]

On Thu, Oct 04, 2007 at 03:20:12PM +1000, Lachlan McIlroy wrote:
> This fix prevents bulkstat from spinning in an infinite loop.
> 
> Here 'agino' increments through the inodes in an allocation group.
> At the end of the innermost 'for' loop it will hold the value of the
> next inode to look at (ie the first inode in the next cluster/chunk).
> Assigning 'lastino' to 'agino' resets it to the last inode in the
> last inode cluster we just looked at.  This causes us to look up
> the very same cluster and examine all the inodes all over again,
> and again, and again...
> 
> We also want to set 'lastino' for the cases when we're not interested
> in the inode so that the next call to bulkstat wont re-examine the
> same uninteresting inodes.

Looks OK. I think you shoul dalso add a comment to the effect that
lastino needs to be set before continuing to process the next inode
in the loop so it doesn't get forgotten next time someone modifies
the code inside the loop..

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group