From owner-freebsd-current@FreeBSD.ORG Wed Aug 3 13: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 BCF57106566B for ; Wed, 3 Aug 2011 13: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 134A98FC08 for ; Wed, 3 Aug 2011 13: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 p73DojW0002304 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 3 Aug 2011 16:50:45 +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 p73Dojcg066045; Wed, 3 Aug 2011 16:50:45 +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 p73DoiY1065994; Wed, 3 Aug 2011 16: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: Wed, 3 Aug 2011 16:50:44 +0300 From: Kostik Belousov To: Kohji Okuno Message-ID: <20110803135044.GM17489@deviant.kiev.zoral.com.ua> References: <20110803.141636.145758949453810779.okuno.kohji@jp.panasonic.com> <20110803.144423.2018247186416313018.okuno.kohji@jp.panasonic.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Z7URUY1CzunwZ4PW" Content-Disposition: inline In-Reply-To: <20110803.144423.2018247186416313018.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: Wed, 03 Aug 2011 13:50:49 -0000 --Z7URUY1CzunwZ4PW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote: > Hello, >=20 > > Hello, > >=20 > > I think that devfs is sure to have the bug. > > I found that I couldn't open "/dev/XXX" though the kernel detected XXX > > device. > >=20 > >=20 > > "dm->dm_generation" is updated with "devfs_generation" in > > devfs_populate(), and the context holds only "dm->dm_lock" in > > devfs_populate(). > >=20 > > On the other hand, "devfs_generation" is incremented in devfs_create() > > and devfs_destroy() the context holds only "devmtx" in devfs_create() > > and devfs_destroy(). > >=20 > > If a context executes devfs_create() when other context is executing > > (***), then "dm->dm_generation" is updated incorrect value. > > As a result, we can not open the last detected device (we receive ENOEN= T). > >=20 > > I think that we should change the lock method. > > May I have advice? > >=20 > > void > > devfs_populate(struct devfs_mount *dm) > > { > >=20 > > sx_assert(&dm->dm_lock, SX_XLOCKED); > > if (dm->dm_generation =3D=3D devfs_generation) > > return; > > while (devfs_populate_loop(dm, 0)) > > continue; > > (***) > > dm->dm_generation =3D devfs_generation; > > } > >=20 > > void > > devfs_create(struct cdev *dev) > > { > > struct cdev_priv *cdp; > >=20 > > mtx_assert(&devmtx, MA_OWNED); > > cdp =3D cdev2priv(dev); > > cdp->cdp_flags |=3D CDP_ACTIVE; > > cdp->cdp_inode =3D alloc_unrl(devfs_inos); > > dev_refl(dev); > > TAILQ_INSERT_TAIL(&cdevp_list, cdp, cdp_list); > > devfs_generation++; > > } > >=20 > > void > > devfs_destroy(struct cdev *dev) > > { > > struct cdev_priv *cdp; > >=20 > > mtx_assert(&devmtx, MA_OWNED); > > cdp =3D cdev2priv(dev); > > cdp->cdp_flags &=3D ~CDP_ACTIVE; > > devfs_generation++; > > } > >=20 > > Thanks. >=20 > I propose the solution. May I have advice? >=20 > void > devfs_populate(struct devfs_mount *dm) > { >=20 > sx_assert(&dm->dm_lock, SX_XLOCKED); > #if 1 /* I propose... */ > int tmp_generation; > retry: > tmp_generation =3D devfs_generation; > if (dm->dm_generation =3D=3D tmp_generation) > return; > while (devfs_populate_loop(dm, 0)) > continue; > if (tmp_generation !=3D devfs_generation) > goto retry; > dm->dm_generation =3D tmp_generation; > #else /* Original */ > if (dm->dm_generation =3D=3D devfs_generation) > return; > while (devfs_populate_loop(dm, 0)) > continue; > dm->dm_generation =3D devfs_generation; > #endif > } I think the problem you described is real, and suggested change is right. 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. 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. 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. 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_priv = storage"); =20 static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesystem= "); =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 clean= up) 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_unloc= k) =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_unl= ock) 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; --Z7URUY1CzunwZ4PW Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk45UjQACgkQC3+MBN1Mb4hpsgCfY6WlwW7KY4ZDqYlaD4qT2LAi 5l0AoOvRtF51UCA0dkVNYz6sa28GvbEK =XL0G -----END PGP SIGNATURE----- --Z7URUY1CzunwZ4PW--