Date: Tue, 22 Sep 1998 12:05:22 -0400 (EDT) From: Luoqi Chen <luoqi@watermarkgroup.com> To: Don.Lewis@tsc.tdk.com, current@FreeBSD.ORG, luoqi@watermarkgroup.com Cc: mckusick@flamingo.McKusick.COM Subject: Re: Yet another patch to try for softupdates panic Message-ID: <199809221605.MAA01306@lor.watermarkgroup.com>
next in thread | raw e-mail | index | archive | help
> Yes, I read the comments in the code and understand why the locking must
> be done that way, and I also understand why this new code is needed when
> a user process calls fsync() for a file that exists on a filesystem using
> softupdates. What's bogus is that this new code changes the semantics of
> VOP_FSYNC(). The fact that VOP_FSYNC() may now unlock the vnode for a while
> then lock it again may break other parts of the kernel that expect the
> vnode to remain locked across a call VOP_FSYNC(). This change in semantics
> caused the directory truncation race that your patch fixed, although
> admittedly this particular call to VOP_FSYNC() was added to support
> softupdates. For another example, take a look at vinvalbuf(), which
> has the following code:
>
> /*
> * Flush out and invalidate all buffers associated with a vnode.
> * Called with the underlying object locked.
> */
> int
> vinvalbuf(vp, flags, cred, p, slpflag, slptimeo)
> [ snip ]
> if (vp->v_dirtyblkhd.lh_first != NULL) {
> splx(s);
> if ((error = VOP_FSYNC(vp, cred, MNT_WAIT, p)) != 0)
> return (error);
> s = splbio();
> if (vp->v_numoutput > 0 ||
> vp->v_dirtyblkhd.lh_first != NULL)
> panic("vinvalbuf: dirty bufs");
> }
> splx(s);
>
> It sure looks to me like if VOP_FSYNC() called ffs_fsync(), ffs_fsync()
> would first write all the dirty buffers and then call softdep_fsync().
> Softdep_fsync() would unlock the vnode in order to sync the parent
> directories, and while the vnode was unlocked, another process could
> grab the vnode and dirty its buffers. Softdep_fsync() would relock the
> vnode and return. When VOP_FSYNC() returns, we get a nice panic ...
>
Yes, there's a race here, and also a couple of other places VOP_FSYNC() are
called.
> You could tweak this call to VOP_FSYNC() to get it to avoid the
> call to softdep_fsync(), but how many other places in the kernel
> also need to be fixed? It may be better to only call softdep_fsync()
> from within the fsync() syscall handler. I don't know that any other
> users of VOP_FSYNC() need to ensure that the parent directories are
> pushed to disk.
>
I totally agree. This is the way to go. I'll try this and let you know
how it turns out.
-lq
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?199809221605.MAA01306>