Date: Fri, 30 Sep 2011 11:20:28 -0700 From: Kirk McKusick <mckusick@mckusick.com> To: Attilio Rao <attilio@freebsd.org> Cc: Garrett Cooper <yanegomi@gmail.com>, freebsd-fs@freebsd.org, Konstantin Belousov <kib@freebsd.org>, Xin LI <delphij@freebsd.org> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems? Message-ID: <201109301820.p8UIKSGj039445@chez.mckusick.com> In-Reply-To: <CAJ-FndBDzv6af%2BAVq9wkLbN8V3wHDk3BGPuTFaXB7j8EXsrrhA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Fri, 30 Sep 2011 15:31:56 +0200
> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
> From: Attilio Rao <attilio@freebsd.org>
> To: Kirk McKusick <mckusick@mckusick.com>
> Cc: Konstantin Belousov <kib@freebsd.org>,
> Garrett Cooper <yanegomi@gmail.com>,
> freebsd-fs@freebsd.org, Xin LI <delphij@freebsd.org>
>
> 2011/9/30 Kirk McKusick <mckusick@mckusick.com>:
>
> > Here is my proposed fix. It does the unroll originally found in the
> > non-FORCE case before sleeping waiting for the vfs_busy to clear.
> > Is it acceptable to hold the mount mutex while calling VOP_UNLOCK?
> > If not, then it needs to be released before the unlock, reacquired
> > afterwards, and the check to see if the sleep is needed redone.
>
> I thought about this approach when sending the e-mail, but there is a
> problem: you need to handle the MNTK_UNMOUNT flag checking and
> subsequent setting after coveredvnode is held, otherwise at the first
> looping you will just return EBUSY.
>
> You can avoid the unlock by passing PVFS | PDROP.
>
> Attilio
Problem noted. I have updated the patch to clear the MNTK_UNMOUNT
(and other flags set above it) after it returns from the sleep.
Which means I cannot use the PDROP flag now, but it is good to
know about it for future reference.
Still not clear to me if it is acceptable to hold the mount mutex
while calling VOP_UNLOCK. Should I drop the mount mutex around the
VOP_UNLOCK(coveredvp)? Other than that possible problem, this patch
appears to solve the EBUSY problem and avoid possible deadlocks.
Kirk McKusick
Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c (revision 225884)
+++ 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?201109301820.p8UIKSGj039445>