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

Dave Chinner
2008-10-21T03:05:47Z

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