Date: Sun, 7 Oct 2012 11:20:37 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Andriy Gapon <avg@FreeBSD.org> Cc: "freebsd-fs@freebsd.org" <freebsd-fs@FreeBSD.org> Subject: Re: zfs_remove: delete_now case Message-ID: <20121007092037.GB28611@garage.freebsd.pl> In-Reply-To: <50713582.9040600@FreeBSD.org> References: <50713582.9040600@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--BwCQnh7xodEAoBMC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Oct 07, 2012 at 10:55:46AM +0300, Andriy Gapon wrote: >=20 > It seems that the delete_now path is never taken in zfs_remove(). > This is probably good in the bug-cancels-bug way... >=20 > On FreeBSD VOP_REMOVE is always called with vp being referenced. zfs_rem= ove > doesn't take advantage of VOP_REMOVE interface. It ignores the vp argume= nt and > instead re-looks up a directory entry id by name (a small performance hit= here) > and then uses zfs_zget, which adds another reference to the entry's vnode. > Thus, a reference count of the vnode is always not less than two. So > may_delete_now and delete_now are always false. >=20 > Why this is good? Because FreeBSD VFS doesn't support direct destruction= (or > corruption) of the vnode in VOP_REMOVE. It expects to still have a valid= vnode > with a valid reference count after VOP_REMOVE and then calls vput/vrele o= n it. > But the code in the delete_now branch does some nasty things. It directly > decrements the use count and it directly destroys the underlying znode (w= hich is > fine in Solaris but not in FreeBSD). > But FreeBSD VFS wouldn't even have a chance to panic on the damaged vnode > because ZFS code would sooner panic in zfs_znode_delete -> zfs_znode_free= -> > ASSERT(ZTOV(zp) =3D=3D NULL) [a FreeBSD-specific assertion). >=20 > I think that we should make zfs_remove code less confusing and more FreeB= SD > friendly. We should explicitly rely on zfs_inactive doing the right thin= g after > VOP_REMOVE and drop all the "direct action" code. >=20 > What do you think? I'm fully aware of this code path being dead on FreeBSD. It is left there only to minimize diff against vendor, so I'd prefer not to remove it. Surrounding the code with '#ifdef sun' or similar is ok, I think. --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl --BwCQnh7xodEAoBMC Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iEYEARECAAYFAlBxSWUACgkQForvXbEpPzT7kgCdF6+4xV+L7aF17HtZFxFVBL+v gjUAniJwvbeAzHRvlHUEBdxjh+6roDiy =UJy4 -----END PGP SIGNATURE----- --BwCQnh7xodEAoBMC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121007092037.GB28611>