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

[Kirk McKusick]

Hi Attilio,

I have been looking into the problem described below and since you
appear to be the person that put in the change in question, I would 
like to get you opinion on what (if anything) should be changed here.

A bit of background. Historically (i.e., since UNIX was written and
up until this change went in) the unmount command would always fully
sync out the filesystem and return with it unmounted. The exception 
was if some file or directory in the filesystem was actively being
used. In this case, the unmount would fail with EBUSY. After this
change, an unmount will fail with EBUSY if there are dirty blocks
that need to be synced, even if there are no active users of the
filesystem. This affects UFS (where the soft-updates code may be 
doing background tasks), NFS (where the NFS daemon may be doing
background tasks), and ZFS (where its syncer may be doing background
tasks). The only way to reliably unmount an idle filesystem is to loop 
doing sync's and unmount attempts until it succeeds. Now it is possible
to get the unmount to succeed by doing a forcible unmount, but that
is often not what is desired as that will kill any legitimate users
on the filesystem. My argument below is that we should revert to the
historic semantics of unmount which is to always succeed unless there
are active users on the filesystem.

So, please look over my suggested change and let me know what you
think.

	Kirk McKusick

=-=-=

Date: Thu, 29 Sep 2011 04:36:35 +0300
From: Kostik Belousov <kostikbel at gmail.com>
To: Kirk McKusick <mckusick at mckusick.com>
Cc: Garrett Cooper <yanegomi at gmail.com>, freebsd-fs at freebsd.org,
    Xin LI <delphij at freebsd.org>
Subject: Re: PR kern/161016 Need to force sync(2) before umounting UFS1 filesystems?

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 ?