Page MenuHomeFreeBSD

stand/zfs: don't fail boot if the first label is broken
Needs ReviewPublic

Authored by p.bruenn_beckhoff.com on May 19 2019, 10:58 PM.

Details

Summary

Booting from a vdev with a broken first label will fail, because
vdev_probe() never tries to read the next label. This revision calls
retries vdev_probe() until a good label was found or all labels are
broken.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24326
Build 23149: arc lint + arc unit

Event Timeline

Harbormaster completed remote builds in B24326: Diff 57572.
tsoome added a comment.Jun 2 2019, 7:28 PM

Something is off there. The for loop in vdev_probe() does continue in case of failures, but is intended to walk through all 4 labels, so why do we fail?

Yes, but not for all failures. If a label is found in the loop but later the associated data found invalid the vdev_probe() will fail even if there would be another label with better data.

tsoome added a comment.Jun 3 2019, 8:36 AM

Yes, but not for all failures. If a label is found in the loop but later the associated data found invalid the vdev_probe() will fail even if there would be another label with better data.

right, I see. I suggest to rework this solution even more; to something like:

vdev_probe()
{

 for (i = 0; i < VDEV_LABELS; i++) {
   vdev_read_label(i, &ub, &nvl);
   check if received  ub and nvl  are better than current.
}
if we have no good ub and nvl
  fail.

}

and vdev_read_label should read the specific label, perform initial tests and return good data.

This way we do not have to play the games with those fancy constants and games with inserting/deleting spa and vdev data.

Agreed, a sane vdev_probe() would be nice. But I have totally no idea how you want to omit cleaning up partially initialized spa/vdev objects without rewriting the whole file. Either some of the current error paths are dead code or things like vdev_init_from_nvlist() have to be replaced.

tsoome added a comment.Jun 5 2019, 7:15 PM

Agreed, a sane vdev_probe() would be nice. But I have totally no idea how you want to omit cleaning up partially initialized spa/vdev objects without rewriting the whole file. Either some of the current error paths are dead code or things like vdev_init_from_nvlist() have to be replaced.

I do not think that rewriting the whole file is needed:) And true, we probably need some configuration release during the process.