Re: [PATCH] Fixing soft NFS umount -f, round 5

[Chuck Silvers <chuq%chuq.com@localhost>]

On Fri, Jul 10, 2015 at 03:43:54AM +0000, Emmanuel Dreyfus wrote:
> On Wed, Jul 08, 2015 at 06:07:11PM +0000, Emmanuel Dreyfus wrote:
> > http://ftp.espci.fr/shadow/manu/umount_f4.patch
> > 
> > With default values, timeout = 3 and retrans = 10, we now wait
> > for ages or the unmount to completes. In the umount -f case
> > it does not makes sense to wait for too long.
> 
> Here is an improved version that still use your proposed timeouts:
> http://ftp.espci.fr/shadow/manu/umount_f5.patch

the new variable "before_ts" should be time_t rather than int.

it looks like "nmp->nm_iflag" is supposed to be protected by "nmp->nm_lock",
you ought to use hold that lock while modifying nm_iflag in nfs_unmount().

otherwise the latest diff looks ok to me.


> I introduced a NFS mount private flag NFSMNT_DISMNTFORCE that informs
> the NFS subsystem that we want to forcibly unmount the filesystem, 
> which can be done at the expense of cutting timeout corners.
> 
> An important point is that with the flag set, we do not attempt any
> NFS server reconnect. It makes sense since dounmount() calls 
> VFS_SYNC() before VFS_UNMOUNT(). NFSMNT_DISMNTFORCE being set in 
> VFS_UNMOUNT() / nfs_unmount(), we already made reconnection attempts
> withinin VFS_SYNC(), hence thereis benefit for doint it again.
> 
> Now with that change, in the umount -f while NFS server is gone case,
> we spend 20s in VFS_SYNC(), and VFS_UNMOUNT() returns in less than a
> second.
> 
> Now I think it would be nice to also cut coners in VFS_SYNC() when 
> the force flag is used, but that touches filesystem-indpendent code,
> in dounmount():
>         if ((mp->mnt_flag & MNT_RDONLY) == 0) {
>                 error = VFS_SYNC(mp, MNT_WAIT, l->l_cred);
>         }       
>         if (error == 0 || (flags & MNT_FORCE)) {
>                 error = VFS_UNMOUNT(mp, flags);
>         }
> 
> The first VFS_SYNC() is making us wait even if MNT_FORCE is set. This could
> be solved by adding a IMNT_UMOUNTFORCE to struct mount's mnt_iflag, just
> like I did for NFS in this patch. The flag would instruct underlying 
> filesystem that force unmount is required and that fast return is expected.

I haven't kept up with the VFS-level locking changes in the last few years
so I'll let someone else comment about all that.

in particular, I don't know what (if anything) prevents vnodes from becoming
dirty again in between the VFS_SYNC() and the VFS_UNMOUNT().  every file system
driver appears to flush everything again (via vflush()) in its *_unmount()
method, so I don't know what benefit the VFS_SYNC() in dounmount() is providing.

-Chuck


> Opinions?
> 
> -- 
> Emmanuel Dreyfus
> manu%netbsd.org@localhost