Date: Thu, 29 Aug 2013 19:43:34 -0400 (EDT) From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-fs <freebsd-fs@freebsd.org> Subject: Re: fixing "umount -f" for the NFS client Message-ID: <537646864.15457428.1377819814825.JavaMail.root@uoguelph.ca> In-Reply-To: <20130829223128.GP4972@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Kostik wrote: > On Thu, Aug 29, 2013 at 06:21:41PM -0400, Rick Macklem wrote: > > Kostik wrote: > > > On Wed, Aug 28, 2013 at 08:15:27PM -0400, Rick Macklem wrote: > > > > I've been doing a little more testing of "umount -f" for NFS > > > > mounts and they seem to be working unless some other > > > > process/thread > > > > has busied the file system via vfs_busy(). > > > > > > > > Unfortunately, it is pretty easy to vfs_busy() the file system > > > > by using a command like "df" that is stuck on the unresponsive > > > > NFS server. > > > > > > > > The problem seems to be that dounmount() msleep()s while > > > > mnt_lockref != 0 before calling VFS_UNMOUNT(). > > > > > > > > If some call into the NFS client was done before this > > > > while (mp->mnt_lockref) loop with msleep() in it, it > > > > can easily kill off RPCs in progress. (It currently > > > > does this in nfs_unmount() using the newnfs_nmcancelreqs() > > > > call. > > > > > > > > In summary: > > > > - Would it be appropriate to add a new vfs_XXX method that > > > > dounmount() would call before the while() loop for the > > > > forced dismount case? > > > > (The default would be a no-op and I have no idea if any > > > > file system other than NFS would have a use for it?) > > > > Alternately, there could be a function pointer set non-NULL > > > > that would specifically be used by the NFS client for this. > > > > This would avoid adding a vfs_XXX() method, but would mean > > > > an NFS specific call ends up in the generic dounmount() code. > > > > > > > > Anyone have comments on this? > > > > > > > Yes, I do. I agree with adding the pre-unmount vfs method. > > > This seems to be the cleanest solution possible. > > > > > I've attached a patch. It is also at > > http://people.freebsd.org/~rmacklem/forced-dism.patch > > in case the attachment gets lost. > > I don't really like doing the MNT_IUNLOCK(), MNT_ILOCK() > > before/after > > the VFS_KILLIO() call, but I couldn't see any better way to do it > > and > > it looks safe to do so, at least for the forced case. > Might be, call it VFS_PURGE() ? > Sure, any name is fine with me. > I suggest to move the call to the VFS_KILLIO after the MNTK_DRAINING > is > set, to avoid getting new references after the current i/o > transactions > are stopped. You would need to set MNTK_DRAINING unconditionally. > Also, > it probably makes sense to replace the if (mnt_lockref) with while > (). > Hmm. When I look at the code, the only use of MNTK_DRAINING seems to be to tell vfs_unbusy() to do a wakeup() if mnt_lockref == 0. I don't see why setting it before VFS_PURGE() would matter? Let me explain what the NFS client does: If is sees MNTK_UNMOUNTF set, it fails VOP/VFS calls without attempting any RPCs. That's why I needed MNTK_UNMOUNTF set before the VFS_PURGE() call. The VFS_PURGE() call causes any RPC that is already in progress to fail (by closing the connection to the server). If there is a case where an RPC attempt can get stuck after this point, it's a bug in the NFS client I will need to find;-) --> Once MNTK_UNMOUNTF is set and VFS_PURGE() is called, all VOP/VFS ops should return failure without attempting to do RPCs against the server. If some thread does vfs_busy() while VFS_PURGE() is in progress or before (any time MNT_ILOCK() isn't held) it should end up doing a vfs_unbusy() at some point without getting stuck trying to do an RPC against the server. If this happens before dounmount() re-acquires the MNT_ILOCK(), it should be ok, since mnt_lockref has been decremented. If it does this after the dounmount() thread re-acquires MNT_ILOCK(), dounmount() should be in the msleep() with MNTK_DRAINING set, so it will get the wakeup once mnt_lockref has decremented to 0. Setting MNTK_DRAINING sooner would just result in the odd unnecessary wakeup(), from what I can see? > > > > I assume I would also need to bump __FreeBSD_version (and maybe > > VFS_VERSION?). > I think you could avoid it. > Do you mean I don't need to bump __FreeBSD_version or VFS_VERSION or both? Thanks, rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?537646864.15457428.1377819814825.JavaMail.root>