Re: [PATCH 3/3] free inodes using destroy_inode

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 21, 2008 at 12:20:44AM +0200, Christoph Hellwig wrote:
> To make sure we free the security data inodes need to be freed using
> the proper VFS helper (which we also need to export for this).  To make
> sure we don't corrupt the radix tree we need to add another special
> case to xfs_reclaim for inodes that haven't been fully initialized yet.

Yes, this should fix the problems destroying inodes in the
not-quite-fully-instantiated state. Couple of things, though.

> Index: xfs-2.6/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs-2.6.orig/fs/xfs/xfs_inode.c	2008-10-20 23:54:05.000000000 +0200
> +++ xfs-2.6/fs/xfs/xfs_inode.c	2008-10-20 23:54:08.000000000 +0200
> @@ -872,10 +872,8 @@ xfs_iread(
>  	imap.im_blkno = bno;
>  	error = xfs_imap(mp, tp, ip->i_ino, &imap,
>  				XFS_IMAP_LOOKUP | imap_flags);
> -	if (error) {
> -		xfs_idestroy(ip);
> -		return error;
> -	}
> +	if (error)
> + 		goto out_destroy_inode;
   ^
Extra whitespace.

> @@ -887,10 +885,8 @@ xfs_iread(
>  	ASSERT(bno == 0 || bno == imap.im_blkno);
>  
>  	error = xfs_imap_to_bp(mp, tp, &imap, &bp, XFS_BUF_LOCK, imap_flags);
> -	if (error) {
> -		xfs_idestroy(ip);
> -		return error;
> -	}
> +	if (error)
> + 		goto out_destroy_inode;
   ^
Ditto.

> Index: xfs-2.6/fs/xfs/xfs_inode.h
> ===================================================================
> --- xfs-2.6.orig/fs/xfs/xfs_inode.h	2008-10-20 23:54:05.000000000 +0200
> +++ xfs-2.6/fs/xfs/xfs_inode.h	2008-10-20 23:54:08.000000000 +0200
> @@ -309,6 +309,12 @@ static inline struct inode *VFS_I(struct
>  	return &ip->i_vnode;
>  }
>  
> +static inline void xfs_destroy_inode(struct xfs_inode *ip)
> +{
> +	make_bad_inode(VFS_I(ip));
> +	return destroy_inode(VFS_I(ip));
> +}

Yes, makes sense to mark it bad first to avoid most of the
reclaim code.

> Index: xfs-2.6/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- xfs-2.6.orig/fs/xfs/xfs_vnodeops.c	2008-10-20 23:49:27.000000000 +0200
> +++ xfs-2.6/fs/xfs/xfs_vnodeops.c	2008-10-20 23:55:31.000000000 +0200
> @@ -2798,13 +2798,19 @@ int
>  xfs_reclaim(
>  	xfs_inode_t	*ip)
>  {
> +	struct inode	*inode = VFS_I(ip);
>  
>  	xfs_itrace_entry(ip);
>  
> -	ASSERT(!VN_MAPPED(VFS_I(ip)));
> +	ASSERT(!VN_MAPPED(inode));
> +
> +	if (unlikely(inode->i_state & I_NEW)) {
> +		xfs_idestroy(ip);
> +		return 0;
> +	}

Can that happen? I thought xfs_iput_new() took care of clearing the
I_NEW flag via unlock_new_inode() and so there is no way that flag
can leak through to here. perhaps a comment explaining what the
error path is that leads to needing this check is in order....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com