vdev_random_leaf() can loop forever

[tcaputi]

Describe the problem you're observing

While adding encrypted dataset support to ztest I cam across an issue with the mmp code. ztest began to eat 100% CPU without making any forward progress. When I examined the process with gdb, I found that vdev_random_leaf() was looping infinitely. It seems that ztest had gotten the pool into a state where it had one top-level mirror vdev with only one child which was not writable. This causes the following code to loop infinitely:

	while (!vd->vdev_ops->vdev_op_leaf) {
		child = vd->vdev_child[spa_get_random(vd->vdev_children)];

		if (!vdev_writeable(child))
			continue;

        ...

Describe how to reproduce the problem

This came up twice while doing lots of zloop runs over the course of a week. I suspect it should be fairly easy to reproduce by creating a pool with the layout I described above.

[behlendorf]

@ofaaland can you please take a look at this. It looks to me like we want to update vdev_random_leaf() such that it fails and returns NULL when there are no valid writable vdevs.

[ofaaland]

@behlendorf it does that now, so something else is amiss. I'll take a look.

[behlendorf]

@ofaaland I believe it will spin if you keep hitting the if (!vdev_writeable(child)) case where it calls continue.

[ofaaland]

@behlendorf yes, but if that happens I believe that means vdev_writeable(vd) == B_TRUE) but vdev_writeable(vd->vdev_child[X]) == B_FALSE for all X. I had believed this to be impossible, because SCL_STATE is held for the duration of the call into vdev_writeable(vd).

Perhaps I'm wrong. That is, that a vdev can be writeable when its children are not writeable, presumably because an error has not yet percolated up the vdev tree. Do you know?

Otherwise, it seems like I'll have to look at how the vdev state changes occur to be able to tell whether this is a ztest flaw, state change flaw, or vdev_random_leaf() flaw.

[behlendorf]

I don't believe SCL_STATE must be held when transitioning an individual vdev to a different state. Although that's commonly the case. I think it only must be taken to prevent the addition/removal of a vdev.

[ofaaland]

OK, that certainly would explain this.