Date: Thu, 29 Aug 2013 18:21:41 -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: <2075428996.15437999.1377814901677.JavaMail.root@uoguelph.ca> In-Reply-To: <20130829005616.GH4972@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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. I assume I would also need to bump __FreeBSD_version (and maybe VFS_VERSION?). Please review it. If anyone would like to test it, please do so. Thanks, rick [-- Attachment #2 --] --- sys/mount.h.sav 2013-08-29 15:19:40.000000000 -0400 +++ sys/mount.h 2013-08-29 15:23:59.000000000 -0400 @@ -609,6 +609,7 @@ typedef int vfs_sysctl_t(struct mount *m struct sysctl_req *req); typedef void vfs_susp_clean_t(struct mount *mp); typedef void vfs_notify_lowervp_t(struct mount *mp, struct vnode *lowervp); +typedef void vfs_killio_t(struct mount *mp); struct vfsops { vfs_mount_t *vfs_mount; @@ -628,6 +629,7 @@ struct vfsops { vfs_susp_clean_t *vfs_susp_clean; vfs_notify_lowervp_t *vfs_reclaim_lowervp; vfs_notify_lowervp_t *vfs_unlink_lowervp; + vfs_killio_t *vfs_killio; }; vfs_statfs_t __vfs_statfs; @@ -756,6 +758,14 @@ vfs_statfs_t __vfs_statfs; } \ } while (0) +#define VFS_KILLIO(MP) do { \ + if (*(MP)->mnt_op->vfs_killio != NULL) { \ + VFS_PROLOGUE(MP); \ + (*(MP)->mnt_op->vfs_killio)(MP); \ + VFS_EPILOGUE(MP); \ + } \ +} while (0) + #define VFS_KNOTE_LOCKED(vp, hint) do \ { \ if (((vp)->v_vflag & VV_NOKNOTE) == 0) \ --- kern/vfs_mount.c.sav 2013-08-29 15:24:36.000000000 -0400 +++ kern/vfs_mount.c 2013-08-29 17:22:51.000000000 -0400 @@ -1269,8 +1269,16 @@ dounmount(mp, flags, td) } mp->mnt_kern_flag |= MNTK_UNMOUNT | MNTK_NOINSMNTQ; /* Allow filesystems to detect that a forced unmount is in progress. */ - if (flags & MNT_FORCE) + if (flags & MNT_FORCE) { mp->mnt_kern_flag |= MNTK_UNMOUNTF; + MNT_IUNLOCK(mp); + /* + * Must be done after setting MNTK_UNMOUNTF and before + * waiting for mnt_lockref to become 0. + */ + VFS_KILLIO(mp); + MNT_ILOCK(mp); + } error = 0; if (mp->mnt_lockref) { mp->mnt_kern_flag |= MNTK_DRAINING; --- fs/nfsclient/nfs_clvfsops.c.sav 2013-08-29 15:30:50.000000000 -0400 +++ fs/nfsclient/nfs_clvfsops.c 2013-08-29 16:52:54.000000000 -0400 @@ -120,6 +120,7 @@ static vfs_root_t nfs_root; static vfs_statfs_t nfs_statfs; static vfs_sync_t nfs_sync; static vfs_sysctl_t nfs_sysctl; +static vfs_killio_t nfs_killio; /* * nfs vfs operations. @@ -134,6 +135,7 @@ static struct vfsops nfs_vfsops = { .vfs_uninit = ncl_uninit, .vfs_unmount = nfs_unmount, .vfs_sysctl = nfs_sysctl, + .vfs_killio = nfs_killio, }; VFS_SET(nfs_vfsops, nfs, VFCF_NETWORK | VFCF_SBDRY); @@ -1676,6 +1678,19 @@ nfs_sysctl(struct mount *mp, fsctlop_t o } /* + * Kill off any RPCs in progress, so that they will all return errors. + * This allows dounmount() to continue as far as VFS_UNMOUNT() for a + * forced dismount. + */ +static void +nfs_killio(struct mount *mp) +{ + struct nfsmount *nmp = VFSTONFS(mp); + + newnfs_nmcancelreqs(nmp); +} + +/* * Extract the information needed by the nlm from the nfs vnode. */ static void
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2075428996.15437999.1377814901677.JavaMail.root>