svn commit: r190677 - in head/sys: cam geom
Scott Long
scottl at samsco.org
Thu Apr 9 19:49:18 PDT 2009
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);
> }
More information about the svn-src-head
mailing list