Page MenuHomeFreeBSD
Differential D10391 Diff 27694 sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c

Changeset View

Standalone View

View Options

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
static g_attrchanged_t vdev_geom_attrchanged; static g_attrchanged_t vdev_geom_attrchanged;
struct g_class zfs_vdev_class = { struct g_class zfs_vdev_class = {
.name = "ZFS::VDEV", .name = "ZFS::VDEV",
.version = G_VERSION, .version = G_VERSION,
.attrchanged = vdev_geom_attrchanged, .attrchanged = vdev_geom_attrchanged,
}; };
struct consumer_vdev_elem {
SLIST_ENTRY(consumer_vdev_elem) elems;
vdev_t *vd;
gibbsUnsubmitted
Done
Inline Actions

I'm always in the habit of putting the links at the beginning of the struct. It doesn't matter for singly linked structs, but with doubly linked, this increases the chances that both links are in the same cache line.

gibbs: I'm always in the habit of putting the links at the beginning of the struct. It doesn't matter…
asomersAuthorUnsubmitted
Not Done
Inline Actions

Sure.

asomers: Sure.
};
SLIST_HEAD(consumer_priv_t, consumer_vdev_elem);
_Static_assert(sizeof(((struct g_consumer*)NULL)->private)
== sizeof(struct consumer_priv_t*),
"consumer_priv_t* can't be stored in g_consumer.private");
DECLARE_GEOM_CLASS(zfs_vdev_class, zfs_vdev); DECLARE_GEOM_CLASS(zfs_vdev_class, zfs_vdev);
SYSCTL_DECL(_vfs_zfs_vdev); SYSCTL_DECL(_vfs_zfs_vdev);
/* Don't send BIO_FLUSH. */ /* Don't send BIO_FLUSH. */
static int vdev_geom_bio_flush_disable; static int vdev_geom_bio_flush_disable;
SYSCTL_INT(_vfs_zfs_vdev, OID_AUTO, bio_flush_disable, CTLFLAG_RWTUN, SYSCTL_INT(_vfs_zfs_vdev, OID_AUTO, bio_flush_disable, CTLFLAG_RWTUN,
&vdev_geom_bio_flush_disable, 0, "Disable BIO_FLUSH"); &vdev_geom_bio_flush_disable, 0, "Disable BIO_FLUSH");
/* Don't send BIO_DELETE. */ /* Don't send BIO_DELETE. */
Show All 20 Linesvdev_geom_set_rotation_rate(vdev_t *vd, struct g_consumer *cp)
error = g_getattr("GEOM::rotation_rate", cp, &rate); error = g_getattr("GEOM::rotation_rate", cp, &rate);
if (error == 0) if (error == 0)
vd->vdev_rotation_rate = rate; vd->vdev_rotation_rate = rate;
else else
vd->vdev_rotation_rate = VDEV_RATE_UNKNOWN; vd->vdev_rotation_rate = VDEV_RATE_UNKNOWN;
} }
static void static void
vdev_geom_set_physpath(struct g_consumer *cp, boolean_t do_null_update) vdev_geom_set_physpath(vdev_t *vd, struct g_consumer *cp,
boolean_t do_null_update)
{ {
boolean_t needs_update = B_FALSE; boolean_t needs_update = B_FALSE;
vdev_t *vd;
char *physpath; char *physpath;
int error, physpath_len; int error, physpath_len;
if (g_access(cp, 1, 0, 0) != 0)
return;
vd = cp->private;
gibbsUnsubmitted
Done
Inline Actions

Do you know why the access/release was required before and now is no longer necessary?

gibbs: Do you know why the access/release was required before and now is no longer necessary?
asomersAuthorUnsubmitted
Not Done
Inline Actions

The access/release was originally added to SpectraBSD by @gibbs back in 2011 (p4 489257). I don't know why , but it doesn't look like it's required now.

asomers: The access/release was originally added to SpectraBSD by @gibbs back in 2011 (p4 489257). I…
physpath_len = MAXPATHLEN; physpath_len = MAXPATHLEN;
physpath = g_malloc(physpath_len, M_WAITOK|M_ZERO); physpath = g_malloc(physpath_len, M_WAITOK|M_ZERO);
error = g_io_getattr("GEOM::physpath", cp, &physpath_len, physpath); error = g_io_getattr("GEOM::physpath", cp, &physpath_len, physpath);
g_access(cp, -1, 0, 0);
if (error == 0) { if (error == 0) {
char *old_physpath; char *old_physpath;
/* g_topology lock ensures that vdev has not been closed */ /* g_topology lock ensures that vdev has not been closed */
g_topology_assert(); g_topology_assert();
old_physpath = vd->vdev_physpath; old_physpath = vd->vdev_physpath;
vd->vdev_physpath = spa_strdup(physpath); vd->vdev_physpath = spa_strdup(physpath);
Show All 14 Linesvdev_geom_set_physpath(vdev_t *vd, struct g_consumer *cp,
if (needs_update) if (needs_update)
spa_async_request(vd->vdev_spa, SPA_ASYNC_CONFIG_UPDATE); spa_async_request(vd->vdev_spa, SPA_ASYNC_CONFIG_UPDATE);
} }
static void static void
vdev_geom_attrchanged(struct g_consumer *cp, const char *attr) vdev_geom_attrchanged(struct g_consumer *cp, const char *attr)
{ {
vdev_t *vd;
char *old_physpath; char *old_physpath;
struct consumer_priv_t *priv;
struct consumer_vdev_elem *elem;
int error; int error;
vd = cp->private; priv = (struct consumer_priv_t*)&cp->private;
if (vd == NULL) if (SLIST_EMPTY(priv))
return; return;
SLIST_FOREACH(elem, priv, elems) {
vdev_t *vd = elem->vd;
if (strcmp(attr, "GEOM::rotation_rate") == 0) { if (strcmp(attr, "GEOM::rotation_rate") == 0) {
vdev_geom_set_rotation_rate(vd, cp); vdev_geom_set_rotation_rate(vd, cp);
return; return;
} }
if (strcmp(attr, "GEOM::physpath") == 0) { if (strcmp(attr, "GEOM::physpath") == 0) {
vdev_geom_set_physpath(cp, /*do_null_update*/B_TRUE); vdev_geom_set_physpath(vd, cp, /*null_update*/B_TRUE);
return; return;
} }
} }
}
static void static void
vdev_geom_orphan(struct g_consumer *cp) vdev_geom_orphan(struct g_consumer *cp)
{ {
vdev_t *vd; struct consumer_priv_t *priv;
struct consumer_vdev_elem *elem;
g_topology_assert(); g_topology_assert();
vd = cp->private; priv = (struct consumer_priv_t*)&cp->private;
if (vd == NULL) { if (SLIST_EMPTY(priv))
/* Vdev close in progress. Ignore the event. */ /* Vdev close in progress. Ignore the event. */
return; return;
}
/* /*
* Orphan callbacks occur from the GEOM event thread. * Orphan callbacks occur from the GEOM event thread.
* Concurrent with this call, new I/O requests may be * Concurrent with this call, new I/O requests may be
* working their way through GEOM about to find out * working their way through GEOM about to find out
* (only once executed by the g_down thread) that we've * (only once executed by the g_down thread) that we've
* been orphaned from our disk provider. These I/Os * been orphaned from our disk provider. These I/Os
* must be retired before we can detach our consumer. * must be retired before we can detach our consumer.
* This is most easily achieved by acquiring the * This is most easily achieved by acquiring the
* SPA ZIO configuration lock as a writer, but doing * SPA ZIO configuration lock as a writer, but doing
* so with the GEOM topology lock held would cause * so with the GEOM topology lock held would cause
* a lock order reversal. Instead, rely on the SPA's * a lock order reversal. Instead, rely on the SPA's
* async removal support to invoke a close on this * async removal support to invoke a close on this
* vdev once it is safe to do so. * vdev once it is safe to do so.
*/ */
SLIST_FOREACH(elem, priv, elems) {
vdev_t *vd = elem->vd;
vd->vdev_remove_wanted = B_TRUE; vd->vdev_remove_wanted = B_TRUE;
spa_async_request(vd->vdev_spa, SPA_ASYNC_REMOVE); spa_async_request(vd->vdev_spa, SPA_ASYNC_REMOVE);
} }
}
static struct g_consumer * static struct g_consumer *
vdev_geom_attach(struct g_provider *pp, vdev_t *vd) vdev_geom_attach(struct g_provider *pp, vdev_t *vd)
{ {
struct g_geom *gp; struct g_geom *gp;
struct g_consumer *cp; struct g_consumer *cp;
int error; int error;
▲ Show 20 Lines • Show All 70 Lines • ▼ Show 20 Lines if (cp == NULL) {
ZFS_LOG(1, "%s(%d): g_access failed: %d\n", ZFS_LOG(1, "%s(%d): g_access failed: %d\n",
__func__, __LINE__, error); __func__, __LINE__, error);
return (NULL); return (NULL);
} }
ZFS_LOG(1, "Used existing consumer for %s.", pp->name); ZFS_LOG(1, "Used existing consumer for %s.", pp->name);
} }
} }
/* if (vd != NULL)
* BUG: cp may already belong to a vdev. This could happen if:
* 1) That vdev is a shared spare, or
* 2) We are trying to reopen a missing vdev and we are scanning by
* guid. In that case, we'll ultimately fail to open this consumer,
* but not until after setting the private field.
* The solution is to:
* 1) Don't set the private field until after the open succeeds, and
* 2) Set it to a linked list of vdevs, not just a single vdev
*/
cp->private = vd;
if (vd != NULL) {
vd->vdev_tsd = cp; vd->vdev_tsd = cp;
vdev_geom_set_physpath(cp, /*do_null_update*/B_FALSE);
}
cp->flags |= G_CF_DIRECT_SEND | G_CF_DIRECT_RECEIVE; cp->flags |= G_CF_DIRECT_SEND | G_CF_DIRECT_RECEIVE;
return (cp); return (cp);
} }
static void static void
vdev_geom_detach(struct g_consumer *cp, boolean_t open_for_read) vdev_geom_detach(struct g_consumer *cp, boolean_t open_for_read)
{ {
struct g_geom *gp; struct g_geom *gp;
vdev_t *vd;
g_topology_assert(); g_topology_assert();
ZFS_LOG(1, "Detaching from %s.", ZFS_LOG(1, "Detaching from %s.",
cp->provider && cp->provider->name ? cp->provider->name : "NULL"); cp->provider && cp->provider->name ? cp->provider->name : "NULL");
vd = cp->private;
cp->private = NULL;
gp = cp->geom; gp = cp->geom;
if (open_for_read) if (open_for_read)
g_access(cp, -1, 0, -1); g_access(cp, -1, 0, -1);
/* Destroy consumer on last close. */ /* Destroy consumer on last close. */
if (cp->acr == 0 && cp->ace == 0) { if (cp->acr == 0 && cp->ace == 0) {
if (cp->acw > 0) if (cp->acw > 0)
g_access(cp, 0, -cp->acw, 0); g_access(cp, 0, -cp->acw, 0);
if (cp->provider != NULL) { if (cp->provider != NULL) {
Show All 9 Lines if (LIST_EMPTY(&gp->consumer)) {
g_wither_geom(gp, ENXIO); g_wither_geom(gp, ENXIO);
} }
} }
static void static void
vdev_geom_close_locked(vdev_t *vd) vdev_geom_close_locked(vdev_t *vd)
{ {
struct g_consumer *cp; struct g_consumer *cp;
struct consumer_priv_t *priv;
struct consumer_vdev_elem *elem, *elem_temp;
g_topology_assert(); g_topology_assert();
cp = vd->vdev_tsd; cp = vd->vdev_tsd;
vd->vdev_tsd = NULL;
vd->vdev_delayed_close = B_FALSE; vd->vdev_delayed_close = B_FALSE;
if (cp == NULL) if (cp == NULL)
return; return;
ZFS_LOG(1, "Closing access to %s.", cp->provider->name); ZFS_LOG(1, "Closing access to %s.", cp->provider->name);
KASSERT(cp->private != NULL, ("%s: cp->private is NULL", __func__));
priv = (struct consumer_priv_t*)&cp->private;
vd->vdev_tsd = NULL;
SLIST_FOREACH_SAFE(elem, priv, elems, elem_temp) {
if (elem->vd == vd) {
gibbsUnsubmitted
Done
Inline Actions

How can this check fail? Should there be an assert before the if, so we crash on debug and just leak (maybe log) on production builds?

gibbs: How can this check fail? Should there be an assert before the if, so we crash on debug and just…
asomersAuthorUnsubmitted
Not Done
Inline Actions

Easy. If the vdev is a shared spare and/or an active spare, then there will be more than one element in the list. vdev_geom_close_locked only closes one vdev at a time. We don't want to remove other vdevs from the list.

asomers: Easy. If the vdev is a shared spare and/or an active spare, then there will be more than one…
SLIST_REMOVE(priv, elem, consumer_vdev_elem, elems);
g_free(elem);
}
}
vdev_geom_detach(cp, B_TRUE); vdev_geom_detach(cp, B_TRUE);
} }
/* /*
* Issue one or more bios to the vdev in parallel * Issue one or more bios to the vdev in parallel
* cmds, datas, offsets, errors, and sizes are arrays of length ncmds. Each IO * cmds, datas, offsets, errors, and sizes are arrays of length ncmds. Each IO
* operation is described by parallel entries from each array. There may be * operation is described by parallel entries from each array. There may be
▲ Show 20 Lines • Show All 520 Lines • ▼ Show 20 Lines if (cp == NULL) {
} }
if (error != 0) { if (error != 0) {
printf("ZFS WARNING: Unable to open %s for writing (error=%d).\n", printf("ZFS WARNING: Unable to open %s for writing (error=%d).\n",
vd->vdev_path, error); vd->vdev_path, error);
vdev_geom_close_locked(vd); vdev_geom_close_locked(vd);
cp = NULL; cp = NULL;
} }
} }
if (cp != NULL) {
struct consumer_priv_t *priv;
struct consumer_vdev_elem *elem;
priv = (struct consumer_priv_t*)&cp->private;
if (cp->private == NULL)
SLIST_INIT(priv);
elem = g_malloc(sizeof(*elem), M_WAITOK|M_ZERO);
elem->vd = vd;
SLIST_INSERT_HEAD(priv, elem, elems);
}
/* Fetch initial physical path information for this device. */ /* Fetch initial physical path information for this device. */
if (cp != NULL) if (cp != NULL)
vdev_geom_attrchanged(cp, "GEOM::physpath"); vdev_geom_attrchanged(cp, "GEOM::physpath");
if (cp != NULL) {
gibbsUnsubmitted
Done
Inline Actions

A static_assert(), up by the declaration of consumer_priv_t, would be better.

gibbs: A static_assert(), up by the declaration of consumer_priv_t, would be better.
asomersAuthorUnsubmitted
Not Done
Inline Actions

good idea.

asomers: good idea.
/* Set other GEOM characteristics */
gibbsUnsubmitted
Done
Inline Actions

Just do this assignment once outside the if block and remove the else block.

gibbs: Just do this assignment once outside the if block and remove the else block.
vdev_geom_set_physpath(vd, cp, /*do_null_update*/B_FALSE);
vdev_geom_set_rotation_rate(vd, cp);
}
g_topology_unlock(); g_topology_unlock();
gibbsUnsubmitted
Not Done
Inline Actions

Merge if blocks and put the comments inside the block.

gibbs: Merge if blocks and put the comments inside the block.
asomersAuthorUnsubmitted
Not Done
Inline Actions

Are you sure you want to merge the if blocks? It will necessitate duplicating the g_topology_unlock(); PICKUP_GIANT(); lines, which will actually increase the SLOC count.

asomers: Are you sure you want to merge the if blocks? It will necessitate duplicating the…
gibbsUnsubmitted
Not Done
Inline Actions

I don't understand how additional locking is required to change this:

if (cp != NULL)
        vdev_geom_attrchanged(cp, "GEOM::physpath");

if (cp != NULL) {
        /* Set other GEOM characteristics */
        vdev_geom_set_physpath(vd, cp, /*do_null_update*/B_FALSE);
        vdev_geom_set_rotation_rate(vd, cp);
}

to this:

if (cp != NULL) {
        vdev_geom_attrchanged(cp, "GEOM::physpath");

        /* Set other GEOM characteristics */
        vdev_geom_set_physpath(vd, cp, /*do_null_update*/B_FALSE);
        vdev_geom_set_rotation_rate(vd, cp);
}
gibbs: I don't understand how additional locking is required to change this: if (cp != NULL)…
asomersAuthorUnsubmitted
Not Done
Inline Actions

Oh, you were talking about the if blocks at lines 891 and 894. I thought you were talking about the if block at line 902, too. Changing that into an else block is what would require duplicating the g_topology_lock and PICKUP_GIANT lines. I can do as you suggested.

asomers: Oh, you were talking about the if blocks at lines 891 and 894. I thought you were talking…
PICKUP_GIANT(); PICKUP_GIANT();
if (cp == NULL) { if (cp == NULL) {
vd->vdev_stat.vs_aux = VDEV_AUX_OPEN_FAILED; vd->vdev_stat.vs_aux = VDEV_AUX_OPEN_FAILED;
return (error); return (error);
} }
skip_open: skip_open:
pp = cp->provider; pp = cp->provider;
Show All 12 Lines if (pp->stripesize > (1 << *logical_ashift) && ISP2(pp->stripesize) &&
pp->stripesize <= (1 << SPA_MAXASHIFT) && pp->stripeoffset == 0) pp->stripesize <= (1 << SPA_MAXASHIFT) && pp->stripeoffset == 0)
*physical_ashift = highbit(pp->stripesize) - 1; *physical_ashift = highbit(pp->stripesize) - 1;
/* /*
* Clear the nowritecache settings, so that on a vdev_reopen() * Clear the nowritecache settings, so that on a vdev_reopen()
* we will try again. * we will try again.
*/ */
vd->vdev_nowritecache = B_FALSE; vd->vdev_nowritecache = B_FALSE;
/*
* Determine the device's rotation rate.
*/
vdev_geom_set_rotation_rate(vd, cp);
return (0); return (0);
} }
static void static void
vdev_geom_close(vdev_t *vd) vdev_geom_close(vdev_t *vd)
{ {
▲ Show 20 Lines • Show All 171 Lines • Show Last 20 Lines