[REVIEW 3 of 4] Fix recalim handling in xfs_iget_core

[REVIEW 3 of 4] Fix recalim handling in xfs_iget_core

[David Chinner]

-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group


Fix the xfs_iget_core() handling of the XFS_IRECLAIMABLE flag so it
doesn't violate the guarantee we need to provide to xfs_iunpin()
w.r.t. the existence of a linux inode.

---
 fs/xfs/xfs_iget.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Index: 2.6.x-xfs-new/fs/xfs/xfs_iget.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_iget.c	2006-10-19 10:25:07.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_iget.c	2006-10-19 10:29:26.460972850 +1000
@@ -238,6 +238,34 @@ again:
 					goto again;
 				}
 
+				/*
+				 * If IRECLAIMABLE is set on this inode and lookup is
+				 * racing with unlink, then we should return an error
+				 * immediately so we don't remove it from the reclaim
+				 * list and potentially leak the inode.
+				 *
+				 * Also, there may be transactions sitting in the
+				 * incore log buffers or being flushed to disk at this
+				 * time.  We can't clear the XFS_IRECLAIMABLE flag
+				 * until these transactions have hit the disk,
+				 * otherwise we will void the guarantee the flag
+				 * provides xfs_iunpin()
+				 */
+				if (xfs_iflags_test(ip, XFS_IRECLAIMABLE)) {
+					if ((ip->i_d.di_mode == 0) &&
+					    !(flags & XFS_IGET_CREATE)) {
+						read_unlock(&ih->ih_lock);
+							return ENOENT;
+					}
+					if (xfs_ipincount(ip)) {
+						read_unlock(&ih->ih_lock);
+						xfs_log_force(mp, 0,
+							XFS_LOG_FORCE|XFS_LOG_SYNC);
+						XFS_STATS_INC(xs_ig_frecycle);
+						goto again;
+					}
+				}
+
 				vn_trace_exit(vp, "xfs_iget.alloc",
 					(inst_t *)__return_address);
 

Re: [REVIEW 3 of 4] Fix recalim handling in xfs_iget_core

[Shailendra Tripathi]

Hi David,
               I can't really see why we need this test:
                if (xfs_iflags_test(ip, XFS_IRECLAIMABLE))
                I think, An inode with no VP can be possibly in only 3 
states (NEW, RECLAIM or RECLAIMABLE). This check is being made inside 
(inode_vp == NULL)  check. If I am correct, may be we can omit an extra 
instruction here.
              It appears that you can see inode with XFS_ISTALE can 
potentially reach. I am not sure how it should reach that path. 
Following code just after this assumes that it must be in reclaimable path:
        
                                XFS_MOUNT_ILOCK(mp);
                                list_del_init(&ip->i_reclaim);
                                XFS_MOUNT_IUNLOCK(mp);
  
Regards,
Shailendra

David Chinner wrote:

Re: [REVIEW 3 of 4] Fix recalim handling in xfs_iget_core

[David Chinner]

On Tue, Oct 24, 2006 at 11:23:11AM -0700, Shailendra Tripathi wrote:
> Hi David,
>               I can't really see why we need this test:
>                if (xfs_iflags_test(ip, XFS_IRECLAIMABLE))
>                I think, An inode with no VP can be possibly in only 3 
> states (NEW, RECLAIM or RECLAIMABLE). This check is being made inside 
> (inode_vp == NULL)  check. If I am correct, may be we can omit an extra 
> instruction here.

We can't have an I_NEW here - that is checked even before we we try
to get the vnode. Hence we can be in either RECLAIM or RECLAIMABLE state.
Then we check RECLAIm and retry. So only reclaimable can get here.

Yes, I think you're right. Good catch. I'll replace the test
with an ASSERT.

>              It appears that you can see inode with XFS_ISTALE can 
> potentially reach. I am not sure how it should reach that path. 
> Following code just after this assumes that it must be in reclaimable path:
>        
>                                XFS_MOUNT_ILOCK(mp);
>                                list_del_init(&ip->i_reclaim);
>                                XFS_MOUNT_IUNLOCK(mp);

That's a no-op if it's not on the reclaim list (next and prev both
point to itself) so it should be safe even on non-IRECLAIMABLE
inodes.

Thanks, Shailendra.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group