From owner-freebsd-current@FreeBSD.ORG Thu Aug 4 09:56:03 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 545D5106564A for ; Thu, 4 Aug 2011 09:56:03 +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 122948FC0C for ; Thu, 4 Aug 2011 09:56:02 +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-maile13) with ESMTP id p749u1Qq022793; Thu, 4 Aug 2011 18:56:01 +0900 (JST) Received: from epochmail.jp.panasonic.com ([157.8.1.130]) by mail.jp.panasonic.com (8.11.6p2/3.7W/kc-maili13) with ESMTP id p749u1G10526; Thu, 4 Aug 2011 18:56:01 +0900 Received: by epochmail.jp.panasonic.com (8.12.11.20060308/3.7W/lomi12) id p749u11O031823; Thu, 4 Aug 2011 18:56:01 +0900 Received: from localhost by lomi12.jp.panasonic.com (8.12.11.20060308/3.7W) with ESMTP id p749u1Ow031806; Thu, 4 Aug 2011 18:56:01 +0900 Date: Thu, 04 Aug 2011 18:56:00 +0900 (JST) Message-Id: <20110804.185600.366306193153346256.okuno.kohji@jp.panasonic.com> To: kostikbel@gmail.com From: Kohji Okuno In-Reply-To: <20110804095043.GA17489@deviant.kiev.zoral.com.ua> References: <20110803135044.GM17489@deviant.kiev.zoral.com.ua> <20110804.114139.1356575228565324749.okuno.kohji@jp.panasonic.com> <20110804095043.GA17489@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 09:56:03 -0000 Hello Kostik, > On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote: >> Hello Kostik, >> >> 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 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 ? > 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. I'm sorry. I need a patch for 8.1-RELEASE. Could you propose patch? Thanks, Kohji Okuno