implement IHOLD/IRELE directly

PATCH In review Christoph Hellwig 3 participants 5 messages Jul 23, 2008 – Jul 24, 2008 XFS filesystem

lore

Share & tools

lore

Christoph Hellwig

author

2008-07-23 19:49 UTC #0 lore raw

PATCH 5/5 implement IHOLD/IRELE directly

Now that all direct calls to VN_HOLD/VN_RELE are gone we can implement IHOLD/IRELE directly.

For the IHOLD case also replace igrab with a direct increment of i_count because we are guaranteed to already have a live and referenced inode by the VFS. Also remove the vn_hold statistic because it's been rather meaningless for some time with most references done by other callers.

Signed-off-by Christoph Hellwig <[email protected]>

fs/xfs/linux-2.6/xfs_ksyms.c +0 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c
===================================================================
@@ -179,7 +179,6 @@ EXPORT_SYMBOL(uuid_getnodeuniq);
 EXPORT_SYMBOL(uuid_hash64);
 EXPORT_SYMBOL(uuid_is_nil);
 EXPORT_SYMBOL(uuid_table_remove);
-EXPORT_SYMBOL(vn_hold);
 
 #if defined(CONFIG_XFS_POSIX_ACL)
 EXPORT_SYMBOL(xfs_acl_vtoacl);

fs/xfs/linux-2.6/xfs_vnode.c +0 ~~-18~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.c
===================================================================
@@ -82,24 +82,6 @@ vn_ioerror(
 		xfs_do_force_shutdown(ip->i_mount, SHUTDOWN_DEVICE_REQ, f, l);
 }
 
-
-/*
- * Add a reference to a referenced vnode.
- */
-bhv_vnode_t *
-vn_hold(
-	bhv_vnode_t	*vp)
-{
-	struct inode	*inode;
-
-	XFS_STATS_INC(vn_hold);
-
-	inode = igrab(vp);
-	ASSERT(inode);
-
-	return vp;
-}
-
 #ifdef	XFS_INODE_TRACE
 
 /*

fs/xfs/linux-2.6/xfs_vnode.h +11 ~~-16~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.h
===================================================================
@@ -69,22 +69,17 @@ static inline int vn_count(bhv_vnode_t *
 	return atomic_read(&vp->i_count);
 }
 
-/*
- * Vnode reference counting functions (and macros for compatibility).
- */
-extern bhv_vnode_t	*vn_hold(bhv_vnode_t *);
-
-#if defined(XFS_INODE_TRACE)
-#define VN_HOLD(vp)		\
-	((void)vn_hold(vp),	\
-	  xfs_itrace_hold(XFS_I(vp), __FILE__, __LINE__, (inst_t *)__return_address))
-#define VN_RELE(vp)		\
-	  (xfs_itrace_rele(XFS_I(vp), __FILE__, __LINE__, (inst_t *)__return_address), \
-	   iput(vp))
-#else
-#define VN_HOLD(vp)		((void)vn_hold(vp))
-#define VN_RELE(vp)		(iput(vp))
-#endif
+#define IHOLD(ip) \
+do { \
+	atomic_inc(&(VFS_I(ip)->i_count)); \
+	xfs_itrace_hold((ip), __FILE__, __LINE__, (inst_t *)__return_address); \
+} while (0)
+
+#define IRELE(ip) \
+do { \
+	xfs_itrace_rele((ip), __FILE__, __LINE__, (inst_t *)__return_address); \
+	iput(VFS_I(ip)); \
+} while (0)
 
 static inline bhv_vnode_t *vn_grab(bhv_vnode_t *vp)
 {

fs/xfs/xfs_utils.h +0 -3

Index: linux-2.6-xfs/fs/xfs/xfs_utils.h
===================================================================
@@ -18,9 +18,6 @@
 #ifndef __XFS_UTILS_H__
 #define __XFS_UTILS_H__
 
-#define IRELE(ip)	VN_RELE(VFS_I(ip))
-#define IHOLD(ip)	VN_HOLD(VFS_I(ip))
-
 extern int xfs_truncate_file(xfs_mount_t *, xfs_inode_t *);
 extern int xfs_dir_ialloc(xfs_trans_t **, xfs_inode_t *, mode_t, xfs_nlink_t,
 				xfs_dev_t, cred_t *, prid_t, int,

Dave Chinner

2008-07-23 22:54 UTC #1 lore raw

Quoted 7 lines · Christoph Hellwig

Now that all direct calls to VN_HOLD/VN_RELE are gone we can implement IHOLD/IRELE directly.
For the IHOLD case also replace igrab with a direct increment of i_count because we are guaranteed to already have a live and referenced inode by the VFS. Also remove the vn_hold statistic because it's been rather meaningless for some time with most references done by other callers.

Seeing as you are changing from an igrab() to a straight atomic_inc(), can you put an:

ASSERT(atomic_read(&VFS_I(ip)->i_count) > 0)

into the IHOLD macro so we catch screwups in reference counting as quickly as possible?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

author

2008-07-24 06:21 UTC #2 lore raw

Quoted 9 lines · Dave Chinner

Quoted 7 lines · Christoph Hellwig
Now that all direct calls to VN_HOLD/VN_RELE are gone we can implement IHOLD/IRELE directly.
For the IHOLD case also replace igrab with a direct increment of i_count because we are guaranteed to already have a live and referenced inode by the VFS. Also remove the vn_hold statistic because it's been rather meaningless for some time with most references done by other callers.

Quoted 7 lines

Seeing as you are changing from an igrab() to a straight atomic_inc(), can you put an:
ASSERT(atomic_read(&VFS_I(ip)->i_count) > 0)
into the IHOLD macro so we catch screwups in reference counting as quickly as possible?

Makes sense, updated patch below. (Half-way through xfsqa with a debug kernel so far)

Signed-off-by Christoph Hellwig <[email protected]>

fs/xfs/linux-2.6/xfs_ksyms.c +0 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c
===================================================================
@@ -179,7 +179,6 @@ EXPORT_SYMBOL(uuid_getnodeuniq);
 EXPORT_SYMBOL(uuid_hash64);
 EXPORT_SYMBOL(uuid_is_nil);
 EXPORT_SYMBOL(uuid_table_remove);
-EXPORT_SYMBOL(vn_hold);
 
 #if defined(CONFIG_XFS_POSIX_ACL)
 EXPORT_SYMBOL(xfs_acl_vtoacl);

fs/xfs/linux-2.6/xfs_vnode.c +0 ~~-18~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.c
===================================================================
@@ -82,24 +82,6 @@ vn_ioerror(
 		xfs_do_force_shutdown(ip->i_mount, SHUTDOWN_DEVICE_REQ, f, l);
 }
 
-
-/*
- * Add a reference to a referenced vnode.
- */
-bhv_vnode_t *
-vn_hold(
-	bhv_vnode_t	*vp)
-{
-	struct inode	*inode;
-
-	XFS_STATS_INC(vn_hold);
-
-	inode = igrab(vp);
-	ASSERT(inode);
-
-	return vp;
-}
-
 #ifdef	XFS_INODE_TRACE
 
 /*

fs/xfs/linux-2.6/xfs_vnode.h +12 ~~-16~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.h
===================================================================
@@ -69,22 +69,18 @@ static inline int vn_count(bhv_vnode_t *
 	return atomic_read(&vp->i_count);
 }
 
-/*
- * Vnode reference counting functions (and macros for compatibility).
- */
-extern bhv_vnode_t	*vn_hold(bhv_vnode_t *);
-
-#if defined(XFS_INODE_TRACE)
-#define VN_HOLD(vp)		\
-	((void)vn_hold(vp),	\
-	  xfs_itrace_hold(XFS_I(vp), __FILE__, __LINE__, (inst_t *)__return_address))
-#define VN_RELE(vp)		\
-	  (xfs_itrace_rele(XFS_I(vp), __FILE__, __LINE__, (inst_t *)__return_address), \
-	   iput(vp))
-#else
-#define VN_HOLD(vp)		((void)vn_hold(vp))
-#define VN_RELE(vp)		(iput(vp))
-#endif
+#define IHOLD(ip) \
+do { \
+	ASSERT(atomic_read(&VFS_I(ip)->i_count) > 0) ; \
+	atomic_inc(&(VFS_I(ip)->i_count)); \
+	xfs_itrace_hold((ip), __FILE__, __LINE__, (inst_t *)__return_address); \
+} while (0)
+
+#define IRELE(ip) \
+do { \
+	xfs_itrace_rele((ip), __FILE__, __LINE__, (inst_t *)__return_address); \
+	iput(VFS_I(ip)); \
+} while (0)
 
 static inline bhv_vnode_t *vn_grab(bhv_vnode_t *vp)
 {

fs/xfs/xfs_utils.h +0 -3

Index: linux-2.6-xfs/fs/xfs/xfs_utils.h
===================================================================
@@ -18,9 +18,6 @@
 #ifndef __XFS_UTILS_H__
 #define __XFS_UTILS_H__
 
-#define IRELE(ip)	VN_RELE(VFS_I(ip))
-#define IHOLD(ip)	VN_HOLD(VFS_I(ip))
-
 extern int xfs_truncate_file(xfs_mount_t *, xfs_inode_t *);
 extern int xfs_dir_ialloc(xfs_trans_t **, xfs_inode_t *, mode_t, xfs_nlink_t,
 				xfs_dev_t, cred_t *, prid_t, int,

Christoph Hellwig

author

2008-07-24 08:06 UTC #3 lore raw

Quoted 2 lines · Christoph Hellwig

Makes sense, updated patch below. (Half-way through xfsqa with a debug kernel so far)

xfsqa passed fine with this patch applied.

Lachlan McIlroy

2008-07-24 08:14 UTC #4 lore raw

Quoted 6 lines · Christoph Hellwig

Quoted 2 lines · Christoph Hellwig
Makes sense, updated patch below. (Half-way through xfsqa with a debug kernel so far)
xfsqa passed fine with this patch applied.

Great. All five patches look good to me. I'll get them into the tree tomorrow.