Page MenuHomeFreeBSD

vdev_geom may associate multiple vdevs per g_consumer
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gibbs added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
54 ↗(On Diff #27426)

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 ↗(On Diff #27426)

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

334 ↗(On Diff #27426)

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?

881–883 ↗(On Diff #27426)

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

884 ↗(On Diff #27426)

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

894–903 ↗(On Diff #27426)

Merge if blocks and put the comments inside the block.

asomers added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
54 ↗(On Diff #27426)

Sure.

95–98 ↗(On Diff #27426)

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.

334 ↗(On Diff #27426)

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.

881–883 ↗(On Diff #27426)

good idea.

894–903 ↗(On Diff #27426)

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 marked 4 inline comments as done.

Respond to @gibbs's comments

gibbs added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
894–903 ↗(On Diff #27426)

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
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
894–903 ↗(On Diff #27426)

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.

This revision was automatically updated to reflect the committed changes.