Paths

Table of Contentst

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

vdev_geom may associate multiple vdevs per g_consumer
ClosedPublic
Actions

Authored by asomers on Apr 13 2017, 11:22 PM.

Details

Reviewers

mav
gibbs

Commits

rS319268: MFC r318189:
rS318648: MFC r318189:
rS318189: vdev_geom may associate multiple vdevs per g_consumer

Summary

vdev_geom may associate multiple vdevs per g_consumer

vdev_geom.c currently uses the g_consumer's private field to point to a
vdev_t. That way, a GEOM event can cause a change to a ZFS vdev. For
example, when you remove a disk, the vdev's status will change to REMOVED.
However, vdev_geom will sometimes attach multiple vdevs to the same GEOM
consumer. If this happens, then geom events will only be propagated to one
of the vdevs.

Fix this by storing a linked list of vdevs in g_consumer's private field.

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

g_consumer.private now stores a linked list of vdev pointers

	  associated with the consumer instead of just a single vdev
	  pointer.

Change vdev_geom_set_physpath's signature to more closely match

	  vdev_geom_set_rotation_rate

Don't bother calling g_access in vdev_geom_set_physpath. It's

	  guaranteed that we've already accessed the consumer by the time we
	  get here.

Don't call vdev_geom_set_physpath in vdev_geom_attach. Instead,

	  call it in vdev_geom_open, after we know that the open has
	  succeeded.

PR: 218634

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 8883
Build 9267: CI src build	Jenkins
Build 9266: arc lint + arc unit

Event Timeline

asomers created this revision.Apr 13 2017, 11:22 PM

Herald added subscribers: delphij, imp. · View Herald TranscriptApr 13 2017, 11:22 PM

Harbormaster completed remote builds in B8722: Diff 27426.Apr 13 2017, 11:58 PM

gibbs added a subscriber: gibbs.Apr 21 2017, 8:19 PM

gibbs added inline comments.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
54	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.
95–98	Do you know why the access/release was required before and now is no longer necessary?
337	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?
891–900	Merge if blocks and put the comments inside the block.
892–894	A static_assert(), up by the declaration of consumer_priv_t, would be better.
895	Just do this assignment once outside the if block and remove the else block.

asomers marked 5 inline comments as done.Apr 24 2017, 3:21 PM

asomers added inline comments.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
54	Sure.
95–98	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.
337	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.
891–900	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.
892–894	good idea.

Respond to @gibbs's comments

Harbormaster completed remote builds in B8883: Diff 27694.Apr 24 2017, 7:26 PM

gibbs accepted this revision.May 9 2017, 6:34 PM

gibbs added inline comments.

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

891–900

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);
}

This revision is now accepted and ready to land.May 9 2017, 6:34 PM

asomers added inline comments.May 10 2017, 5:26 PM

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
891–900	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.

Closed by commit rS318189: vdev_geom may associate multiple vdevs per g_consumer (authored by asomers). · Explain WhyMay 11 2017, 4:27 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

cddl/

contrib/

opensolaris/

uts/

common/

fs/

zfs/

vdev_geom.c

113 lines

Diff 27694

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 Lines	vdev_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 Lines	vdev_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

vdev_geom may associate multiple vdevs per g_consumerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 27694

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

vdev_geom may associate multiple vdevs per g_consumer
ClosedPublic
Actions

Revision Contents
Changeset List