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