From owner-svn-src-head@FreeBSD.ORG Fri Apr 10 02:49:06 2009 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C3B591065670; Fri, 10 Apr 2009 02:49:06 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.freebsd.org (Postfix) with ESMTP id 6B5C98FC18; Fri, 10 Apr 2009 02:49:06 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from phobos.local (pooker.samsco.org [168.103.85.57]) by pooker.samsco.org (8.14.2/8.14.2) with ESMTP id n3A2m1gQ054487; Thu, 9 Apr 2009 20:48:01 -0600 (MDT) (envelope-from scottl@samsco.org) Message-ID: <49DEB361.3000109@samsco.org> Date: Thu, 09 Apr 2009 20:48:01 -0600 From: Scott Long User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9 MIME-Version: 1.0 To: Andrew Thompson References: <200904031949.n33JnXfP031500@svn.freebsd.org> In-Reply-To: <200904031949.n33JnXfP031500@svn.freebsd.org> X-Enigmail-Version: 0.95.6 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.4 required=3.8 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.1.8 X-Spam-Checker-Version: SpamAssassin 3.1.8 (2007-02-13) on pooker.samsco.org Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r190677 - in head/sys: cam geom X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Apr 2009 02:49:07 -0000 I've never liked the root_mount_hold() thing, but I'll get into that below. This patch is absolutely wrong for CAM. I think it'll work on SIMs that set PIM_SEQSCAN, but it'll almost certainly cause root_mount_rel() to be called multiple times for drivers that don't set it, which is the vast majority of the drivers in the tree. It looks like it works for umass because umass only allows 1 target to be probed on the bus. But given the multiple reports to the mailing lists of failures caused by this revision, even if it works, it's only be accident. I've always felt that the right way to solve this was to use the existing infrastructure. The SI_SUB_INT_CONFIG_HOOKS sysinit was specifically created to allow drivers to hold up the root device discovery while they did actions that required threads, sleeping, and interrupts. Hook it directly, or use the config_intrhook API. Just about every RAID storage driver in the system uses this to solve precisely the problem that you are trying to solve. There is one legitimate problem here, though. CAM uses config_intrhook to drive its bus discovery scan, and there's no way to control the order in which these hooks are run. So you can have a situation where a driver like usb/umass gets put after CAM in the hook order, and thus doesn't get picked up before CAM is done with its scan. This is what needs to be fixed, and I think that the correct solution is to either create a new sysinit just for CAM, or have can hook the sysinit directly and ensure that it gets placed last in order. So please back this out, it's causing numerous reports of unbootable systems. I'm very happy to discuss the right way to get usb, umass, and CAM to configure themselves correctly into the boot sequence. I'll even go so far as to help get the rest of GEOM and the graid stuff working right so we can kill this root_mount_hold() hack entirely. Scott Andrew Thompson wrote: > Author: thompsa > Date: Fri Apr 3 19:49:33 2009 > New Revision: 190677 > URL: http://svn.freebsd.org/changeset/base/190677 > > Log: > Add interleaving root hold tokens from the CAM probe to disk_create and geom > provider tasting. This is needed for disk attachments that happen after threads > are running in the boot process. > > Tested by: rnoland > > Modified: > head/sys/cam/cam_xpt.c > head/sys/geom/geom.h > head/sys/geom/geom_disk.c > head/sys/geom/geom_disk.h > head/sys/geom/geom_subr.c > > Modified: head/sys/cam/cam_xpt.c > ============================================================================== > --- head/sys/cam/cam_xpt.c Fri Apr 3 19:46:12 2009 (r190676) > +++ head/sys/cam/cam_xpt.c Fri Apr 3 19:49:33 2009 (r190677) > @@ -5139,6 +5139,7 @@ xpt_find_device(struct cam_et *target, l > typedef struct { > union ccb *request_ccb; > struct ccb_pathinq *cpi; > + struct root_hold_token *roothold; > int counter; > } xpt_scan_bus_info; > > @@ -5201,6 +5202,7 @@ xpt_scan_bus(struct cam_periph *periph, > } > scan_info->request_ccb = request_ccb; > scan_info->cpi = &work_ccb->cpi; > + scan_info->roothold = root_mount_hold("CAM", M_NOWAIT); > > /* Cache on our stack so we can work asynchronously */ > max_target = scan_info->cpi->max_target; > @@ -5232,6 +5234,7 @@ xpt_scan_bus(struct cam_periph *periph, > printf("xpt_scan_bus: xpt_create_path failed" > " with status %#x, bus scan halted\n", > status); > + root_mount_rel(scan_info->roothold); > free(scan_info, M_CAMXPT); > request_ccb->ccb_h.status = status; > xpt_free_ccb(work_ccb); > @@ -5240,6 +5243,7 @@ xpt_scan_bus(struct cam_periph *periph, > } > work_ccb = xpt_alloc_ccb_nowait(); > if (work_ccb == NULL) { > + root_mount_rel(scan_info->roothold); > free(scan_info, M_CAMXPT); > xpt_free_path(path); > request_ccb->ccb_h.status = CAM_RESRC_UNAVAIL; > @@ -5353,6 +5357,7 @@ xpt_scan_bus(struct cam_periph *periph, > xpt_free_ccb(request_ccb); > xpt_free_ccb((union ccb *)scan_info->cpi); > request_ccb = scan_info->request_ccb; > + root_mount_rel(scan_info->roothold); > free(scan_info, M_CAMXPT); > request_ccb->ccb_h.status = CAM_REQ_CMP; > xpt_done(request_ccb); > @@ -5372,6 +5377,7 @@ xpt_scan_bus(struct cam_periph *periph, > xpt_free_ccb(request_ccb); > xpt_free_ccb((union ccb *)scan_info->cpi); > request_ccb = scan_info->request_ccb; > + root_mount_rel(scan_info->roothold); > free(scan_info, M_CAMXPT); > request_ccb->ccb_h.status = status; > xpt_done(request_ccb); > > Modified: head/sys/geom/geom.h > ============================================================================== > --- head/sys/geom/geom.h Fri Apr 3 19:46:12 2009 (r190676) > +++ head/sys/geom/geom.h Fri Apr 3 19:49:33 2009 (r190677) > @@ -193,6 +193,8 @@ struct g_provider { > /* Two fields for the implementing class to use */ > void *private; > u_int index; > + > + struct root_hold_token *roothold; > }; > > /* geom_dev.c */ > > Modified: head/sys/geom/geom_disk.c > ============================================================================== > --- head/sys/geom/geom_disk.c Fri Apr 3 19:46:12 2009 (r190676) > +++ head/sys/geom/geom_disk.c Fri Apr 3 19:49:33 2009 (r190677) > @@ -381,6 +381,7 @@ g_disk_create(void *arg, int flag) > printf("GEOM: new disk %s\n", gp->name); > dp->d_geom = gp; > g_error_provider(pp, 0); > + root_mount_rel(dp->d_roothold); > } > > static void > @@ -467,6 +468,7 @@ disk_create(struct disk *dp, int version > dp->d_sectorsize, DEVSTAT_ALL_SUPPORTED, > DEVSTAT_TYPE_DIRECT, DEVSTAT_PRIORITY_MAX); > dp->d_geom = NULL; > + dp->d_roothold = root_mount_hold(dp->d_name, M_WAITOK); > g_disk_ident_adjust(dp->d_ident, sizeof(dp->d_ident)); > g_post_event(g_disk_create, dp, M_WAITOK, dp, NULL); > } > > Modified: head/sys/geom/geom_disk.h > ============================================================================== > --- head/sys/geom/geom_disk.h Fri Apr 3 19:46:12 2009 (r190676) > +++ head/sys/geom/geom_disk.h Fri Apr 3 19:49:33 2009 (r190677) > @@ -88,6 +88,8 @@ struct disk { > > /* Fields private to the driver */ > void *d_drv1; > + > + struct root_hold_token *d_roothold; > }; > > #define DISKFLAG_NEEDSGIANT 0x1 > > Modified: head/sys/geom/geom_subr.c > ============================================================================== > --- head/sys/geom/geom_subr.c Fri Apr 3 19:46:12 2009 (r190676) > +++ head/sys/geom/geom_subr.c Fri Apr 3 19:49:33 2009 (r190677) > @@ -545,6 +545,10 @@ g_new_provider_event(void *arg, int flag > mp->taste(mp, pp, 0); > g_topology_assert(); > } > + if (pp->roothold != NULL) { > + root_mount_rel(pp->roothold); > + pp->roothold = NULL; > + } > } > > > @@ -581,6 +585,7 @@ g_new_providerf(struct g_geom *gp, const > pp->stat = devstat_new_entry(pp, -1, 0, DEVSTAT_ALL_SUPPORTED, > DEVSTAT_TYPE_DIRECT, DEVSTAT_PRIORITY_MAX); > LIST_INSERT_HEAD(&gp->provider, pp, provider); > + pp->roothold = root_mount_hold(pp->name, M_WAITOK); > g_post_event(g_new_provider_event, pp, M_WAITOK, pp, gp, NULL); > return (pp); > }