In D50947#1171973, @zlei wrote:

P.S. Why do you want to merge that to a stable branch?

The structs has __packed attribute so it is unlikely to have wrong size. Well I'm a little paranoiac that may happen in some way :)

Please include the link to the exp-run bugzilla into the commit message.

Add missing 'break;'. We aren't using _SAFE iterator here, and we don't
expect any more matches.

As Alexander noted, a single label doesn't store information about the
entire pool, but only about one top-level vdev. Thus, we should blow
away only this part of the vdev tree, not entire tree.

All this hardware TLS assist is a crazy layer violation. But this is a wave a can't stand against.

I can't say that I am happy with this change, sorry. But looks like you need it, so let it be so.

In D51219#1169582, @mav wrote:

TXGs are committed once there is some minimal amount of dirty data reached, but no less than every 5 seconds if there were any changes. Under constant data stream TXGs are committed as fast as underlying storage can handle metadata updates and cache flushes, which may vary between some fractions of a second to multiple seconds. Many zpool and zfs commands do it forcefully. zpool sync does just that, for example, if you need up to date stats, etc.

Works as a panic prevention :)

In D51219#1169554, @mav wrote:

The labels are updated on each TXG commit, that may be many times per second. So the races are quite probable there, and unlike resurrecting old disk are completely unpredictable and also harder to recover by just removing that disk.

In D51219#1169548, @mav wrote:

I wonder what happen if system crash after updating one label on only one disk out of several. In such case vdev_label_read_config() will return config of that vdev for that TXG. But no other vdev have config for that TXG yet. So if we already probed some other vdevs, the added vdev_free(spa->spa_root_vdev); will wipe root vdev and all the children configs for the pool and restart it from scratch, populating only vdev where the currently probed disk belongs. But already probed vdevs I guess will not be re-probed, leaving pool-wide configuration incomplete. It seems not implemented here, looking on UINT64_MAX always passed to vdev_label_read_config(), but IIRC normal pool import code tries to find maximum common TXG where it has sufficient quorum. It might not be the maximum TXG, but the previous one, or theoretically allowed even one before that, if something went very wrong.

Make mktemp+bind optional only for the SOCK_STREAM case.

Thanks! Didn't know this page exists.

Note: the binding is actually needed when we run in SOCK_DGRAM mode. So this isn't a final revision.

Note: 4 more reviews stacked on top of this one, if anyone interested.

We could also use SYSCTL_PROC() instead of SYSCTL_BOOL() and then add gone_in() to emphasis that the sysctl-variable is deprecated. Just let me know if this is wanted.

We will sort out how to correctly put #error later.

In D51031#1164827, @des wrote:

I assume the original concern here was that mktemp() is subject to TOCTTOU races. Thus, it would be better to use mkdtemp() to create a unique temporary directory, and create our socket inside that directory.

I am really puzzled how D34550 was created and how did it pass your review :)

Agreed, let's try 495 and see what CI reports after a week.

No objection from me. Thanks for the change and sorry for the delay. But please wait for the stabweek closure.

Thanks!

Thanks, @imp! What if instead of allowing the header but masking its internals, we want to eliminate any use of a header by userland applications? Some headers have #error no user serviceable parts inside and AFAIU all of them are listed in badfiles.inc. Is this the way to go?

What is the problem we are trying to fix here? What is the panic trace?

I clicked on "Accept", trusting your expertise. The change looks correct, with small comment nit. But I didn't test it. And to be fair the gist of the code already washed out of my L3 brain cache :)

There is no #warning or #error in case the header file is not included in the kernel context, since then this header file would need to be added to tools/build/test-includes/badfiles.inc, but that list should not grow.

Manual page counter.9 also needs to be updated.

I totally agree with the period feature. Thanks!

bsnmpd done. We can put the entire header under _KERNEL. We also can add #error in there to catch all incorrect inclusions of this header.

• Don't forget rack/bbr.
• Document the new sysctl and improve paragraph.