PR kern/161016 Need to force sync(2) before umounting UFS1 filesystems?

[Garrett Cooper]

On Wed, Sep 28, 2011 at 6:36 PM, Kostik Belousov <kostikbel at gmail.com> wrote:
> On Tue, Sep 27, 2011 at 05:19:31PM -0700, Kirk McKusick wrote:
>> > Date: Sun, 25 Sep 2011 12:07:18 -0700
>> > From: Garrett Cooper <yanegomi at gmail.com>
>> > To: lev at freebsd.org
>> > Cc: freebsd-fs at freebsd.org, Xin LI <delphij at freebsd.org>, current at freebsd.org
>> > Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
>> >
>> > 2011/9/25 Lev Serebryakov <lev at freebsd.org>:
>> > > Hello, Garrett.
>> > > You wrote 25 =D3=C5=CE=D4=D1=C2=D2=D1 2011 =C7., 12:06:05:
>> > >
>> > >> =9A =9A Talking to Xin yesterday, he was convinced that this was a
>> > >> filesystem//kern bug. Before I file a PR, I'm wondering if anyone else
>> > >> has seen this issue..
>> > > =9AYes, and I posted message about it in embedded@ (Message-ID
>> > > <1175277342.20110821215629 at serebryakov.spb.ru>), I've got additional
>> > > question from Warner Losh about base (underlying) file system, without
>> > > any additional reaction.
>> >
>> > Thanks for the comments Adrian and Lev! I've filed PR 161016 to track
>> > the issue, because it might be due to changes in the SU code, md, or a
>> > subtle race condition in umount (highly unlikely, but it's been
>> > noted).
>> > -Garrett
>> > _______________________________________________
>> > freebsd-fs at freebsd.org mailing list
>> > http://lists.freebsd.org/mailman/listinfo/freebsd-fs
>> > To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
>>
>> I have taken responsibility for working on this bug report (PR kern/161016).
>>
>> I propose the following change to correct it:
>>
>> Index: sys/kern/vfs_mount.c
>> ===================================================================
>> --- sys/kern/vfs_mount.c      (revision 225807)
>> +++ sys/kern/vfs_mount.c      (working copy)
>> @@ -1227,18 +1227,6 @@
>>               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);
>> -             }
>>               mp->mnt_kern_flag |= MNTK_DRAINING;
>>               error = msleep(&mp->mnt_lockref, MNT_MTX(mp), PVFS,
>>                   "mount drain", 0);
>>
>> The things to check for are:
>>
>> 1) That it fixes the EBUSY on unmount.
>>
>> 2) That it does not cause unmount to hang.
>>
>> I would appreciate feedback as to whether this fix helps.
>
> I think the item 2) should be tested mostly on the hung NFS server.
>
> I understand what you are doing, you do not want a transient mount point
> busy caller to fail the unmount. But my belief is that this is the
> intended mode of operation for non-forced unmounts.
>
> As I compare the original bug report and your change, the reason that
> UFS gives spurious EBUSY on soft unmounts is that SU code busies mp
> around some processing. Is my guess right ? Then, restoring some amount
> of sync(2) before the unmount would be useful, please see r222466 for
> the most likely reason why the issue appeared.
>
> Might be, the best route would be to add a kludge mnt_flag that request
> dounmount() to do a VFS_SYNC() before checking for the busy holder ?

This would undo some of the changes attillio added for the locking
stuff in r184554. Not sure what the prior behavior was because I only
traced back the change a little bit.
Thanks,
-Garrett

1. http://svnweb.freebsd.org/base/head/sys/kern/vfs_mount.c?revision=184554&view=markup