d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Delay GEOM disk_create() until CAM periph probe completes.

Browse Source 
Before this patch CAM periph drivers called both disk_alloc() and
disk_create() same time on periph creation.  But then prevented disks
from opening until the periph probe completion with cam_periph_hold().
As result, especially if disk misbehaves during the probe, GEOM event
thread, triggered to taste the disk, got blocked on open attempt,
potentially for a long time, unable to process other events.

This patch moves disk_create() call from periph creation to the end of
the probe. To allow disk_create() calls from non-sleepable CAM contexts
some of its duties requiring memory allocations are moved either back
to disk_alloc() or forward to g_disk_create(), so now disk_alloc() and
disk_add_alias() are the only disk methods that require sleeping.  If
disk fails during the probe disk_create() may just be skipped, going
directly to disk_destroy().  Other method calls during that time are
just ignored.  Since GEOM may now see the disks after CAM bus scan is
already completed, introduce per-periph boot hold functions. Enclosure
driver already had such mechanism, so just generalize it.

Reviewed by:	imp
MFC after:	1 month
Sponsored by:	iXsystems, Inc.
Differential Revision:	https://reviews.freebsd.org/D35784
This commit is contained in:
Alexander Motin 2022-07-14 15:38:14 -04:00
parent b62848b0c3
commit 90bcc81bc3
10 changed files with 134 additions and 128 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
sys/cam/ata/ata_da.c
View File

@ -1845,9 +1845,11 @@ adaregister(struct cam_periph *periph, void *arg)
TASK_INIT(&softc->sysctl_task, 0, adasysctlinit, periph);
/*
* Register this media as a disk
* Take a reference on the periph while adastart is called to finish
* the probe. The reference will be dropped in adaprobedone at the
* end of probe.
*/
(void)cam_periph_hold(periph, PRIBIO);
(void)cam_periph_acquire(periph);
cam_periph_unlock(periph);
snprintf(announce_buf, ADA_ANNOUNCETMP_SZ,
"kern.cam.ada.%d.quirks", periph->unit_number);
@ -1905,19 +1907,6 @@ adaregister(struct cam_periph *periph, void *arg)
softc->disk->d_name = "ada";
softc->disk->d_drv1 = periph;
softc->disk->d_unit = periph->unit_number;
/*
* Acquire a reference to the periph before we register with GEOM.
* We'll release this reference once GEOM calls us back (via
* adadiskgonecb()) telling us that our provider has been freed.
*/
if (cam_periph_acquire(periph) != 0) {
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);
dp = &softc->params;
@ -1960,6 +1949,9 @@ adaregister(struct cam_periph *periph, void *arg)
SBT_1S / ADA_ORDEREDTAG_INTERVAL * ada_default_timeout, 0,
adasendorderedtag, softc, C_PREL(1));
/* Released after probe when disk_create() call pass it to GEOM. */
cam_periph_hold_boot(periph);
if (ADA_RA >= 0 && softc->flags & ADA_FLAG_CAN_RAHEAD) {
softc->state = ADA_STATE_RAHEAD;
} else if (ADA_WC >= 0 && softc->flags & ADA_FLAG_CAN_WCACHE) {
@ -1977,7 +1969,6 @@ adaregister(struct cam_periph *periph, void *arg)
}
xpt_schedule(periph, CAM_PRIORITY_DEV);
return(CAM_REQ_CMP);
}
@ -2705,10 +2696,17 @@ adaprobedone(struct cam_periph *periph, union ccb *ccb)
adaschedule(periph);
if ((softc->flags & ADA_FLAG_ANNOUNCED) == 0) {
softc->flags |= ADA_FLAG_ANNOUNCED;
cam_periph_unhold(periph);
} else {
cam_periph_release_locked(periph);
/*
* We'll release this reference once GEOM calls us back via
* adadiskgonecb(), telling us that our provider has been freed.
*/
if (cam_periph_acquire(periph) == 0)
disk_create(softc->disk, DISK_VERSION);
cam_periph_release_boot(periph);
}
cam_periph_release_locked(periph);
}
static void

14
sys/cam/cam_periph.c
View File

@ -530,6 +530,20 @@ cam_periph_unhold(struct cam_periph *periph)
cam_periph_release_locked(periph);
}
void
cam_periph_hold_boot(struct cam_periph *periph)
{
root_mount_hold_token(periph->periph_name, &periph->periph_rootmount);
}
void
cam_periph_release_boot(struct cam_periph *periph)
{
root_mount_rel(&periph->periph_rootmount);
}
/*
* Look for the next unit number that is not currently in use for this
* peripheral type starting at "newunit". Also exclude unit numbers that

3
sys/cam/cam_periph.h
View File

@ -150,6 +150,7 @@ struct cam_periph {
ac_code deferred_ac;
struct task periph_run_task;
uma_zone_t ccb_zone;
struct root_hold_token periph_rootmount;
};
#define CAM_PERIPH_MAXMAPS 2
@ -175,6 +176,8 @@ void cam_periph_release_locked(struct cam_periph *periph);
void cam_periph_release_locked_buses(struct cam_periph *periph);
int cam_periph_hold(struct cam_periph *periph, int priority);
void cam_periph_unhold(struct cam_periph *periph);
void cam_periph_hold_boot(struct cam_periph *periph);
void cam_periph_release_boot(struct cam_periph *periph);
void cam_periph_invalidate(struct cam_periph *periph);
int cam_periph_mapmem(union ccb *ccb,
struct cam_periph_map_info *mapinfo,

24
sys/cam/nvme/nvme_da.c
View File

@ -886,7 +886,7 @@ ndaregister(struct cam_periph *periph, void *arg)
/*
* Register this media as a disk
*/
(void)cam_periph_hold(periph, PRIBIO);
(void)cam_periph_acquire(periph);
cam_periph_unlock(periph);
snprintf(announce_buf, sizeof(announce_buf),
"kern.cam.nda.%d.quirks", periph->unit_number);
@ -964,20 +964,7 @@ ndaregister(struct cam_periph *periph, void *arg)
if (nda_nvd_compat)
disk_add_alias(disk, "nvd");
/*
* Acquire a reference to the periph before we register with GEOM.
* We'll release this reference once GEOM calls us back (via
* ndadiskgonecb()) telling us that our provider has been freed.
*/
if (cam_periph_acquire(periph) != 0) {
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);
cam_periph_unhold(periph);
snprintf(announce_buf, sizeof(announce_buf),
"%juMB (%ju %u byte sectors)",
@ -1002,6 +989,15 @@ ndaregister(struct cam_periph *periph, void *arg)
ndaasync, periph, periph->path);
softc->state = NDA_STATE_NORMAL;
/*
* We'll release this reference once GEOM calls us back via
* ndadiskgonecb(), telling us that our provider has been freed.
*/
if (cam_periph_acquire(periph) == 0)
disk_create(softc->disk, DISK_VERSION);
cam_periph_release_locked(periph);
return(CAM_REQ_CMP);
}

34
sys/cam/scsi/scsi_cd.c
View File

@ -650,10 +650,10 @@ cdregister(struct cam_periph *periph, void *arg)
softc->minimum_command_size = 6;
/*
* Refcount and block open attempts until we are setup
* Can't block
* Take a reference on the periph while cdstart is called to finish the
* probe. The reference will be dropped in cddone at the end of probe.
*/
(void)cam_periph_hold(periph, PRIBIO);
(void)cam_periph_acquire(periph);
cam_periph_unlock(periph);
/*
* Load the user's default, if any.
@ -714,20 +714,6 @@ cdregister(struct cam_periph *periph, void *arg)
softc->disk->d_hba_subdevice = cpi.hba_subdevice;
snprintf(softc->disk->d_attachment, sizeof(softc->disk->d_attachment),
"%s%d", cpi.dev_name, cpi.unit_number);
/*
* 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) != 0) {
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);
/*
@ -748,6 +734,9 @@ cdregister(struct cam_periph *periph, void *arg)
0, cdmediapoll, periph, C_PREL(1));
}
/* Released after probe when disk_create() call pass it to GEOM. */
cam_periph_hold_boot(periph);
xpt_schedule(periph, CAM_PRIORITY_DEV);
return(CAM_REQ_CMP);
}
@ -1385,7 +1374,16 @@ cddone(struct cam_periph *periph, union ccb *done_ccb)
* operation.
*/
xpt_release_ccb(done_ccb);
cam_periph_unhold(periph);
/*
* We'll release this reference once GEOM calls us back via
* cddiskgonecb(), telling us that our provider has been freed.
*/
if (cam_periph_acquire(periph) == 0)
disk_create(softc->disk, DISK_VERSION);
cam_periph_release_boot(periph);
cam_periph_release_locked(periph);
return;
}
case CD_CCB_TUR:

51
sys/cam/scsi/scsi_da.c
View File

@ -329,7 +329,6 @@ typedef enum {
DA_REF_OPEN = 1,
DA_REF_OPEN_HOLD,
DA_REF_CLOSE_HOLD,
DA_REF_PROBE_HOLD,
DA_REF_TUR,
DA_REF_GEOM,
DA_REF_SYSCTL,
@ -2602,9 +2601,17 @@ daprobedone(struct cam_periph *periph, union ccb *ccb)
wakeup(&softc->disk->d_mediasize);
if ((softc->flags & DA_FLAG_ANNOUNCED) == 0) {
softc->flags |= DA_FLAG_ANNOUNCED;
da_periph_unhold(periph, DA_REF_PROBE_HOLD);
} else
da_periph_release_locked(periph, DA_REF_REPROBE);
/*
* We'll release this reference once GEOM calls us back via
* dadiskgonecb(), telling us that our provider has been freed.
*/
if (da_periph_acquire(periph, DA_REF_GEOM) == 0)
disk_create(softc->disk, DISK_VERSION);
cam_periph_release_boot(periph);
}
da_periph_release_locked(periph, DA_REF_REPROBE);
}
static void
@ -2864,14 +2871,10 @@ daregister(struct cam_periph *periph, void *arg)
}
/*
* Take an exclusive section lock on the periph while dastart is called
* to finish the probe. The lock will be dropped in dadone at the end
* of probe. This locks out daopen and daclose from racing with the
* probe.
*
* XXX if cam_periph_hold returns an error, we don't hold a refcount.
* Take a reference on the periph while dastart is called to finish the
* probe. The reference will be dropped in dadone at the end of probe.
*/
(void)da_periph_hold(periph, PRIBIO, DA_REF_PROBE_HOLD);
(void)da_periph_acquire(periph, DA_REF_REPROBE);
/*
* Schedule a periodic event to occasionally send an
@ -2967,20 +2970,6 @@ daregister(struct cam_periph *periph, void *arg)
softc->disk->d_hba_subdevice = cpi.hba_subdevice;
snprintf(softc->disk->d_attachment, sizeof(softc->disk->d_attachment),
"%s%d", cpi.dev_name, cpi.unit_number);
/*
* 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 (da_periph_acquire(periph, DA_REF_GEOM) != 0) {
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);
/*
@ -2994,14 +2983,6 @@ daregister(struct cam_periph *periph, void *arg)
AC_ADVINFO_CHANGED | AC_SCSI_AEN | AC_UNIT_ATTENTION |
AC_INQ_CHANGED, daasync, periph, periph->path);
/*
* Emit an attribute changed notification just in case
* physical path information arrived before our async
* event handler was registered, but after anyone attaching
* to our disk device polled it.
*/
disk_attr_changed(softc->disk, "GEOM::physpath", M_NOWAIT);
/*
* Schedule a periodic media polling events.
*/
@ -3013,8 +2994,10 @@ daregister(struct cam_periph *periph, void *arg)
0, damediapoll, periph, C_PREL(1));
}
xpt_schedule(periph, CAM_PRIORITY_DEV);
/* Released after probe when disk_create() call pass it to GEOM. */
cam_periph_hold_boot(periph);
xpt_schedule(periph, CAM_PRIORITY_DEV);
return(CAM_REQ_CMP);
}

9
sys/cam/scsi/scsi_enc.c
View File

@ -205,7 +205,7 @@ enc_dtor(struct cam_periph *periph)
if (enc->enc_vec.softc_cleanup != NULL)
enc->enc_vec.softc_cleanup(enc);
root_mount_rel(&enc->enc_rootmount);
cam_periph_release_boot(periph);
ENC_FREE(enc);
}
@ -842,7 +842,7 @@ enc_daemon(void *arg)
* We've been through our state machine at least
* once. Allow the transition to userland.
*/
root_mount_rel(&enc->enc_rootmount);
cam_periph_release_boot(enc->periph);
callout_reset_sbt(&enc->status_updater, 60 * SBT_1S, 0,
enc_status_updater, enc, C_PREL(1));
@ -937,9 +937,8 @@ enc_ctor(struct cam_periph *periph, void *arg)
* through our state machine so that physical path data is
* present.
*/
if (enc->enc_vec.poll_status != NULL) {
root_mount_hold_token(periph->periph_name, &enc->enc_rootmount);
}
if (enc->enc_vec.poll_status != NULL)
cam_periph_hold_boot(periph);
/*
* The softc field is set only once the enc is fully initialized

2
sys/cam/scsi/scsi_enc_internal.h
View File

@ -161,8 +161,6 @@ struct enc_softc {
struct enc_fsm_state *enc_fsm_states;
struct root_hold_token enc_rootmount;
#define ENC_ANNOUNCE_SZ 400
char announce_buf[ENC_ANNOUNCE_SZ];
};

85
sys/geom/geom_disk.c
View File

@ -721,6 +721,11 @@ g_disk_create(void *arg, int flag)
sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO);
mtx_init(&sc->done_mtx, "g_disk_done", NULL, MTX_DEF);
sc->dp = dp;
if (dp->d_devstat == NULL) {
dp->d_devstat = devstat_new_entry(dp->d_name, dp->d_unit,
dp->d_sectorsize, DEVSTAT_ALL_SUPPORTED,
DEVSTAT_TYPE_DIRECT, DEVSTAT_PRIORITY_MAX);
}
sc->d_devstat = dp->d_devstat;
gp = g_new_geomf(&g_disk_class, "%s%d", dp->d_name, dp->d_unit);
gp->softc = sc;
@ -862,6 +867,9 @@ disk_alloc(void)
dp = g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO);
LIST_INIT(&dp->d_aliases);
dp->d_init_level = DISK_INIT_NONE;
dp->d_cevent = g_alloc_event(M_WAITOK);
dp->d_devent = g_alloc_event(M_WAITOK);
return (dp);
}
@ -888,31 +896,40 @@ disk_create(struct disk *dp, int version)
KASSERT(dp->d_name != NULL, ("disk_create need d_name"));
KASSERT(*dp->d_name != 0, ("disk_create need d_name"));
KASSERT(strlen(dp->d_name) < SPECNAMELEN - 4, ("disk name too long"));
if (dp->d_devstat == NULL)
dp->d_devstat = devstat_new_entry(dp->d_name, dp->d_unit,
dp->d_sectorsize, DEVSTAT_ALL_SUPPORTED,
DEVSTAT_TYPE_DIRECT, DEVSTAT_PRIORITY_MAX);
dp->d_geom = NULL;
dp->d_event = g_alloc_event(M_WAITOK);
dp->d_init_level = DISK_INIT_NONE;
g_disk_ident_adjust(dp->d_ident, sizeof(dp->d_ident));
g_post_event(g_disk_create, dp, M_WAITOK, dp, NULL);
dp->d_init_level = DISK_INIT_CREATE;
KASSERT(dp->d_cevent != NULL,
("Disk create for %p with event NULL", dp));
g_post_event_ep(g_disk_create, dp, dp->d_cevent, dp, NULL);
}
void
disk_destroy(struct disk *dp)
{
struct disk_alias *dap, *daptmp;
KASSERT(dp->d_event != NULL,
/* If disk_create() was never called, just free the resources. */
if (dp->d_init_level < DISK_INIT_CREATE) {
if (dp->d_devstat != NULL)
devstat_remove_entry(dp->d_devstat);
LIST_FOREACH_SAFE(dap, &dp->d_aliases, da_next, daptmp)
g_free(dap);
g_free(dp->d_cevent);
g_free(dp->d_devent);
g_free(dp);
return;
}
KASSERT(dp->d_devent != NULL,
("Disk destroy for %p with event NULL", dp));
disk_gone(dp);
dp->d_destroyed = 1;
g_cancel_event(dp);
if (dp->d_devstat != NULL)
devstat_remove_entry(dp->d_devstat);
g_post_event_ep(g_disk_destroy, dp, dp->d_event, NULL);
g_post_event_ep(g_disk_destroy, dp, dp->d_devent, NULL);
}
void
@ -979,14 +996,14 @@ disk_gone(struct disk *dp)
void
disk_attr_changed(struct disk *dp, const char *attr, int flag)
{
struct g_geom *gp;
struct g_geom *gp = dp->d_geom;
struct g_provider *pp;
char devnamebuf[128];
gp = dp->d_geom;
if (gp != NULL)
LIST_FOREACH(pp, &gp->provider, provider)
(void)g_attr_changed(pp, attr, flag);
if (gp == NULL)
return;
LIST_FOREACH(pp, &gp->provider, provider)
(void)g_attr_changed(pp, attr, flag);
snprintf(devnamebuf, sizeof(devnamebuf), "devname=%s%d", dp->d_name,
dp->d_unit);
devctl_notify("GEOM", "disk", attr, devnamebuf);
@ -995,34 +1012,32 @@ disk_attr_changed(struct disk *dp, const char *attr, int flag)
void
disk_media_changed(struct disk *dp, int flag)
{
struct g_geom *gp;
struct g_geom *gp = dp->d_geom;
struct g_provider *pp;
gp = dp->d_geom;
if (gp != NULL) {
pp = LIST_FIRST(&gp->provider);
if (pp != NULL) {
KASSERT(LIST_NEXT(pp, provider) == NULL,
("geom %p has more than one provider", gp));
g_media_changed(pp, flag);
}
if (gp == NULL)
return;
pp = LIST_FIRST(&gp->provider);
if (pp != NULL) {
KASSERT(LIST_NEXT(pp, provider) == NULL,
("geom %p has more than one provider", gp));
g_media_changed(pp, flag);
}
}
void
disk_media_gone(struct disk *dp, int flag)
{
struct g_geom *gp;
struct g_geom *gp = dp->d_geom;
struct g_provider *pp;
gp = dp->d_geom;
if (gp != NULL) {
pp = LIST_FIRST(&gp->provider);
if (pp != NULL) {
KASSERT(LIST_NEXT(pp, provider) == NULL,
("geom %p has more than one provider", gp));
g_media_gone(pp, flag);
}
if (gp == NULL)
return;
pp = LIST_FIRST(&gp->provider);
if (pp != NULL) {
KASSERT(LIST_NEXT(pp, provider) == NULL,
("geom %p has more than one provider", gp));
g_media_gone(pp, flag);
}
}

4
sys/geom/geom_disk.h
View File

@ -69,6 +69,7 @@ struct devstat;
typedef enum {
DISK_INIT_NONE,
DISK_INIT_CREATE,
DISK_INIT_START,
DISK_INIT_DONE
} disk_init_level;
@ -125,7 +126,8 @@ struct disk {
/* Fields private to geom_disk, to be moved on next version bump */
LIST_HEAD(,disk_alias) d_aliases;
struct g_event *d_event;
struct g_event *d_cevent;
struct g_event *d_devent;
};
#define DISKFLAG_RESERVED 0x0001 /* Was NEEDSGIANT */