Date: Mon, 08 Dec 2014 10:38:25 -0800 From: Kirk McKusick <mckusick@mckusick.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: fs@freebsd.org Subject: Re: VFS_SYNC() call in dounmount() Message-ID: <201412081838.sB8IcPSD012844@chez.mckusick.com> In-Reply-To: <20141208181906.GW97072@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Mon, 8 Dec 2014 20:19:06 +0200 > From: Konstantin Belousov <kostikbel@gmail.com> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201412081838.sB8IcPSD012844>