Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 1 Jul 2012 05:13:51 +0000 (UTC)
From:      "Kenneth D. Merry" <ken@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org
Subject:   svn commit: r237873 - in stable/9: share/man/man9 sys/cam/scsi sys/dev/xen/blkfront sys/geom
Message-ID:  <201207010513.q615Dp2F067167@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ken
Date: Sun Jul  1 05:13:50 2012
New Revision: 237873
URL: http://svn.freebsd.org/changeset/base/237873

Log:
  MFC 237518, 237545, 237648:
  
    r237518 | ken | 2012-06-23 22:29:03 -0600 (Sat, 23 Jun 2012) | 72 lines
  
    Fix a bug which causes a panic in daopen(). The panic is caused by
    a da(4) instance going away while GEOM is still probing it.
  
    In this case, the GEOM disk class instance has been created by
    disk_create(), and the taste of the disk is queued in the GEOM
    event queue.
  
    While that event is queued, the da(4) instance goes away.  When the
    open call comes into the da(4) driver, it dereferences the freed
    (but non-NULL) peripheral pointer provided by GEOM, which results
    in a panic.
  
    The solution is to add a callback to the GEOM disk code that is
    called when all of its resources are cleaned up.  This is
    implemented inside GEOM by adding an optional callback that is
    called when all consumers have detached from a provider, and the
    provider is about to be deleted.
  
    scsi_cd.c,
    scsi_da.c:	In the register routine for the cd(4) and da(4)
    		routines, acquire a reference to the CAM peripheral
    		instance just before we call disk_create().
  
    		Use the new GEOM disk d_gone() callback to register
    		a callback (dadiskgonecb()/cddiskgonecb()) that
    		decrements the peripheral reference count once GEOM
    		has finished cleaning up its resources.
  
    		In the cd(4) driver, clean up open and close
    		behavior slightly.  GEOM makes sure we only get one
    		open() and one close call, so there is no need to
    		set an open flag and decrement the reference count
    		if we are not the first open.
  
    		In the cd(4) driver, use cam_periph_release_locked()
    		in a couple of error scenarios to avoid extra mutex
    		calls.
  
    geom.h:	Add a new, optional, providergone callback that
    		is called when a provider is about to be deleted.
  
    geom_disk.h:	Add a new d_gone() callback to the GEOM disk
    		interface.
  
    		Bump the DISK_VERSION to version 2.  This probably
    		should have been done after a couple of previous
    		changes, especially the addition of the d_getattr()
    		callback.
  
    geom_disk.c:	Add a providergone callback for the disk class,
    		g_disk_providergone(), that calls the user's
    		d_gone() callback if it exists.
  
    		Bump the DISK_VERSION to 2.
  
    geom_subr.c:	In g_destroy_provider(), call the providergone
    		callback if it has been provided.
  
    		In g_new_geomf(), propagate the class's
    		providergone callback to the new geom instance.
  
    blkfront.c:	Callers of disk_create() are supposed to pass in
    		DISK_VERSION, not an explicit disk API version
    		number.  Update the blkfront driver to do that.
  
    disk.9:	Update the disk(9) man page to include information
    		on the new d_gone() callback, as well as the
    		previously added d_getattr() callback, d_descr
    		field, and HBA PCI ID fields.
  
    r237545 | ken | 2012-06-24 22:26:10 -0600 (Sun, 24 Jun 2012) | 7 lines
  
    Consume spare fields for the providergone pointers added to the g_class and
    g_geom structures in change 237518.  The original change would have broken
    the ABI.
  
    Suggested by:	ae
  
    r237648 | ken | 2012-06-27 10:05:09 -0600 (Wed, 27 Jun 2012) | 6 lines
  
    In g_disk_providergone(), don't continue if the softc is NULL.  This may be
    the case if we've already gone through g_disk_destroy().
  
    Reported by:	Michael Butler <imb@protected-networks.net>

Modified:
  stable/9/share/man/man9/disk.9
  stable/9/sys/cam/scsi/scsi_cd.c
  stable/9/sys/cam/scsi/scsi_da.c
  stable/9/sys/dev/xen/blkfront/blkfront.c
  stable/9/sys/geom/geom.h
  stable/9/sys/geom/geom_disk.c
  stable/9/sys/geom/geom_disk.h
  stable/9/sys/geom/geom_subr.c
Directory Properties:
  stable/9/share/man/man9/   (props changed)
  stable/9/sys/   (props changed)
  stable/9/sys/dev/   (props changed)

Modified: stable/9/share/man/man9/disk.9
==============================================================================
--- stable/9/share/man/man9/disk.9	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/share/man/man9/disk.9	Sun Jul  1 05:13:50 2012	(r237873)
@@ -145,6 +145,16 @@ Optional: if configured with
 .Xr dumpon 8 ,
 this function is invoked from a very restricted system state after a
 kernel panic to record a copy of the system RAM to the disk.
+.It Vt "disk_getattr_t *" Va d_getattr
+Optional: if this method is provided, it gives the disk driver the
+opportunity to override the default GEOM response to BIO_GETATTR requests.
+This function should return -1 if the attribute is not handled, 0 if the
+attribute is handled, or an errno to be passed to g_io_deliver().
+.It Vt "disk_gone_t *" Va d_gone
+Optional: if this method is provided, it will be called after disk_gone()
+is called, once GEOM has finished its cleanup process.
+Once this callback is called, it is safe for the disk driver to free all of
+its resources, as it will not be receiving further calls from GEOM.
 .El
 .Ss Mandatory Media Properties
 The following fields identify the size and granularity of the disk device.
@@ -180,7 +190,23 @@ Please see
 .Pa src/sys/geom/notes
 for details.
 .It Vt char Va d_ident[DISK_IDENT_SIZE]
-This field can and should be used to store disk's serial number.
+This field can and should be used to store disk's serial number if the
+d_getattr method described above isn't implemented, or if it does not
+support the GEOM::ident attribute.
+.It Vt char Va d_descr[DISK_IDENT_SIZE]
+This field can be used to store the disk vendor and product description.
+.It Vt uint16_t Va d_hba_vendor
+This field can be used to store the PCI vendor ID for the HBA connected to
+the disk.
+.It Vt uint16_t Va d_hba_device
+This field can be used to store the PCI device ID for the HBA connected to
+the disk.
+.It Vt uint16_t Va d_hba_subvendor
+This field can be used to store the PCI subvendor ID for the HBA connected to
+the disk.
+.It Vt uint16_t Va d_hba_subdevice
+This field can be used to store the PCI subdevice ID for the HBA connected to
+the disk.
 .El
 .Ss Driver Private Data
 This field may be used by the device driver to store a pointer to

Modified: stable/9/sys/cam/scsi/scsi_cd.c
==============================================================================
--- stable/9/sys/cam/scsi/scsi_cd.c	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/cam/scsi/scsi_cd.c	Sun Jul  1 05:13:50 2012	(r237873)
@@ -103,8 +103,7 @@ typedef enum {
 	CD_FLAG_RETRY_UA	= 0x0200,
 	CD_FLAG_VALID_MEDIA	= 0x0400,
 	CD_FLAG_VALID_TOC	= 0x0800,
-	CD_FLAG_SCTX_INIT	= 0x1000,
-	CD_FLAG_OPEN		= 0x2000
+	CD_FLAG_SCTX_INIT	= 0x1000
 } cd_flags;
 
 typedef enum {
@@ -357,6 +356,20 @@ cdinit(void)
 	}
 }
 
+/*
+ * Callback from GEOM, called when it has finished cleaning up its
+ * resources.
+ */
+static void
+cddiskgonecb(struct disk *dp)
+{
+	struct cam_periph *periph;
+
+	periph = (struct cam_periph *)dp->d_drv1;
+
+	cam_periph_release(periph);
+}
+
 static void
 cdoninvalidate(struct cam_periph *periph)
 {
@@ -388,7 +401,7 @@ cdoninvalidate(struct cam_periph *periph
 		camq_remove(&softc->changer->devq, softc->pinfo.index);
 
 	disk_gone(softc->disk);
-	xpt_print(periph->path, "lost device\n");
+	xpt_print(periph->path, "lost device, %d refs\n", periph->refcount);
 }
 
 static void
@@ -725,6 +738,7 @@ cdregister(struct cam_periph *periph, vo
 	softc->disk->d_open = cdopen;
 	softc->disk->d_close = cdclose;
 	softc->disk->d_strategy = cdstrategy;
+	softc->disk->d_gone = cddiskgonecb;
 	softc->disk->d_ioctl = cdioctl;
 	softc->disk->d_name = "cd";
 	cam_strvis(softc->disk->d_descr, cgd->inq_data.vendor,
@@ -746,6 +760,19 @@ cdregister(struct cam_periph *periph, vo
 	softc->disk->d_hba_device = cpi.hba_device;
 	softc->disk->d_hba_subvendor = cpi.hba_subvendor;
 	softc->disk->d_hba_subdevice = cpi.hba_subdevice;
+
+	/*
+	 * Acquire a reference to the periph before we register with GEOM.
+	 * We'll release this reference once GEOM calls us back (via
+	 * dadiskgonecb()) telling us that our provider has been freed.
+	 */
+	if (cam_periph_acquire(periph) != CAM_REQ_CMP) {
+		xpt_print(periph->path, "%s: lost periph during "
+			  "registration!\n", __func__);
+		cam_periph_lock(periph);
+		return (CAM_REQ_CMP_ERR);
+	}
+
 	disk_create(softc->disk, DISK_VERSION);
 	cam_periph_lock(periph);
 
@@ -999,14 +1026,14 @@ cdopen(struct disk *dp)
 	cam_periph_lock(periph);
 
 	if (softc->flags & CD_FLAG_INVALID) {
+		cam_periph_release_locked(periph);
 		cam_periph_unlock(periph);
-		cam_periph_release(periph);
 		return(ENXIO);
 	}
 
 	if ((error = cam_periph_hold(periph, PRIBIO | PCATCH)) != 0) {
+		cam_periph_release_locked(periph);
 		cam_periph_unlock(periph);
-		cam_periph_release(periph);
 		return (error);
 	}
 
@@ -1023,14 +1050,7 @@ cdopen(struct disk *dp)
 	CAM_DEBUG(periph->path, CAM_DEBUG_TRACE, ("leaving cdopen\n"));
 	cam_periph_unhold(periph);
 
-	/* Closes aren't symmetrical with opens, so fix up the refcounting. */
-	if ((softc->flags & CD_FLAG_OPEN) == 0) {
-		softc->flags |= CD_FLAG_OPEN;
-		cam_periph_unlock(periph);
-	} else {
-		cam_periph_unlock(periph);
-		cam_periph_release(periph);
-	}
+	cam_periph_unlock(periph);
 
 	return (0);
 }
@@ -1069,11 +1089,11 @@ cdclose(struct disk *dp)
 	/*
 	 * We'll check the media and toc again at the next open().
 	 */
-	softc->flags &= ~(CD_FLAG_VALID_MEDIA|CD_FLAG_VALID_TOC|CD_FLAG_OPEN);
+	softc->flags &= ~(CD_FLAG_VALID_MEDIA|CD_FLAG_VALID_TOC);
 
 	cam_periph_unhold(periph);
+	cam_periph_release_locked(periph);
 	cam_periph_unlock(periph);
-	cam_periph_release(periph);
 
 	return (0);
 }

Modified: stable/9/sys/cam/scsi/scsi_da.c
==============================================================================
--- stable/9/sys/cam/scsi/scsi_da.c	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/cam/scsi/scsi_da.c	Sun Jul  1 05:13:50 2012	(r237873)
@@ -1234,6 +1234,20 @@ dainit(void)
 	}
 }
 
+/*
+ * Callback from GEOM, called when it has finished cleaning up its
+ * resources.
+ */
+static void
+dadiskgonecb(struct disk *dp)
+{
+	struct cam_periph *periph;
+
+	periph = (struct cam_periph *)dp->d_drv1;
+
+	cam_periph_release(periph);
+}
+
 static void
 daoninvalidate(struct cam_periph *periph)
 {
@@ -1256,7 +1270,12 @@ daoninvalidate(struct cam_periph *periph
 	bioq_flush(&softc->bio_queue, NULL, ENXIO);
 	bioq_flush(&softc->delete_queue, NULL, ENXIO);
 
+	/*
+	 * Tell GEOM that we've gone away, we'll get a callback when it is
+	 * done cleaning up its resources.
+	 */
 	disk_gone(softc->disk);
+
 	xpt_print(periph->path, "lost device - %d outstanding, %d refs\n",
 		  softc->outstanding_cmds, periph->refcount);
 }
@@ -1634,6 +1653,7 @@ daregister(struct cam_periph *periph, vo
 	softc->disk->d_strategy = dastrategy;
 	softc->disk->d_dump = dadump;
 	softc->disk->d_getattr = dagetattr;
+	softc->disk->d_gone = dadiskgonecb;
 	softc->disk->d_name = "da";
 	softc->disk->d_drv1 = periph;
 	if (cpi.maxio == 0)
@@ -1656,6 +1676,19 @@ daregister(struct cam_periph *periph, vo
 	softc->disk->d_hba_device = cpi.hba_device;
 	softc->disk->d_hba_subvendor = cpi.hba_subvendor;
 	softc->disk->d_hba_subdevice = cpi.hba_subdevice;
+
+	/*
+	 * Acquire a reference to the periph before we register with GEOM.
+	 * We'll release this reference once GEOM calls us back (via
+	 * dadiskgonecb()) telling us that our provider has been freed.
+	 */
+	if (cam_periph_acquire(periph) != CAM_REQ_CMP) {
+		xpt_print(periph->path, "%s: lost periph during "
+			  "registration!\n", __func__);
+		mtx_lock(periph->sim->mtx);
+		return (CAM_REQ_CMP_ERR);
+	}
+
 	disk_create(softc->disk, DISK_VERSION);
 	mtx_lock(periph->sim->mtx);
 

Modified: stable/9/sys/dev/xen/blkfront/blkfront.c
==============================================================================
--- stable/9/sys/dev/xen/blkfront/blkfront.c	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/dev/xen/blkfront/blkfront.c	Sun Jul  1 05:13:50 2012	(r237873)
@@ -230,7 +230,7 @@ xlvbd_add(struct xb_softc *sc, blkif_sec
 	sc->xb_disk->d_mediasize = sectors * sector_size;
 	sc->xb_disk->d_maxsize = sc->max_request_size;
 	sc->xb_disk->d_flags = 0;
-	disk_create(sc->xb_disk, DISK_VERSION_00);
+	disk_create(sc->xb_disk, DISK_VERSION);
 
 	return error;
 }

Modified: stable/9/sys/geom/geom.h
==============================================================================
--- stable/9/sys/geom/geom.h	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/geom/geom.h	Sun Jul  1 05:13:50 2012	(r237873)
@@ -76,6 +76,7 @@ typedef void g_orphan_t (struct g_consum
 typedef void g_start_t (struct bio *);
 typedef void g_spoiled_t (struct g_consumer *);
 typedef void g_attrchanged_t (struct g_consumer *, const char *attr);
+typedef void g_provgone_t (struct g_provider *);
 typedef void g_dumpconf_t (struct sbuf *, const char *indent, struct g_geom *,
     struct g_consumer *, struct g_provider *);
 
@@ -106,7 +107,7 @@ struct g_class {
 	g_access_t		*access;
 	g_orphan_t		*orphan;
 	g_ioctl_t		*ioctl;
-	void			*spare1;
+	g_provgone_t		*providergone;
 	void			*spare2;
 	/*
 	 * The remaining elements are private
@@ -137,7 +138,7 @@ struct g_geom {
 	g_access_t		*access;
 	g_orphan_t		*orphan;
 	g_ioctl_t		*ioctl;
-	void			*spare0;
+	g_provgone_t		*providergone;
 	void			*spare1;
 	void			*softc;
 	unsigned		flags;

Modified: stable/9/sys/geom/geom_disk.c
==============================================================================
--- stable/9/sys/geom/geom_disk.c	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/geom/geom_disk.c	Sun Jul  1 05:13:50 2012	(r237873)
@@ -75,6 +75,7 @@ static g_fini_t g_disk_fini;
 static g_start_t g_disk_start;
 static g_ioctl_t g_disk_ioctl;
 static g_dumpconf_t g_disk_dumpconf;
+static g_provgone_t g_disk_providergone;
 
 static struct g_class g_disk_class = {
 	.name = "DISK",
@@ -84,6 +85,7 @@ static struct g_class g_disk_class = {
 	.start = g_disk_start,
 	.access = g_disk_access,
 	.ioctl = g_disk_ioctl,
+	.providergone = g_disk_providergone,
 	.dumpconf = g_disk_dumpconf,
 };
 
@@ -484,6 +486,33 @@ g_disk_create(void *arg, int flag)
 	g_error_provider(pp, 0);
 }
 
+/*
+ * We get this callback after all of the consumers have gone away, and just
+ * before the provider is freed.  If the disk driver provided a d_gone
+ * callback, let them know that it is okay to free resources -- they won't
+ * be getting any more accesses from GEOM.
+ */
+static void
+g_disk_providergone(struct g_provider *pp)
+{
+	struct disk *dp;
+	struct g_disk_softc *sc;
+
+	sc = (struct g_disk_softc *)pp->geom->softc;
+
+	/*
+	 * If the softc is already NULL, then we've probably been through
+	 * g_disk_destroy already; there is nothing for us to do anyway.
+	 */
+	if (sc == NULL)
+		return;
+
+	dp = sc->dp;
+
+	if (dp->d_gone != NULL)
+		dp->d_gone(dp);
+}
+
 static void
 g_disk_destroy(void *ptr, int flag)
 {
@@ -548,7 +577,7 @@ disk_alloc()
 void
 disk_create(struct disk *dp, int version)
 {
-	if (version != DISK_VERSION_00 && version != DISK_VERSION_01) {
+	if (version != DISK_VERSION_02) {
 		printf("WARNING: Attempt to add disk %s%d %s",
 		    dp->d_name, dp->d_unit,
 		    " using incompatible ABI version of disk(9)\n");

Modified: stable/9/sys/geom/geom_disk.h
==============================================================================
--- stable/9/sys/geom/geom_disk.h	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/geom/geom_disk.h	Sun Jul  1 05:13:50 2012	(r237873)
@@ -50,6 +50,7 @@ typedef	int	disk_open_t(struct disk *);
 typedef	int	disk_close_t(struct disk *);
 typedef	void	disk_strategy_t(struct bio *bp);
 typedef	int	disk_getattr_t(struct bio *bp);
+typedef	void	disk_gone_t(struct disk *);
 typedef	int	disk_ioctl_t(struct disk *, u_long cmd, void *data,
 			int fflag, struct thread *td);
 		/* NB: disk_ioctl_t SHALL be cast'able to d_ioctl_t */
@@ -77,6 +78,7 @@ struct disk {
 	disk_ioctl_t		*d_ioctl;
 	dumper_t		*d_dump;
 	disk_getattr_t		*d_getattr;
+	disk_gone_t		*d_gone;
 
 	/* Info fields from driver to geom_disk.c. Valid when open */
 	u_int			d_sectorsize;
@@ -110,7 +112,8 @@ void disk_attr_changed(struct disk *dp, 
 
 #define DISK_VERSION_00		0x58561059
 #define DISK_VERSION_01		0x5856105a
-#define DISK_VERSION		DISK_VERSION_01
+#define DISK_VERSION_02		0x5856105b
+#define DISK_VERSION		DISK_VERSION_02
 
 #endif /* _KERNEL */
 #endif /* _GEOM_GEOM_DISK_H_ */

Modified: stable/9/sys/geom/geom_subr.c
==============================================================================
--- stable/9/sys/geom/geom_subr.c	Sun Jul  1 04:26:51 2012	(r237872)
+++ stable/9/sys/geom/geom_subr.c	Sun Jul  1 05:13:50 2012	(r237873)
@@ -351,6 +351,7 @@ g_new_geomf(struct g_class *mp, const ch
 	gp->start = mp->start;
 	gp->spoiled = mp->spoiled;
 	gp->attrchanged = mp->attrchanged;
+	gp->providergone = mp->providergone;
 	gp->dumpconf = mp->dumpconf;
 	gp->access = mp->access;
 	gp->orphan = mp->orphan;
@@ -634,6 +635,13 @@ g_destroy_provider(struct g_provider *pp
 	LIST_REMOVE(pp, provider);
 	gp = pp->geom;
 	devstat_remove_entry(pp->stat);
+	/*
+	 * If a callback was provided, send notification that the provider
+	 * is now gone.
+	 */
+	if (gp->providergone != NULL)
+		gp->providergone(pp);
+
 	g_free(pp);
 	if ((gp->flags & G_GEOM_WITHER))
 		g_do_wither();



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201207010513.q615Dp2F067167>