Skip to contentSkip to conversation

free inodes using destroy_inode

PATCH In review Christoph Hellwig 3 participants 5 messages Oct 20, 2008 – Oct 22, 2008

Subsystems (2)
VFS XFS filesystem
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 3/3 free inodes using destroy_inode

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.

Signed-off-by Christoph Hellwig <[email protected]>
fs/inode.c +1 -0
Index: xfs-2.6/fs/inode.c
===================================================================
@@ -212,6 +212,7 @@ void destroy_inode(struct inode *inode) 
 	else
 		kmem_cache_free(inode_cachep, (inode));
 }
+EXPORT_SYMBOL(destroy_inode);
 
 
 /*
fs/xfs/xfs_iget.c +1 -1
Index: xfs-2.6/fs/xfs/xfs_iget.c
===================================================================
@@ -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;
 }
fs/xfs/xfs_inode.c +13 -14
Index: xfs-2.6/fs/xfs/xfs_inode.c
===================================================================
@@ -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;
 }
 
 /*
fs/xfs/xfs_inode.h +6 -0
Index: xfs-2.6/fs/xfs/xfs_inode.h
===================================================================
@@ -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));
+}
+
 /*
  * i_flags helper functions
  */
fs/xfs/xfs_vnodeops.c +8 -2
Index: xfs-2.6/fs/xfs/xfs_vnodeops.c
===================================================================
@@ -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;
+	}
 
 	/* bad inode, get out here ASAP */
-	if (VN_BAD(VFS_I(ip))) {
+	if (unlikely(is_bad_inode(inode))) {
 		xfs_ireclaim(ip);
 		return 0;
 	}
#1 lore raw
Quoted 4 lines · Christoph Hellwig

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.

Quoted 5 lines

Index: xfs-2.6/fs/xfs/xfs_inode.c
===================================================================

fs/xfs/xfs_inode.c xfs_iread( · line 876

Quoted 9 lines
fs/xfs/xfs_inode.c +2 -4
@@ -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.

fs/xfs/xfs_inode.c xfs_iread( · line 889

Quoted 10 lines
fs/xfs/xfs_inode.c +2 -4
@@ -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.

fs/xfs/xfs_inode.h static inline struct inode *VFS_I(struct · line 316

Quoted 13 lines

Index: xfs-2.6/fs/xfs/xfs_inode.h
===================================================================

fs/xfs/xfs_inode.h +5 -0
@@ -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.

Quoted 9 lines

Index: xfs-2.6/fs/xfs/xfs_vnodeops.c
===================================================================

fs/xfs/xfs_vnodeops.c +1 -0
@@ -2798,13 +2798,19 @@ int
 xfs_reclaim(
 	xfs_inode_t	*ip)
 {
+	struct inode	*inode = VFS_I(ip);

fs/xfs/xfs_vnodeops.c int · line 2810

Quoted 10 lines
fs/xfs/xfs_vnodeops.c +6 -1
 
 	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.

Signature · 2 lines
Dave Chinner
[email protected]
#2 lore raw
Quoted 5 lines · Dave Chinner
   ^

Extra whitespace.

   ^

Ditto.

Fixed.

Quoted 7 lines

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 <[email protected]>
fs/inode.c +1 -0
Index: xfs-2.6/fs/inode.c
===================================================================
@@ -212,6 +212,7 @@ void destroy_inode(struct inode *inode) 
 	else
 		kmem_cache_free(inode_cachep, (inode));
 }
+EXPORT_SYMBOL(destroy_inode);
 
 
 /*
fs/xfs/xfs_iget.c +1 -1
Index: xfs-2.6/fs/xfs/xfs_iget.c
===================================================================
@@ -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;
 }
fs/xfs/xfs_inode.c +13 -14
Index: xfs-2.6/fs/xfs/xfs_inode.c
===================================================================
@@ -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;
 }
 
 /*
fs/xfs/xfs_inode.h +5 -0
Index: xfs-2.6/fs/xfs/xfs_inode.h
===================================================================
@@ -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
  */
fs/xfs/xfs_vnodeops.c +18 -2
Index: xfs-2.6/fs/xfs/xfs_vnodeops.c
===================================================================
@@ -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;
 	}
#3 lore raw
Quoted 17 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner
   ^

Extra whitespace.

   ^

Ditto.

Fixed.

Quoted 7 lines

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

Quoted 5 lines

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.

Hmmmm - I still don't see that as possible. We don't set I_NEW until we are inside xfs_setup_inode(), which occurs after we insert the inode into the radix tree. xfs_setup_inode() also calls unlock_new_inode(), so the I_NEW flag is cleared before it returns, too. So I can't really see how this check in reclaim does anything....

AFAICT, once we've inserted the new inode into the radix tree, we can't get an error before xfs_setup_inode() is called - even in the allocation case. Hence once we're in the radix tree, xfs_iput_new() should be called to cleanup.

All the cases that xfs_destroy_inode() handles are before the inode is inserted into the radix tree, so marking the XFS inode XFS_IBAD in xfs_destroy_inode() is probably a much more reliable way to detect immediate destroy cases in the reclaim code than relying on I_NEW......

Thoughts?

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
#4 lore raw
Quoted 11 lines · Dave Chinner

Hmmmm - I still don't see that as possible. We don't set I_NEW until we are inside xfs_setup_inode(), which occurs after we insert the inode into the radix tree. xfs_setup_inode() also calls unlock_new_inode(), so the I_NEW flag is cleared before it returns, too. So I can't really see how this check in reclaim does anything....

AFAICT, once we've inserted the new inode into the radix tree, we can't get an error before xfs_setup_inode() is called - even in the allocation case. Hence once we're in the radix tree, xfs_iput_new() should be called to cleanup.

Quoted 5 lines

All the cases that xfs_destroy_inode() handles are before the inode is inserted into the radix tree, so marking the XFS inode XFS_IBAD in xfs_destroy_inode() is probably a much more reliable way to detect immediate destroy cases in the reclaim code than relying on I_NEW......

I've put in some instrumentation and we indeed newer hit the I_NEW case. I also can't reproduce the problems I saw before anymore - in fact calling radix_tree_remove on an inode is explicitly fine per documentation and does work. I'll send a reworked patch that just uses make_inode_bad as this has passed xfsqa a few times now.

I've also removed patch 2 for now a it's not a bug fix an I'll put it into my later series that cleans up a lot of mess in that area.