Date: Tue, 22 Sep 1998 13:51:20 -0700 From: Kirk McKusick <mckusick@McKusick.COM> To: Luoqi Chen <luoqi@watermarkgroup.com> Cc: Don.Lewis@tsc.tdk.com, current@FreeBSD.ORG Subject: Re: Yet another patch to try for softupdates panic Message-ID: <199809222051.NAA14655@flamingo.McKusick.COM> In-Reply-To: Your message of "Tue, 22 Sep 1998 18:11:27 EDT." <199809222211.SAA04717@lor.watermarkgroup.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199809222051.NAA14655>