VFS_SYNC() call in dounmount()

[Kirk McKusick]

> Date: Mon, 8 Dec 2014 20:19:06 +0200
> From: Konstantin Belousov <kostikbel at gmail.com>
> To: fs at freebsd.org
> Subject: VFS_SYNC() call in dounmount()
> 
> Right now, dounmount() has the following code:
> 	if (((mp->mnt_flag & MNT_RDONLY) ||
> 	     (error = VFS_SYNC(mp, MNT_WAIT)) == 0) || (flags & MNT_FORCE) != 0)
> 		error = VFS_UNMOUNT(mp, flags);
> In other words, if the filesystem is mounted rw, we try VFS_SYNC().
> If the unmount request if forced, VFS_UNMOUNT() is called unconditionally,
> otherwise, VFS_UNMOUNT() is only performed when VFS_SYNC() succeeded.
> 
> Apparently, the sync call is problematic, both for UFS and NFS. It
> was demonstrated by Peter Holm that sufficient fsx load prevents sync
> from finishing for unbounded amount of time. The ffs_unmount() makes
> neccessary measures to stop writers and to sync filesystem to the stable
> state before destroying the mount structures, so removal of VFS_SYNC()
> above fixed the test.
> 
> More, NFS client just ignores the VFS_SYNC() call for forced unmount,
> to work around the hung nfs requests.
> 
> Andrey Gapon assured me that ZFS handles unmount correctly and does
> not need help in the form of sync before unmount.  The only major
> writeable filesystem which apparently did not correctly synced on
> unmount is msdosfs.
> 
> Note that relying on VFS_SYNC() before VFS_UNMOUNT() to flush all caches
> to permanent storage is racy, since VFS does not (and cannot) stop
> other threads from writing to fs meantime.  UFS and TMPFS suspend
> filesystem in VFS_UNMOUNT(), handling the race in VFS_UNMOUNT().
> 
> I propose to only call VFS_SYNC() before VFS_UNMOUNT() for non-forced
> unmount.  As I explained, the call for forced case is mostly pointless.
> For non-forced unmount, this is needed for KBI compatibility for NFS
> (not important), and to increase the chances of unmount succeedeing
> (again not important).  I still prefer to keep the call around for
> non-forced case for some time.
> 
> diff --git a/sys/fs/msdosfs/msdosfs_vfsops.c b/sys/fs/msdosfs/msdosfs_vfsops.c
> index 213dd00..d14cdef 100644
> --- a/sys/fs/msdosfs/msdosfs_vfsops.c
> +++ b/sys/fs/msdosfs/msdosfs_vfsops.c
> @@ -797,11 +797,15 @@ msdosfs_unmount(struct mount *mp, int mntflags)
>  	int error, flags;
> 
>  	flags = 0;
> -	if (mntflags & MNT_FORCE)
> +	error = msdosfs_sync(mp, MNT_WAIT);
> +	if ((mntflags & MNT_FORCE) != 0) {
>  		flags |= FORCECLOSE;
> +	} else if (error != 0) {
> +		return (error);
> +	}
>  	error = vflush(mp, 0, flags, curthread);
> -	if (error && error != ENXIO)
> -		return error;
> +	if (error != 0 && error != ENXIO)
> +		return (error);
>  	pmp = VFSTOMSDOSFS(mp);
>  	if ((pmp->pm_flags & MSDOSFSMNT_RONLY) == 0) {
>  		error = markvoldirty(pmp, 0);
> diff --git a/sys/kern/vfs_mount.c b/sys/kern/vfs_mount.c
> index c407699..b2b4969 100644
> --- a/sys/kern/vfs_mount.c
> +++ b/sys/kern/vfs_mount.c
> @@ -1305,8 +1305,8 @@ dounmount(mp, flags, td)
>  		}
>  		vput(fsrootvp);
>  	}
> -	if (((mp->mnt_flag & MNT_RDONLY) ||
> -	     (error = VFS_SYNC(mp, MNT_WAIT)) == 0) || (flags & MNT_FORCE) != 0)
> +	if ((mp->mnt_flag & MNT_RDONLY) != 0 || (flags & MNT_FORCE) != 0 ||
> +	    (error = VFS_SYNC(mp, MNT_WAIT)) == 0)
>  		error = VFS_UNMOUNT(mp, flags);
>  	vn_finished_write(mp);
>  	/*

I agree with your analysis and believe that your proposed change is
functionally correct for at least UFS.

	Kirk McKusick