From owner-cvs-all@FreeBSD.ORG Wed Aug 15 17:19:10 2012 Return-Path: Delivered-To: cvs-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DB5A31065673; Wed, 15 Aug 2012 17:19:10 +0000 (UTC) (envelope-from pawel@dawidek.net) Received: from mail.dawidek.net (garage.dawidek.net [91.121.88.72]) by mx1.freebsd.org (Postfix) with ESMTP id 967348FC18; Wed, 15 Aug 2012 17:19:10 +0000 (UTC) Received: from localhost (89-73-195-149.dynamic.chello.pl [89.73.195.149]) by mail.dawidek.net (Postfix) with ESMTPSA id 89D541B0; Wed, 15 Aug 2012 19:18:43 +0200 (CEST) Date: Wed, 15 Aug 2012 19:19:02 +0200 From: Pawel Jakub Dawidek To: Poul-Henning Kamp Message-ID: <20120815171902.GD1856@garage.freebsd.pl> References: <437D6AB5.7020306@root.org> <6759.1132305580@critter.freebsd.dk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OaZoDhBhXzo6bW1J" Content-Disposition: inline In-Reply-To: <6759.1132305580@critter.freebsd.dk> X-OS: FreeBSD 10.0-CURRENT amd64 User-Agent: Mutt/1.5.21 (2010-09-15) Cc: John Polstra , cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org, Nate Lawson Subject: Re: cvs commit: src/sys/cam/scsi scsi_cd.c scsi_da.c src/sys/geom geom_disk.c geom_disk.h geom_subr.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: **OBSOLETE** CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Aug 2012 17:19:11 -0000 --OaZoDhBhXzo6bW1J Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 18, 2005 at 10:19:40AM +0100, Poul-Henning Kamp wrote: > In message <437D6AB5.7020306@root.org>, Nate Lawson writes: >=20 > >> +void > >> +disk_gone(struct disk *dp) > >> +{ > >> + struct g_geom *gp; > >> + struct g_provider *pp; > >> + > >> + gp =3D dp->d_geom; > >> + if (gp !=3D NULL) > >> + LIST_FOREACH(pp, &gp->provider, provider) > >> + g_orphan_provider(pp, ENXIO); > >> +} > >> + > > > >Does there need to be locking for this list traversal? Couldn't=20 > >disk_gone() race in parallel with a taste event if someone plugs/unplugs= =20 > >quickly, especially for a slow device (i.e. floppy)? >=20 > Disk gone is called by the driver which owns struct disk, so nobody > else has any business messing with that particular list. >=20 > Obviously the driver needs to not stomp on itself, but Giant does > that for CAM. Sorry for answering to ~7 years old e-mail, but the lock is actually necessary. Without holding the topology lock in disk_gone() we risk that g_orphan_provider() or g_wither_provider() is more recent version will remove provider from the list and we will use a pointer to freed provider to find next one on the list. Someone recently reported such panic to me and asked if LIST_FOREACH_SAFE() would be enough to fix the problem. Taking into account your response it seems it will be enough, but I still think we should use the topology lock here, especially now that CAM is not Giant-locked. --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl --OaZoDhBhXzo6bW1J Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (FreeBSD) iEYEARECAAYFAlAr2gYACgkQForvXbEpPzTbiACgvf3TEqKh7EmzZ3nhiJ8nh/f5 NzsAn2/8vLQ8AEIC4O1WMcLjbi7Vv1U2 =DhqB -----END PGP SIGNATURE----- --OaZoDhBhXzo6bW1J--