From owner-freebsd-current@FreeBSD.ORG Thu Aug 4 02:41:42 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 00C16106564A for ; Thu, 4 Aug 2011 02:41:42 +0000 (UTC) (envelope-from okuno.kohji@jp.panasonic.com) Received: from smtp.mei.co.jp (smtp.mei.co.jp [133.183.100.20]) by mx1.freebsd.org (Postfix) with ESMTP id B7E3A8FC17 for ; Thu, 4 Aug 2011 02:41:41 +0000 (UTC) Received: from mail-gw.jp.panasonic.com ([157.8.1.157]) by smtp.mei.co.jp (8.12.11.20060614/3.7W/kc-maile11) with ESMTP id p742feim019664; Thu, 4 Aug 2011 11:41:40 +0900 (JST) Received: from epochmail.jp.panasonic.com ([157.8.1.130]) by mail.jp.panasonic.com (8.11.6p2/3.7W/kc-maili14) with ESMTP id p742fed22990; Thu, 4 Aug 2011 11:41:40 +0900 Received: by epochmail.jp.panasonic.com (8.12.11.20060308/3.7W/lomi17) id p742fehD028733; Thu, 4 Aug 2011 11:41:40 +0900 Received: from localhost by lomi17.jp.panasonic.com (8.12.11.20060308/3.7W) with ESMTP id p742fdfa028702; Thu, 4 Aug 2011 11:41:39 +0900 Date: Thu, 04 Aug 2011 11:41:39 +0900 (JST) Message-Id: <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> To: kostikbel@gmail.com From: Kohji Okuno In-Reply-To: <20110803135044.GM17489@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> Organization: Panasonic Corporation X-Mailer: Mew version 6.3 on Emacs 23.3 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com 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 02:41:42 -0000 Hello Kostik, From: Kostik Belousov Subject: Re: Bug: devfs is sure to have the bug. Date: Wed, 3 Aug 2011 16:50:44 +0300 Message-ID: <20110803135044.GM17489@deviant.kiev.zoral.com.ua> > On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote: >> Hello, >> >> > Hello, >> > >> > 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. >> > >> > >> > "dm->dm_generation" is updated with "devfs_generation" in >> > devfs_populate(), and the context holds only "dm->dm_lock" in >> > devfs_populate(). >> > >> > 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(). >> > >> > 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 ENOENT). >> > >> > I think that we should change the lock method. >> > May I have advice? >> > >> > void >> > devfs_populate(struct devfs_mount *dm) >> > { >> > >> > sx_assert(&dm->dm_lock, SX_XLOCKED); >> > if (dm->dm_generation == devfs_generation) >> > return; >> > while (devfs_populate_loop(dm, 0)) >> > continue; >> > (***) >> > dm->dm_generation = devfs_generation; >> > } >> > >> > void >> > devfs_create(struct cdev *dev) >> > { >> > struct cdev_priv *cdp; >> > >> > mtx_assert(&devmtx, MA_OWNED); >> > cdp = cdev2priv(dev); >> > cdp->cdp_flags |= CDP_ACTIVE; >> > cdp->cdp_inode = alloc_unrl(devfs_inos); >> > dev_refl(dev); >> > TAILQ_INSERT_TAIL(&cdevp_list, cdp, cdp_list); >> > devfs_generation++; >> > } >> > >> > void >> > devfs_destroy(struct cdev *dev) >> > { >> > struct cdev_priv *cdp; >> > >> > mtx_assert(&devmtx, MA_OWNED); >> > cdp = cdev2priv(dev); >> > cdp->cdp_flags &= ~CDP_ACTIVE; >> > devfs_generation++; >> > } >> > >> > Thanks. >> >> I propose the solution. May I have advice? >> >> void >> devfs_populate(struct devfs_mount *dm) >> { >> >> sx_assert(&dm->dm_lock, SX_XLOCKED); >> #if 1 /* I propose... */ >> int tmp_generation; >> retry: >> tmp_generation = devfs_generation; >> if (dm->dm_generation == tmp_generation) >> return; >> while (devfs_populate_loop(dm, 0)) >> continue; >> if (tmp_generation != devfs_generation) >> goto retry; >> dm->dm_generation = tmp_generation; >> #else /* Original */ >> if (dm->dm_generation == devfs_generation) >> return; >> while (devfs_populate_loop(dm, 0)) >> continue; >> dm->dm_generation = 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"); > > static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, "DEVFS filesystem"); > > -static unsigned devfs_generation; > +unsigned devfs_generation; > SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD, > &devfs_generation, 0, "DEVFS generation number"); > > @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup) > void > devfs_populate(struct devfs_mount *dm) > { > + unsigned gen; > > sx_assert(&dm->dm_lock, SX_XLOCKED); > - if (dm->dm_generation == devfs_generation) > + gen = devfs_generation; > + if (dm->dm_generation == gen) > return; > while (devfs_populate_loop(dm, 0)) > continue; > - dm->dm_generation = devfs_generation; > + dm->dm_generation = gen; > } > > /* > 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 { > > #define cdev2priv(c) member2struct(cdev_priv, cdp_c, c) > > +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 = VFSTODEVFS(vp->v_mount); > locked = VOP_ISLOCKED(vp); > > - sx_xlock(&dmp->dm_lock); > + if (!dm_locked) > + sx_xlock(&dmp->dm_lock); > DEVFS_DMP_HOLD(dmp); > > /* Can't call devfs_populate() with the vnode lock held. */ > @@ -242,7 +243,7 @@ devfs_vptocnp(struct vop_vptocnp_args *ap) > > dmp = VFSTODEVFS(vp->v_mount); > > - error = devfs_populate_vp(vp); > + error = devfs_populate_vp(vp, 0); > if (error != 0) > return (error); > > @@ -643,7 +644,7 @@ devfs_getattr(struct vop_getattr_args *ap) > struct devfs_mount *dmp; > struct cdev *dev; > > - error = devfs_populate_vp(vp); > + error = devfs_populate_vp(vp, 0); > if (error != 0) > return (error); > > @@ -903,7 +904,7 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock) > > if (cdev == NULL) > sx_xlock(&dmp->dm_lock); > - else if (devfs_populate_vp(dvp) != 0) { > + else if (devfs_populate_vp(dvp, 0) != 0) { > *dm_unlock = 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; > > - if (devfs_populate_vp(ap->a_dvp) != 0) > + dm_unlock = 0; > +retry: > + if (devfs_populate_vp(ap->a_dvp, dm_unlock) != 0) > return (ENOTDIR); > > dmp = VFSTODEVFS(ap->a_dvp->v_mount); > dm_unlock = 1; > - j = devfs_lookupx(ap, &dm_unlock); > - if (dm_unlock == 1) > + error = devfs_lookupx(ap, &dm_unlock); > + if (error == ENOENT) { > + if (dm_unlock) > + sx_assert(&dmp->dm_lock, SA_XLOCKED); > + else { > + sx_xlock(&dmp->dm_lock); > + dm_unlock = 1; > + } > + if (devfs_generation != dmp->dm_generation) > + goto retry; > + } > + if (dm_unlock) > sx_xunlock(&dmp->dm_lock); > - return (j); > + return (error); > } > > static int > @@ -1202,7 +1214,7 @@ devfs_readdir(struct vop_readdir_args *ap) > } > > dmp = VFSTODEVFS(ap->a_vp->v_mount); > - if (devfs_populate_vp(ap->a_vp) != 0) { > + if (devfs_populate_vp(ap->a_vp, 0) != 0) { > if (tmp_ncookies != NULL) > ap->a_ncookies = tmp_ncookies; > return (EIO); > @@ -1565,7 +1577,7 @@ devfs_symlink(struct vop_symlink_args *ap) > if (error) > return(error); > dmp = VFSTODEVFS(ap->a_dvp->v_mount); > - if (devfs_populate_vp(ap->a_dvp) != 0) > + if (devfs_populate_vp(ap->a_dvp, 0) != 0) > return (ENOENT); > > dd = ap->a_dvp->v_data; Thank you for your comment. But, now I'm using 8.1-RELEASE. May I have advice about 8.X ? Best regards, Kohji Okuno