On Sep 21, 7:27pm, Luoqi Chen wrote: } Subject: Re: Yet another patch to try for softupdates panic } > It looks like this bogus unlocking could also affect vinvalbuf() which } > expects the vnode to be locked. } > } The unlock is not bogus at all. You have to maintain a strict locking order } to avoid deadlocks. For vnodes, this locking order is the directory tree } itself, i.e., you have to lock the parent first, then the child. So if you } hold the lock on the child and intend to lock the parent, you have to } release the lock on child first, then acquire the lock on the parent, and } then reacquire lock the the child. That's also the reason why you can't have } hardlinks to a directory.

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

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.

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