Christoph Hellwig wrote:
> Add an assert for inodes not added to the inode cache in xfs_ireclaim, to make
> sure we're not going to introduce something like the famous nfsd inode cache
> bug again.
I confess I have not looked at the radix tree
code in detail yet, but radix_tree_delete()
returns a value that is ignored here. It reportedly
will return a null pointer if the index has no
corresponding item in the tree, so this could be
used in the assertion rather than doing a separate
I think the change is fine, but would rather see
it done without the extra lookup (though there may
be a good reason for doing it anyway).
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Index: xfs/fs/xfs/xfs_iget.c
> --- xfs.orig/fs/xfs/xfs_iget.c 2009-11-12 17:10:20.216276486 +0100
> +++ xfs/fs/xfs/xfs_iget.c 2009-11-12 17:13:27.565003964 +0100
> @@ -514,17 +514,21 @@ xfs_ireclaim(
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_perag *pag;
> + xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
> - * Remove the inode from the per-AG radix tree. It doesn't matter
> - * if it was never added to it because radix_tree_delete can deal
> - * with that case just fine.
> + * Remove the inode from the per-AG radix tree.
> + *
> + * Because radix_tree_delete won't complain even if the item was never
> + * added to the tree assert that it's been there before to catch
> + * problems with the inode life time early on.
> pag = xfs_get_perag(mp, ip->i_ino);
> - radix_tree_delete(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, ip->i_ino));
> + ASSERT(radix_tree_lookup(&pag->pag_ici_root, agino));
> + radix_tree_delete(&pag->pag_ici_root, agino);
> xfs_put_perag(mp, pag);
> xfs mailing list