Need to force sync(2) before umounting UFS1 filesystems?
Garrett Cooper
yanegomi at gmail.com
Tue Oct 11 08:02:03 UTC 2011
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,
More information about the freebsd-fs
mailing list