Date: Tue, 11 Oct 2011 01:01:59 -0700 (PDT) From: Garrett Cooper <yanegomi@gmail.com> To: Kirk McKusick <mckusick@mckusick.com> Cc: Garrett Cooper <yanegomi@gmail.com>, Xin LI <delphij@freebsd.org>, Attilio Rao <attilio@freebsd.org>, freebsd-fs@freebsd.org Subject: Re: Need to force sync(2) before umounting UFS1 filesystems? Message-ID: <alpine.BSF.2.00.1110110058270.4348@toaster.local> In-Reply-To: <201110110756.p9B7ul0g051037@chez.mckusick.com> References: <201110110756.p9B7ul0g051037@chez.mckusick.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 11 Oct 2011, Kirk McKusick wrote: >> Date: Mon, 10 Oct 2011 19:12:59 -0700 >> From: Garrett Cooper <yanegomi@gmail.com> >> To: Kostik Belousov <kostikbel@gmail.com> >> Cc: Kirk McKusick <mckusick@mckusick.com>, Attilio Rao <attilio@freebsd.org>, >> Xin LI <delphij@freebsd.org>, freebsd-fs@freebsd.org >> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems? >> >> 2011/10/10 Kostik Belousov <kostikbel@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,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1110110058270.4348>