From owner-freebsd-fs@FreeBSD.ORG Mon Dec 8 18:38:41 2014 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 70978F97 for ; Mon, 8 Dec 2014 18:38:41 +0000 (UTC) Received: from chez.mckusick.com (chez.mckusick.com [IPv6:2001:5a8:4:7e72:4a5b:39ff:fe12:452]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 51420DD9 for ; Mon, 8 Dec 2014 18:38:41 +0000 (UTC) Received: from chez.mckusick.com (localhost [127.0.0.1]) by chez.mckusick.com (8.14.3/8.14.3) with ESMTP id sB8IcPSD012844; Mon, 8 Dec 2014 10:38:25 -0800 (PST) (envelope-from mckusick@chez.mckusick.com) Message-Id: <201412081838.sB8IcPSD012844@chez.mckusick.com> To: Konstantin Belousov Subject: Re: VFS_SYNC() call in dounmount() In-reply-to: <20141208181906.GW97072@kib.kiev.ua> Date: Mon, 08 Dec 2014 10:38:25 -0800 From: Kirk McKusick Cc: fs@freebsd.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Dec 2014 18:38:41 -0000 > Date: Mon, 8 Dec 2014 20:19:06 +0200 > From: Konstantin Belousov > To: fs@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