From owner-freebsd-current@FreeBSD.ORG Thu Aug 4 09:50:49 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 520B01065670 for ; Thu, 4 Aug 2011 09:50:49 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id B5FAA8FC08 for ; Thu, 4 Aug 2011 09:50:48 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p749oiVp003961 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 4 Aug 2011 12:50:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p749oicC028246; Thu, 4 Aug 2011 12:50:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p749oiAM028245; Thu, 4 Aug 2011 12:50:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 4 Aug 2011 12:50:43 +0300 From: Kostik Belousov To: Kohji Okuno Message-ID: <20110804095043.GA17489@deviant.kiev.zoral.com.ua> References: <20110803.141636.145758949453810779.okuno.kohji@jp.panasonic.com> <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com> <20110803135044.GM17489@deviant.kiev.zoral.com.ua> <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="KQOSl2CQwBKYGM8f" Content-Disposition: inline In-Reply-To: <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.3 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-current@freebsd.org Subject: Re: Bug: devfs is sure to have the bug. X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Aug 2011 09:50:49 -0000 --KQOSl2CQwBKYGM8f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote: > Hello Kostik, >=20 > From: Kostik Belousov > Subject: Re: Bug: devfs is sure to have the bug. > Date: Wed, 3 Aug 2011 16:50:44 +0300 > > I think the problem you described is real, and suggested change is righ= t. > > Initially, I thought that we should work with devfs_generation as with > > the atomic type due to unlocked access in the devfs_populate(), but then > > convinced myself that this is not needed. > >=20 > > But also, I think there is another half of the problem. Namely, > > devfs_lookup() calls devfs_populate_vp(), and then does lookup with the > > help of devfs_lookupx(). We will miss the generation update > > happen after the drop of the dm_lock in devfs_populate_vp() to reacquire > > the directory vnode lock. > >=20 > > I propose the change below, consisting of your fix and also retry of > > population and lookup in case generations do not match for ENOENT > >case. > >=20 > > diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c > > index d72ada0..8ff9bc2 100644 > > --- a/sys/fs/devfs/devfs_devs.c > > +++ b/sys/fs/devfs/devfs_devs.c > > @@ -63,7 +63,7 @@ static MALLOC_DEFINE(M_CDEVP, "DEVFS1", "DEVFS cdev_p= riv storage"); > > =20 > > static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesy= stem"); > > =20 > > -static unsigned devfs_generation; > > +unsigned devfs_generation; > > SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD, > > &devfs_generation, 0, "DEVFS generation number"); > > =20 > > @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int c= leanup) > > void > > devfs_populate(struct devfs_mount *dm) > > { > > + unsigned gen; > > =20 > > sx_assert(&dm->dm_lock, SX_XLOCKED); > > - if (dm->dm_generation =3D=3D devfs_generation) > > + gen =3D devfs_generation; > > + if (dm->dm_generation =3D=3D gen) > > return; > > while (devfs_populate_loop(dm, 0)) > > continue; > > - dm->dm_generation =3D devfs_generation; > > + dm->dm_generation =3D gen; > > } > > =20 > > /* > > diff --git a/sys/fs/devfs/devfs_int.h b/sys/fs/devfs/devfs_int.h > > index cdc6aba..cb01ad1 100644 > > --- a/sys/fs/devfs/devfs_int.h > > +++ b/sys/fs/devfs/devfs_int.h > > @@ -71,6 +71,8 @@ struct cdev_priv { > > =20 > > #define cdev2priv(c) member2struct(cdev_priv, cdp_c, c) > > =20 > > +extern unsigned devfs_generation; > > + > > struct cdev *devfs_alloc(int); > > int devfs_dev_exists(const char *); > > void devfs_free(struct cdev *); > > diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c > > index 955bd8b..2603caa 100644 > > --- a/sys/fs/devfs/devfs_vnops.c > > +++ b/sys/fs/devfs/devfs_vnops.c > > @@ -188,7 +188,7 @@ devfs_clear_cdevpriv(void) > > * On success devfs_populate_vp() returns with dmp->dm_lock held. > > */ > > static int > > -devfs_populate_vp(struct vnode *vp) > > +devfs_populate_vp(struct vnode *vp, int dm_locked) > > { > > struct devfs_dirent *de; > > struct devfs_mount *dmp; > > @@ -199,7 +199,8 @@ devfs_populate_vp(struct vnode *vp) > > dmp =3D VFSTODEVFS(vp->v_mount); > > locked =3D VOP_ISLOCKED(vp); > > =20 > > - sx_xlock(&dmp->dm_lock); > > + if (!dm_locked) > > + sx_xlock(&dmp->dm_lock); > > DEVFS_DMP_HOLD(dmp); > > =20 > > /* Can't call devfs_populate() with the vnode lock held. */ > > @@ -242,7 +243,7 @@ devfs_vptocnp(struct vop_vptocnp_args *ap) > > =20 > > dmp =3D VFSTODEVFS(vp->v_mount); > > =20 > > - error =3D devfs_populate_vp(vp); > > + error =3D devfs_populate_vp(vp, 0); > > if (error !=3D 0) > > return (error); > > =20 > > @@ -643,7 +644,7 @@ devfs_getattr(struct vop_getattr_args *ap) > > struct devfs_mount *dmp; > > struct cdev *dev; > > =20 > > - error =3D devfs_populate_vp(vp); > > + error =3D devfs_populate_vp(vp, 0); > > if (error !=3D 0) > > return (error); > > =20 > > @@ -903,7 +904,7 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_u= nlock) > > =20 > > if (cdev =3D=3D NULL) > > sx_xlock(&dmp->dm_lock); > > - else if (devfs_populate_vp(dvp) !=3D 0) { > > + else if (devfs_populate_vp(dvp, 0) !=3D 0) { > > *dm_unlock =3D 0; > > sx_xlock(&dmp->dm_lock); > > if (DEVFS_DMP_DROP(dmp)) { > > @@ -966,19 +967,30 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm= _unlock) > > static int > > devfs_lookup(struct vop_lookup_args *ap) > > { > > - int j; > > struct devfs_mount *dmp; > > - int dm_unlock; > > + int error, dm_unlock; > > =20 > > - if (devfs_populate_vp(ap->a_dvp) !=3D 0) > > + dm_unlock =3D 0; > > +retry: > > + if (devfs_populate_vp(ap->a_dvp, dm_unlock) !=3D 0) > > return (ENOTDIR); > > =20 > > dmp =3D VFSTODEVFS(ap->a_dvp->v_mount); > > dm_unlock =3D 1; > > - j =3D devfs_lookupx(ap, &dm_unlock); > > - if (dm_unlock =3D=3D 1) > > + error =3D devfs_lookupx(ap, &dm_unlock); > > + if (error =3D=3D ENOENT) { > > + if (dm_unlock) > > + sx_assert(&dmp->dm_lock, SA_XLOCKED); > > + else { > > + sx_xlock(&dmp->dm_lock); > > + dm_unlock =3D 1; > > + } > > + if (devfs_generation !=3D dmp->dm_generation) > > + goto retry; > > + } > > + if (dm_unlock) > > sx_xunlock(&dmp->dm_lock); > > - return (j); > > + return (error); > > } > > =20 > > static int > > @@ -1202,7 +1214,7 @@ devfs_readdir(struct vop_readdir_args *ap) > > } > > =20 > > dmp =3D VFSTODEVFS(ap->a_vp->v_mount); > > - if (devfs_populate_vp(ap->a_vp) !=3D 0) { > > + if (devfs_populate_vp(ap->a_vp, 0) !=3D 0) { > > if (tmp_ncookies !=3D NULL) > > ap->a_ncookies =3D tmp_ncookies; > > return (EIO); > > @@ -1565,7 +1577,7 @@ devfs_symlink(struct vop_symlink_args *ap) > > if (error) > > return(error); > > dmp =3D VFSTODEVFS(ap->a_dvp->v_mount); > > - if (devfs_populate_vp(ap->a_dvp) !=3D 0) > > + if (devfs_populate_vp(ap->a_dvp, 0) !=3D 0) > > return (ENOENT); > > =20 > > dd =3D ap->a_dvp->v_data; >=20 > Thank you for your comment. > But, now I'm using 8.1-RELEASE. May I have advice about 8.X ? Do you mean a patch for the stable/8 ? I believe it is enough to apply rev. 211628 to stable/8, then the patch I posted yesterday should be compilable. --KQOSl2CQwBKYGM8f Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk46a3MACgkQC3+MBN1Mb4hCDACg6RkKaszL0UO2d2dqfqZT/Ouq I7sAoPBZ0w5jX7/wPR+H4qUXLTVfpK9z =c1bA -----END PGP SIGNATURE----- --KQOSl2CQwBKYGM8f--