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

[Garrett Cooper]

On Tue, 11 Oct 2011, Kirk McKusick wrote:

>> Date: Mon, 10 Oct 2011 19:12:59 -0700
>> From: Garrett Cooper <yanegomi at gmail.com>
>> To: Kostik Belousov <kostikbel at gmail.com>
>> Cc: Kirk McKusick <mckusick at mckusick.com>, Attilio Rao <attilio at freebsd.org>,
>>         Xin LI <delphij at freebsd.org>, freebsd-fs at freebsd.org
>> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
>>
>> 2011/10/10 Kostik Belousov <kostikbel at gmail.com>:
>>
>>> The real case to test is the NFS mount which is wedged due to
>>> hung/unresponsive NFS server. I have high suspect that the patch
>>> could introduce the unkillable hung unmount process.
>>
>>     It blocked, but I could ^C it perfectly fine. I tested it via:
>>
>> Setup:
>> 1. Started up FreeNAS 8.x image; it acquired an IP from my server with
>> dhcp-75.local.
>>
>> Test 1:
>> 1. mount -t nfs dhcp-75:/mnt/tank /mnt/nfs/ from my test workstation.
>> 2. Paused VM.
>> 3. umount /mnt/nfs (the command blocked).
>> 4. ^C.
>> 5. mount | grep /mnt/nfs showed nothing (it had unmounted).
>>
>> Test 2:
>> 1. mount -t nfs dhcp-75:/mnt/tank /mnt/nfs/ from my test workstation (blocked).
>> 2. Opened up another ssh session and cd'ed to /mnt/nfs .
>> 3. Paused VM.
>> 4. umount /mnt/nfs . It failed with EBUSY.
>> 5. mount | grep /mnt/nfs showed that it was still mounted, as expected.
>>
>>     So unless there are buffers still waiting to be written out to an
>> NFS share, or other reasons that would prevent the NFS share from
>> being fully released, I doubt the proposed behavior is really
>> different from previous versions of FreeBSD.
>> Thanks,
>> -Garrett
>
> Given the testing that has been done and our discussion about deadlocks,
> I believe that I should proceed to check in my originally proposed change.
> Notably the one that simply deleted the != MNT_FORCE conditional. However,
> there is no harm in using my revised version that releases the covered vnode before draining vfs_busy, and there might be some future case where that would be a necessary thing to do.
>
> Speak up if you think I should not proceed to check in this change.
> Also, let me know if you have thoughts on which version I should use.

 	I think the final version that you provided to me should be the 
one that's put through long-term soak testing because it appeared 
functionally sound based on my soak testing over the past couple days.
 	I personally wasn't able to unroot the concern that kib had with 
deadlocked unmounts via NFS.
Thanks!
-Garrett

Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c        (revision 226242)
+++ sys/kern/vfs_mount.c        (working copy)
@@ -1187,6 +1187,7 @@

         mtx_assert(&Giant, MA_OWNED);

+top:
         if ((coveredvp = mp->mnt_vnodecovered) != NULL) {
                 mnt_gen_r = mp->mnt_gen;
                 VI_LOCK(coveredvp);
@@ -1227,21 +1228,19 @@
                 mp->mnt_kern_flag |= MNTK_UNMOUNTF;
         error = 0;
         if (mp->mnt_lockref) {
-               if ((flags & MNT_FORCE) == 0) {
-                       mp->mnt_kern_flag &= ~(MNTK_UNMOUNT | 
MNTK_NOINSMNTQ |
-                           MNTK_UNMOUNTF);
-                       if (mp->mnt_kern_flag & MNTK_MWAIT) {
-                               mp->mnt_kern_flag &= ~MNTK_MWAIT;
-                               wakeup(mp);
-                       }
-                       MNT_IUNLOCK(mp);
-                       if (coveredvp)
-                               VOP_UNLOCK(coveredvp, 0);
-                       return (EBUSY);
+               if (mp->mnt_kern_flag & MNTK_MWAIT) {
+                       mp->mnt_kern_flag &= ~MNTK_MWAIT;
+                       wakeup(mp);
                 }
+               if (coveredvp)
+                       VOP_UNLOCK(coveredvp, 0);
                 mp->mnt_kern_flag |= MNTK_DRAINING;
                 error = msleep(&mp->mnt_lockref, MNT_MTX(mp), PVFS,
                     "mount drain", 0);
+               mp->mnt_kern_flag &= ~(MNTK_UNMOUNT | MNTK_NOINSMNTQ |
+                   MNTK_UNMOUNTF );
+               MNT_IUNLOCK(mp);
+               goto top;
         }
         MNT_IUNLOCK(mp);
         KASSERT(mp->mnt_lockref == 0,