Need to force sync(2) before umounting UFS1 filesystems?

[Kirk McKusick]

> Date: Mon, 3 Oct 2011 23:38:11 +0300
> From: Kostik Belousov <kostikbel at gmail.com>
> To: Attilio Rao <attilio at freebsd.org>
> Cc: Kirk McKusick <mckusick at mckusick.com>,
>     Garrett Cooper <yanegomi at gmail.com>,
>     freebsd-fs at freebsd.org, Xin LI <delphij at freebsd.org>
> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
> 
> On Sun, Oct 02, 2011 at 02:19:32AM +0200, Attilio Rao wrote:
> > I'm sorry if it wasn't clear in kib/my latest message, but we don't
> > need the coveredvnode unlocking logic because of the tegge's commit.
> >
> > I just think we should commit the change in policy Kirk initially
> > submitted + a comment on top of vfs_busy() explaining why the deadlock
> > with coveredvnode cannot happen.
> 
> Below is my take on the comment.

Sorry for going silent. I have just finished my travel to the EuroBSD
conference which is no small journey from Berkeley California :-)

I believe that your comment below nicely summarizes Tor's commit log
entry. It also convinces me that my original change is indeed correct.
Looked at another way, if my original fix was not correct then the
forced unmount case would also have been incorrect. So, I am glad
that we did this exercise.

Please go ahead and commit the comment (and MFC it to at least 9
if not 8 and 7 to which I believe it also applies).

I am awaiting further testing of my change from Garrett Cooper
before proceeding with checking it in to head.

	Kirk McKusick

=-=-=

commit 3981acdadcf4313dbdf813ec107f7bfbb4057d09
Author: Konstantin Belousov <kostik at pooma.home>
Date:   Mon Oct 3 23:33:06 2011 +0300

    Move parts of the commit log for r166167, where Tor explained the
    interaction between vnode locks and vfs_busy(), into comment.

diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
index 7eb619a..3d7735d 100644
--- a/sys/kern/vfs_subr.c
+++ b/sys/kern/vfs_subr.c
@@ -348,6 +348,38 @@ SYSINIT(vfs, SI_SUB_VFS, SI_ORDER_FIRST, vntblinit, NU=
LL);
 /*
  * Mark a mount point as busy. Used to synchronize access and to delay
  * unmounting. Eventually, mountlist_mtx is not released on failure.
+ *
+ * vfs_busy() is a custom lock, it can block the caller.
+ * vfs_busy() only sleeps if the unmount is active on the mount point.
+ * For a mountpoint mp, vfs_busy-enforced lock is before lock of any
+ * vnode belonging to mp.
+ *
+ * Lookup uses vfs_busy() to traverse mount points.
+ * root fs			var fs
+ * / vnode lock		A	/ vnode lock (/var)		D
+ * /var vnode lock	B	/log vnode lock(/var/log)	E
+ * vfs_busy lock	C	vfs_busy lock			F
+ *
+ * Within each file system, the lock order is C->A->B and F->D->E.
+ *
+ * When traversing across mounts, the system follows that lock order:
+ *
+ *        C->A->B
+ *		|
+ *	        +->F->D->E
+ *
+ * The lookup() process for namei("/var") illustrates the process:
+ *  VOP_LOOKUP() obtains B while A is held
+ *  vfs_busy() obtains a shared lock on F while A and B are held
+ *  vput() releases lock on B
+ *  vput() releases lock on A
+ *  VFS_ROOT() obtains lock on D while shared lock on F is held
+ *  vfs_unbusy() releases shared lock on F
+ *  vn_lock() obtains lock on deadfs vnode vp_crossmp instead of A.
+ *    Attempt to lock A (instead of vp_crossmp) while D is held would
+ *    violate the global order, causing deadlocks.
+ *
+ * dounmount() locks B while F is drained.
  */
 int
 vfs_busy(struct mount *mp, int flags)