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

Christoph Hellwig
2008-10-21T09:05:30Z

On Tue, Oct 21, 2008 at 02:07:26PM +1100, Dave Chinner wrote:
> ^
> Extra whitespace.
>
> ^
> Ditto.

Fixed.

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

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

The make_inode_bad isn't actually nessecary anymore - this was my first
attempt at skipping the flushing in xfs_reclaim, but it was still too
much as the radix tree removal for and inode that's not in the tree
tripped up quite badly. So I use I_NEW here to detect these half setup
inodes. Real I_NEW indoes still go through xfs_iput_new.

Updated version that has a comment explaining all this below:

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

Index: xfs-2.6/fs/inode.c
===================================================================
--- xfs-2.6.orig/fs/inode.c 2008-10-20 23:49:27.000000000 +0200
+++ xfs-2.6/fs/inode.c 2008-10-20 23:54:08.000000000 +0200
@@ -212,6 +212,7 @@ void destroy_inode(struct inode *inode)
else
kmem_cache_free(inode_cachep, (inode));
}
+EXPORT_SYMBOL(destroy_inode);

/*
Index: xfs-2.6/fs/xfs/xfs_iget.c
===================================================================
--- xfs-2.6.orig/fs/xfs/xfs_iget.c 2008-10-20 23:49:27.000000000 +0200
+++ xfs-2.6/fs/xfs/xfs_iget.c 2008-10-20 23:54:08.000000000 +0200
@@ -197,7 +197,7 @@ out_unlock:
write_unlock(&pag->pag_ici_lock);
radix_tree_preload_end();
out_destroy:
- xfs_idestroy(ip);
+ xfs_destroy_inode(ip);
return error;
}

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-21 09:24:30.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;

/*
* Fill in the fields in the inode that will be used to
@@ -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;

dip = (xfs_dinode_t *)xfs_buf_offset(bp, imap.im_boffset);

@@ -899,8 +895,6 @@ xfs_iread(
* (nfs or dmi) has a stale handle.
*/
if (be16_to_cpu(dip->di_core.di_magic) != XFS_DINODE_MAGIC) {
- xfs_idestroy(ip);
- xfs_trans_brelse(tp, bp);
#ifdef DEBUG
xfs_fs_cmn_err(CE_ALERT, mp, "xfs_iread: "
"dip->di_core.di_magic (0x%x) != "
@@ -908,7 +902,8 @@ xfs_iread(
be16_to_cpu(dip->di_core.di_magic),
XFS_DINODE_MAGIC);
#endif /* DEBUG */
- return XFS_ERROR(EINVAL);
+ error = XFS_ERROR(EINVAL);
+ goto out_brelse;
}

/*
@@ -922,14 +917,12 @@ xfs_iread(
xfs_dinode_from_disk(&ip->i_d, &dip->di_core);
error = xfs_iformat(ip, dip);
if (error) {
- xfs_idestroy(ip);
- xfs_trans_brelse(tp, bp);
#ifdef DEBUG
xfs_fs_cmn_err(CE_ALERT, mp, "xfs_iread: "
"xfs_iformat() returned error %d",
error);
#endif /* DEBUG */
- return error;
+ goto out_brelse;
}
} else {
ip->i_d.di_magic = be16_to_cpu(dip->di_core.di_magic);
@@ -995,6 +988,12 @@ xfs_iread(
xfs_trans_brelse(tp, bp);
*ipp = ip;
return 0;
+
+ out_brelse:
+ xfs_trans_brelse(tp, bp);
+ out_destroy_inode:
+ xfs_destroy_inode(ip);
+ return error;
}

/*
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-21 09:24:42.000000000 +0200
@@ -309,6 +309,11 @@ static inline struct inode *VFS_I(struct
return &ip->i_vnode;
}

+static inline void xfs_destroy_inode(struct xfs_inode *ip)
+{
+ return destroy_inode(VFS_I(ip));
+}
+
/*
* i_flags helper functions
*/
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-21 09:26:38.000000000 +0200
@@ -2798,13 +2798,29 @@ 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 we get an uninitialized inode, immediate destroy it here and
+ * don't go through writeback or removing it from the radix-tree to
+ * which is has never been added.
+ *
+ * Note that I_NEW is a rather fragile way to detect this as I_NEW
+ * is still set by partially set-up inode that has been added to the
+ * radix-tree. But all those failure cases go through xfs_iput_new
+ * and thus never end up with I_NEW set here.
+ */
+ if (unlikely(inode->i_state & I_NEW)) {
+ xfs_idestroy(ip);
+ return 0;
+ }

/* bad inode, get out here ASAP */
- if (VN_BAD(VFS_I(ip))) {
+ if (unlikely(is_bad_inode(inode))) {
xfs_ireclaim(ip);
return 0;
}