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

[Garrett Cooper]

On Sat, Oct 1, 2011 at 5:39 AM, Attilio Rao <attilio at freebsd.org> wrote:
> 2011/9/30 Kostik Belousov <kostikbel at gmail.com>:
>> On Fri, Sep 30, 2011 at 11:20:28AM -0700, Kirk McKusick wrote:
>>> > Date: Fri, 30 Sep 2011 15:31:56 +0200
>>> > Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
>>> > From: Attilio Rao <attilio at freebsd.org>
>>> > To: Kirk McKusick <mckusick at mckusick.com>
>>> > Cc: Konstantin Belousov <kib at freebsd.org>,
>>> >     Garrett Cooper <yanegomi at gmail.com>,
>>> >     freebsd-fs at freebsd.org, Xin LI <delphij at freebsd.org>
>>> >
>>> > 2011/9/30 Kirk McKusick <mckusick at 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.
>> I do not understand which deadlock is talked about there.
>> It seems thay Attilio concern was with acquiring covered vnode lock
>> after mounted fs is busied, but this is prohibited.
>>
>> See r166167 for more detailed description of the order.
>
> Ok, so that is the invariant I was forgetting, thanks Kostik.
>
> Kirk, you can make the 'forced unmount' behaviour by default for me,
> now, thanks.
> It would be great to have a comment on top of vfs_busy() or
> dounmount() check of mnt_ref on why this deadlock cannot happen,
> likely squeezing some good words from tegge's description of r166167.
> Kirk may be the best person to do it, but I can have his backs if he
> doesn't have time right now.

Ok. Now that I know this is the direction you guys want to go, I'll
start testing the change.
Thanks!
-Garrett