From owner-freebsd-current  Tue Sep 22 15:32:23 1998
Return-Path: <owner-freebsd-current@FreeBSD.ORG>
Received: (from majordom@localhost)
          by hub.freebsd.org (8.8.8/8.8.8) id PAA18090
          for freebsd-current-outgoing; Tue, 22 Sep 1998 15:32:23 -0700 (PDT)
          (envelope-from owner-freebsd-current@FreeBSD.ORG)
Received: from flamingo.McKusick.COM (flamingo.mckusick.com [209.31.233.178])
          by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id PAA18031
          for <current@freebsd.org>; Tue, 22 Sep 1998 15:31:52 -0700 (PDT)
          (envelope-from mckusick@flamingo.McKusick.COM)
Received: from flamingo.McKusick.COM (mckusick@localhost [127.0.0.1])
	by flamingo.McKusick.COM (8.8.5/8.8.5) with ESMTP id NAA14655;
	Tue, 22 Sep 1998 13:51:21 -0700 (PDT)
Message-Id: <199809222051.NAA14655@flamingo.McKusick.COM>
To: Luoqi Chen <luoqi@watermarkgroup.com>
Subject: Re: Yet another patch to try for softupdates panic 
cc: Don.Lewis@tsc.tdk.com, current@FreeBSD.ORG
In-reply-to: Your message of "Tue, 22 Sep 1998 18:11:27 EDT."
             <199809222211.SAA04717@lor.watermarkgroup.com> 
Date: Tue, 22 Sep 1998 13:51:20 -0700
From: Kirk McKusick <mckusick@McKusick.COM>
Sender: owner-freebsd-current@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

	Date: Tue, 22 Sep 1998 18:11:27 -0400 (EDT)
	From: Luoqi Chen <luoqi@watermarkgroup.com>
	To: luoqi@watermarkgroup.com, mckusick@McKusick.COM
	Subject: Re: Yet another patch to try for softupdates panic
	Cc: Don.Lewis@tsc.tdk.com, current@freebsd.org

	> In case (2), the fsync() syscall does call VOP_FSYNC with MNT_WAIT
	> and you most definitely *must* write out the parent directory
	> entries! When the fsync system call returns, it ensures that
	> the entire file, including its name, is on stable storage.
	> 
	> 	Kirk McKusick
	> 
	That's not what I see in FreeBSD's code. Here is the fsync() function
	copied from kern/vfs_syscalls.c [rev1.106],

int
fsync(p, uap)
	struct proc *p;
	struct fsync_args /* {
		syscallarg(int) fd;
	} */ *uap;
{
	register struct vnode *vp;
	struct file *fp;
	int error;

	if (error = getvnode(p->p_fd, SCARG(uap, fd), &fp))
		return (error);
	vp = (struct vnode *)fp->f_data;
	if ((error = vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p)) == NULL) {
		if (vp->v_object) {
			vm_object_page_clean(vp->v_object, 0, 0, FALSE);
		}
		if (vp->v_mount && (vp->v_mount->mnt_flag & MNT_SOFTDEP)) {
			error = VOP_FSYNC(vp, fp->f_cred, MNT_LAZY, p);
							  ^^^^^^^^
		} else {
			error = VOP_FSYNC(vp, fp->f_cred,
				(vp->v_mount && (vp->v_mount->mnt_flag & MNT_ASY
NC)) ?
				MNT_NOWAIT : MNT_WAIT, p);
		}
		VOP_UNLOCK(vp, 0, p);

		if ((vp->v_mount->mnt_flag & MNT_SOFTDEP) && bioops.io_sync)
			(*bioops.io_sync)(NULL);
	}
	return (error);
}

	Ok, I did a little digging through the CVS log, the waitfor flag
	was changed to MNT_LAZY by John Dyson in March with the following
	log message:

	    Correct a significant problem with the softupdates
	    port.  Allow fsync to work properly within the softupdates
	    framework, and thereby eliminate some unfortunate panics.

	Does anyone know what significant problem John was refering to?

	-lq

That is very strange. The code that I put in was:

int
fsync(p, uap)
	struct proc *p;
	struct fsync_args /* {
		syscallarg(int) fd;
	} */ *uap;
{
	register struct vnode *vp;
	struct file *fp;
	int error;

	if (error = getvnode(p->p_fd, SCARG(uap, fd), &fp))
		return (error);
	vp = (struct vnode *)fp->f_data;
	if ((error = vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p)) == NULL) {
		if (vp->v_object) {
			vm_object_page_clean(vp->v_object, 0, 0, FALSE);
		}
		error = VOP_FSYNC(vp, fp->f_cred,
			(vp->v_mount && (vp->v_mount->mnt_flag & MNT_ASYNC)) ? 
			MNT_NOWAIT : MNT_WAIT, p);
		VOP_UNLOCK(vp, 0, p);
	}
	return (error);
}

The whole point of adding softdep_fsync was specifically to make sure
that the name got sync'ed when the user did an fsync syscall. Also the
addition of a call to bioops.io_sync is totally gratuitous and indeed
will cause terrible performance problems if fsyncs occur too rapidly.
I really wish that people that make changes that affect the soft update
code would at least send me the diffs before they commit them so I can
catch blatent errors like the change you sent me. Sigh.

I do think that you are moving in the right direction towards resolving
the problem. Specifically, I think that what we need to do is to break
softdep_fsync out of VOP_FSYNC and call it separately in those places
where it is really needed and desired. That way VOP_FSYNC will not 
violate the locking rule on the vnode with which it is called. I am
plowing through the list that you made to check whether there are any
other cases beside the fsync system call where softdep_fsync is needed.

	Kirk McKusick

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message