[PATCH] xfs: check for not fully initialized inodes in xfs_ireclaim

[PATCH] xfs: check for not fully initialized inodes in xfs_ireclaim

[Christoph Hellwig]

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.

Signed-off-by: Christoph Hellwig <hch@lst.de>

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);
 
 	XFS_STATS_INC(xs_ig_reclaims);
 
 	/*
-	 * 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);
 	write_lock(&pag->pag_ici_lock);
-	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);
 	write_unlock(&pag->pag_ici_lock);
 	xfs_put_perag(mp, pag);
 

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

RE: [PATCH] xfs: check for not fully initialized inodes in xfs_ireclaim

[Alex Elder]

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
lookup.

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).

					-Alex

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 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);
> 
>  	XFS_STATS_INC(xs_ig_reclaims);
> 
>  	/*
> -	 * 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);
>  	write_lock(&pag->pag_ici_lock);
> -	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);
>  	write_unlock(&pag->pag_ici_lock);
>  	xfs_put_perag(mp, pag);
> 
> 
> _______________________________________________
> 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] xfs: check for not fully initialized inodes in xfs_ireclaim

[Christoph Hellwig]

On Mon, Nov 30, 2009 at 02:07:40PM -0600, Alex Elder wrote:
> 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
> lookup.
> 
> 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).

Sounds good, I'll redo it.

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

[PATCH V2] xfs: check for not fully initialized inodes in xfs_ireclaim

[Christoph Hellwig]

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.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/xfs_iget.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iget.c	2009-11-26 19:10:47.206253628 +0100
+++ xfs/fs/xfs/xfs_iget.c	2009-12-01 19:09:43.310004091 +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);
 
 	XFS_STATS_INC(xs_ig_reclaims);
 
 	/*
-	 * 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);
 	write_lock(&pag->pag_ici_lock);
-	radix_tree_delete(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, ip->i_ino));
+	if (!radix_tree_delete(&pag->pag_ici_root, agino))
+		ASSERT(0);
 	write_unlock(&pag->pag_ici_lock);
 	xfs_put_perag(mp, pag);
 

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

RE: [PATCH V2] xfs: check for not fully initialized inodes in xfs_ireclaim

[Alex Elder]

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.

It's a shame we can't do more than an ASSERT() here
(for non-debug builds).

Looks good though.

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Alex Elder <aelder@sgi.com>

> Index: xfs/fs/xfs/xfs_iget.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iget.c	2009-11-26 19:10:47.206253628 +0100
> +++ xfs/fs/xfs/xfs_iget.c	2009-12-01 19:09:43.310004091 +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);
> 
>  	XFS_STATS_INC(xs_ig_reclaims);
> 
>  	/*
> -	 * 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);
>  	write_lock(&pag->pag_ici_lock);
> -	radix_tree_delete(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, ip->i_ino));
> +	if (!radix_tree_delete(&pag->pag_ici_root, agino))
> +		ASSERT(0);
>  	write_unlock(&pag->pag_ici_lock);
>  	xfs_put_perag(mp, pag);
> 
> 
> _______________________________________________
> 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