Subject: Re: bloated code paths in ufs_lock/unlock
To: None <tech-kern@netbsd.org>
From: Juergen Hannken-Illjes <hannken@eis.cs.tu-bs.de>
List: tech-kern
Date: 04/21/2007 17:54:45
--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Apr 14, 2007 at 03:13:52PM +0100, David Laight wrote:
> I've just looked at the current versions of ufs_lock/unlock (in
> ufs/usf/ufs_vnops.c), they both contain the following conditional:
> 
>         if ((vp->v_type == VREG || vp->v_type == VDIR) &&
> 	    fstrans_is_owner(mp) &&
> 	    fstrans_getstate(mp) == FSTRANS_SUSPENDING) {
> 
> Now the first 2 tests are almost always true.
> 
> fstrans_is_owner() would be fairly cheap if IMNT_HAS_TRANS were clear on
> the mount - but it is always set for ffs, instead it has the loop:
> 
> 	for (fli = lwp_getspecific(lwp_data_key); fli; fli = fli->fli_succ)
> 		if (fli->fli_mount == mp)
> 			break;
> 
> fstrans_getstate() calls mount_getspecific(mp, mount_data_key)
> 
> I don't suppose either of the latter functions are intended to be
> called every time a vnode is locked/unlocked on the off chance that
> 'file system suspension' is in progess.

fstrans_is_owner() is not that expensive as the list in question is
usually short, one element per mount this thread is using.

Anyway, the attached diff speeds up `stat("/a/a/a", &sbuf)' by ~5%.

Comments?

-- 
Juergen Hannken-Illjes - hannken@eis.cs.tu-bs.de - TU Braunschweig (Germany)

--Nq2Wo0NMKNjxTN9z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ufslock.diff"

Index: ffs/ffs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.198
diff -p -u -2 -r1.198 ffs_vfsops.c
--- ffs/ffs_vfsops.c	7 Apr 2007 14:21:52 -0000	1.198
+++ ffs/ffs_vfsops.c	21 Apr 2007 15:39:07 -0000
@@ -1779,5 +1779,7 @@ ffs_suspendctl(struct mount *mp, int cmd
 		if ((error = fstrans_setstate(mp, FSTRANS_SUSPENDING)) != 0)
 			return error;
+		VFSTOUFS(mp)->um_flags |= UFS_SUSPENDING;
 		error = ffs_sync(mp, MNT_WAIT, l->l_proc->p_cred, l);
+		VFSTOUFS(mp)->um_flags &= ~UFS_SUSPENDING;
 		if (error == 0)
 			error = fstrans_setstate(mp, FSTRANS_SUSPENDED);
Index: ufs/ufs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.152
diff -p -u -2 -r1.152 ufs_vnops.c
--- ufs/ufs_vnops.c	4 Mar 2007 06:03:48 -0000	1.152
+++ ufs/ufs_vnops.c	21 Apr 2007 15:39:07 -0000
@@ -2335,4 +2335,13 @@ ufs_gop_markupdate(struct vnode *vp, int
 
 /*
+ * Fake vnode locking during file system suspension.
+ */
+#define UFS_FAKE_VNLOCK(vp) \
+	__predict_false((vp)->v_mount != NULL && \
+	    (VFSTOUFS((vp)->v_mount)->um_flags & UFS_SUSPENDING) != 0 && \
+	    ((vp)->v_type == VREG || (vp)->v_type == VDIR) && \
+	    fstrans_is_owner((vp)->v_mount))
+
+/*
  * Lock the node.
  */
@@ -2345,12 +2354,6 @@ ufs_lock(void *v)
 	} */ *ap = v;
 	struct vnode *vp = ap->a_vp;
-	struct mount *mp = vp->v_mount;
 
-	/*
-	 * Fake lock during file system suspension.
-	 */
-	if ((vp->v_type == VREG || vp->v_type == VDIR) &&
-	    fstrans_is_owner(mp) &&
-	    fstrans_getstate(mp) == FSTRANS_SUSPENDING) {
+	if (UFS_FAKE_VNLOCK(vp)) {
 		if ((ap->a_flags & LK_INTERLOCK) != 0)
 			simple_unlock(&vp->v_interlock);
@@ -2371,12 +2374,6 @@ ufs_unlock(void *v)
 	} */ *ap = v;
 	struct vnode *vp = ap->a_vp;
-	struct mount *mp = vp->v_mount;
 
-	/*
-	 * Fake unlock during file system suspension.
-	 */
-	if ((vp->v_type == VREG || vp->v_type == VDIR) &&
-	    fstrans_is_owner(mp) &&
-	    fstrans_getstate(mp) == FSTRANS_SUSPENDING) {
+	if (UFS_FAKE_VNLOCK(vp)) {
 		if ((ap->a_flags & LK_INTERLOCK) != 0)
 			simple_unlock(&vp->v_interlock);
Index: ufs/ufsmount.h
===================================================================
RCS file: /cvsroot/src/sys/ufs/ufs/ufsmount.h,v
retrieving revision 1.28
diff -p -u -2 -r1.28 ufsmount.h
--- ufs/ufsmount.h	4 Mar 2007 06:03:48 -0000	1.28
+++ ufs/ufsmount.h	21 Apr 2007 15:39:07 -0000
@@ -137,4 +137,5 @@ struct ufs_ops {
 #define UFS_NEEDSWAP	0x01	/* filesystem metadata need byte-swapping */
 #define UFS_ISAPPLEUFS	0x02	/* filesystem is Apple UFS */
+#define UFS_SUSPENDING	0x04	/* filesystem is suspending */
 
 /*

--Nq2Wo0NMKNjxTN9z--