Date: Mon, 28 Apr 2008 16:24:13 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Daichi GOTO <daichi@freebsd.org> Cc: fs@freebsd.org Subject: Re: Approval request of some additions to src/sys/kern/vfs_subr.c and src/sys/sys/vnode.h Message-ID: <20080428132413.GS18958@deviant.kiev.zoral.com.ua> In-Reply-To: <48159AC5.3030000@freebsd.org> References: <4811B0A0.8040702@freebsd.org> <20080426100116.GL18958@deviant.kiev.zoral.com.ua> <48159AC5.3030000@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--A61Eau4L8twGtri1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 28, 2008 at 06:37:09PM +0900, Daichi GOTO wrote: > Kostik Belousov wrote: > >On Fri, Apr 25, 2008 at 07:21:20PM +0900, Daichi GOTO wrote: > >>Hi Konstantin :) > >> > >>To fix a unionfs issue of=20 > >>http://www.freebsd.org/cgi/query-pr.cgi?pr=3D109377, > >>we need to add new functions > >> > >> void vkernrele(struct vnode *vp); > >> void vkernref(struct vnode *vp); > >> > >>and one value > >> > >> int v_kernusecount; /* i ref count of kernel */ > >> > >>to src/sys/sys/vnode.h and rc/sys/kern/vfs_subr.c. > >> > >>Unionfs will be panic when lower fs layer is forced umounted by > >>"umount -f". So to avoid this issue, we've added > >>"v_kernusecount" value that means "a vnode count that kernel are > >>using". vkernrele() and vkernref() are functions that manage > >>"v_kernusecount" value. > >> > >>Please check those and give us an approve or some comments! > > > >There is already the vnode reference count. From your description, I can= not > >understand how the kernusecount would prevent the panic when forced unmo= unt > >is performed. Could you, please, show the actual code ? PR mentioned > >does not contain any patch. >=20 > Our patch realizes avoiding kernel panic by "umount -f" operation using w= ith > EBUSY process. >=20 > On current implementation (not applied our patch), "umount -f" tries to > release vnode at any vnode reference count value. Since that, unionfs > and nullfs access invalid vnode and lead kernel panic. To prevent this > issue, we need a some kind of not-umount-accept-mechanism in invalid case > (e.x. fs in unionfsed stack, it must be umounted in correct order) and > to realize that, current vnode reference count is not enough we are=20 > thinking. >=20 > If you have any ideas to realize the same solution with current vnode > reference, would you please tell us your idea :) >=20 > >The problem you described is common for the kernel code, and right way > >to handle it, for now, is to keep refcount _and_ check for the forced > >reclaim. Your patch in essence disables the forced unmount. I would object against such decision. Even if taking this direction, I believe more cleaner solution would be to introduce a counter that disables the (forced) unmount into the struct mount, instead of the struct vnode. Having the counter in the vnode, the unmount -f behaviour is non-deterministic and depended on the presence of the cached vnodes of the upper layer. The mount counter would be incremented by unionfs cover mount. But, as I said above, this looks like a wrong solution. The right way to handle the forced reclaim with the current VFS is to add the explicit checks for the reclaimed vnodes where it is needed. The vnode cannot be reclaimed while the vnode lock is held. When obtaining the vnode lock, the reclamation can be detected. For instance, the vget() without LK_RETRY shall be checked for ENOENT. You said that that nullfs is vulnerable to the problem. Could you, please, point me to the corresponding stack trace ? At least, the nullfs vop_lock() seems to carefully check the possible problems. --A61Eau4L8twGtri1 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkgVz/wACgkQC3+MBN1Mb4isvwCfbECmYEu6lJ2FXIqaU3zYPTZs 5I0AoNzrqhXvT5XHDQs+l65owxM8rfp3 =eTfF -----END PGP SIGNATURE----- --A61Eau4L8twGtri1--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080428132413.GS18958>