On Wed, Nov 09, 2011 at 03:37:29AM -0500, Christoph Hellwig wrote: > Stefan Pfetzing reported a bug where xfs_repair got stuck eating 100% CPU in > phase3. We track it down to a loop in the unlinked inode list, apparently > caused by memory corruption on an iSCSI target. > > I looked into tracking if we already saw a given unlinked inode, but given > that we keep walking even for inodes where we can't find an allocation btree > record that seems infeasible. On the other hand these inodes had their > final unlink and thus were dead even before the system went down. There > really is no point in adding them to the uncertain list and looking for > references to them later. You're making the assumption that log recovery has done the correct thing any only replayed entire unlink transactions and hence the filesystem is otherwise consistent (i.e that there are no other references). I think that's a bad assumption - there's no guarantee that the unlinked list only contains unreferenced inodes if there's been corruption and/or log replay was not able to be run. > So the simplest fix seems to be to simply remove the unlinked inode list > walk and just clear it - when we rebuild the inode allocation btrees these > will simply be marked free. I also think there's more to it than that. The walk of the inode list also marks all the blocks in the block map as containing inodes, and all the blocks still used by those inodes as data/bmap/attr types. This change removes that, so we're going to potentially lose that state if all the inodes in a block are on the unlinked list. Hence we'll end up with blocks containing inodes that are still marked as used in the AGINO btree, but are marked as free space in the block map. We'll also end up with data blocks that are otherwise still used as not being marked as used, and that is especially important for discovering multiply allocated blocks when a block has been freed (e.g. just before unlink) and then immediately reallocated and then the crash has left the state on disk inconsistent.... IOWs, it seems to me that simply removing the walk has more potential downsides in terms of error detection and tracking than it provides in benefits. I suspect that just capping the number of loops that can be executed is the simplest thing to do here. e.g. allow it to loop for as many times as there are inodes allocated in the AG or filesystem (e.g. agi->agi_count - agi->agi_free). Yes, it will still spin for some time on this sort of corruption, but it won't get stuck, and it won't add new holes into our block/inode usage tracking... The logical extension of this is that having a "unlinked inode count" in the AGI would be really useful here. I'll add it to the (growing) list of "things to add with CRC checking on-disk format modifications". Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
