Re: kqueue LOR

On Wed, Dec 13, 2006 at 04:12:57AM +0000, Tor Egge wrote:
> > Hmm, may be, since vnode must be interlocked by ffs_sync() after
> > MNTK_SUSPENDED set, and before MNTK_SUSPEND, mount interlock is not
> > needed in ufs_itimes.
> > 
> > Tor ?

> If neither IN_CHANGE nor IN_UPDATE is set then it might be unsafe
> to set IN_MODIFIED in ufs_itimes() since the file system might be
> suspended or in the process of being suspended with the vnode sync
> loop in ffs_sync() having iterated past the vnode.
This was exactly the reason for IN_LAZYACCESS flag introduction. It is
set when fs is suspending or suspended, and no IN_CHANGE or IN_UPDATE
flags are set.

> I don't think the mount interlock is needed to check for MNTK_SUSPEND
> being set in ufs_itimes() as long as the vnode interlock is held. If
> a stale value is read without MNTK_SUSPEND set then the vnode sync
> loop in ffs_sync() cannot have iterated past the vnode, thus it should
> still be safe to set IN_MODIFIED. All writes by the CPU performing
> the vnode sync loop before it released the vnode interlock for the
> same vnode should be visible to the CPU in ufs_itimes() after it has
> obtained the vnode interlock.

I think that you statement is valid for both MNTK_SUSPEND and
MNTK_SUSPENDED flags. In other words, aquision of mount interlock could
be safely removed from the ufs_itimes(), as was suggested by ssouhlal_at_.

Index: ufs/ufs/ufs_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
retrieving revision 1.283
diff -u -r1.283 ufs_vnops.c
--- ufs/ufs/ufs_vnops.c	6 Nov 2006 13:42:09 -0000	1.283
+++ ufs/ufs/ufs_vnops.c	13 Dec 2006 14:10:31 -0000
_at__at_ -133,19 +133,15 _at__at_
 {
 	struct inode *ip;
 	struct timespec ts;
-	int mnt_locked;
 
 	ip = VTOI(vp);
-	mnt_locked = 0;
-	if ((vp->v_mount->mnt_flag & MNT_RDONLY) != 0) {
-		VI_LOCK(vp);
+	VI_LOCK(vp);
+	if ((vp->v_mount->mnt_flag & MNT_RDONLY) != 0)
 		goto out;
+	if ((ip->i_flag & (IN_ACCESS | IN_CHANGE | IN_UPDATE)) == 0) {
+		VI_UNLOCK(vp);
+		return;
 	}
-	MNT_ILOCK(vp->v_mount);		/* For reading of mnt_kern_flags. */
-	mnt_locked = 1;
-	VI_LOCK(vp);
-	if ((ip->i_flag & (IN_ACCESS | IN_CHANGE | IN_UPDATE)) == 0)
-		goto out_unl;
 
 	if ((vp->v_type == VBLK || vp->v_type == VCHR) && !DOINGSOFTDEP(vp))
 		ip->i_flag |= IN_LAZYMOD;
_at__at_ -172,10 +168,7 _at__at_
 
  out:
 	ip->i_flag &= ~(IN_ACCESS | IN_CHANGE | IN_UPDATE);
- out_unl:
 	VI_UNLOCK(vp);
-	if (mnt_locked)
-		MNT_IUNLOCK(vp->v_mount);
 }
 
 /*

[ It seems that MNTK_SUSPENDED flag implies MNTK_SUSPEND. ]