From owner-freebsd-fs@FreeBSD.ORG Fri Aug 30 08:53:42 2013 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id AEB0B592 for ; Fri, 30 Aug 2013 08:53:42 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 4CE562322 for ; Fri, 30 Aug 2013 08:53:42 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.7/8.14.7) with ESMTP id r7U8ralr052323; Fri, 30 Aug 2013 11:53:36 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua r7U8ralr052323 Received: (from kostik@localhost) by tom.home (8.14.7/8.14.7/Submit) id r7U8rapG052322; Fri, 30 Aug 2013 11:53:36 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 30 Aug 2013 11:53:36 +0300 From: Konstantin Belousov To: Rick Macklem Subject: Re: fixing "umount -f" for the NFS client Message-ID: <20130830085336.GU4972@kib.kiev.ua> References: <20130829223128.GP4972@kib.kiev.ua> <537646864.15457428.1377819814825.JavaMail.root@uoguelph.ca> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="J4V09s3zFtpAP6QA" Content-Disposition: inline In-Reply-To: <537646864.15457428.1377819814825.JavaMail.root@uoguelph.ca> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on tom.home Cc: freebsd-fs X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Aug 2013 08:53:42 -0000 --J4V09s3zFtpAP6QA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 29, 2013 at 07:43:34PM -0400, Rick Macklem wrote: > 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(). > > > > >=20 > > > > > 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. > > > > >=20 > > > > > The problem seems to be that dounmount() msleep()s while > > > > > mnt_lockref !=3D 0 before calling VFS_UNMOUNT(). > > > > >=20 > > > > > 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. > > > > >=20 > > > > > 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. > > > > >=20 > > > > > Anyone have comments on this? > > > > >=20 > > > > Yes, I do. I agree with adding the pre-unmount vfs method. > > > > This seems to be the cleanest solution possible. > > > >=20 > > > 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() ? > >=20 > Sure, any name is fine with me. >=20 > > 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 > > (). > >=20 > 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 =3D=3D 0. I don't > see why setting it before VFS_PURGE() would matter? >=20 > 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 clie= nt > 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 serve= r. > 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(), dou= nmount() > should be in the msleep() with MNTK_DRAINING set, so it will get the wake= up > once mnt_lockref has decremented to 0. >=20 > Setting MNTK_DRAINING sooner would just result in the odd unnecessary wak= eup(), > from what I can see? >=20 Hm, I mis-remembered the vfs_busy() code. Yes, I agree with you. > > >=20 > > > I assume I would also need to bump __FreeBSD_version (and maybe > > > VFS_VERSION?). > > I think you could avoid it. > >=20 > Do you mean I don't need to bump __FreeBSD_version or VFS_VERSION or both? I do not see much sense in bumping either of them. You might want to bump __FreeBSD_version when merging to stable. --J4V09s3zFtpAP6QA Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.21 (FreeBSD) iQIcBAEBAgAGBQJSIF2PAAoJEJDCuSvBvK1BBFoQAI6iR1c8lZ6AQxdCfmukzlUm g1R4ZqmyOO2RSYgWpYr0vDCyBjDaZ0yHZ9B4LEHNXQ9mg7P5Y/fouYuCluuo/jWt +7He7xf3NZy7ziYKzj7HdV76Rktwd00y5QZ2b1tJSw0C3fmHySs78DH7N0I5eFCj vlKnUxHm+sw5ZnXq3bXj6DHuMFgESlxME3kbauHKglF40LNTBRjYArahgkdIKHgF +bIX5MCva1F5AccbYhF9iTld8W3Zvftcif2XuuUlEKG/R10wdaJfwAhDZGNO8R5c BXa0t6rN3xaxJpCs66anSDdFHOc2rfSuEH0/kH4i1MLfqYyHDIweaSnhMIOp82U2 ausgzDseHA+yAkNNkzRG3XIyEvUfo7O1veWNC+I0tzuQDHTjDdbRwS+A20pUPonS ehSayrmbl4VTV6levQuaALMM5OydrvT0PAsB4mP+qjFnbrtutxTCjUZ7IgaRI7Qs 4G+OMelfhFaV/nOABxYLpXe7ft2m+BODJgu2CNA4ECF3tU1qSICNEW0yqGWUpVAM T2nA3/ISCCcUJf4p/jA6F/iSNGCh61eMYgm5YHVsyeqkUg7Q/I0t0v1iraOsz3jR 1cD+Lojm/L4WqEU9txwYPQMmcskAiiAviy5AY1IjfrzQIX6/EZJshN3+797vMIOB B/VoWnK+ebInmSQ85rKO =zco0 -----END PGP SIGNATURE----- --J4V09s3zFtpAP6QA--