In D19140#411495, @rozhuk.im-gmail.com wrote:

Not sure that this works.
I try this on 11.2 and on Hyper-V gen1 system stick at ldelf loading stage on boot from zfs root.

Introduced local char * buffer pointer to avoid casts on output buffer pointer updates

In D19140#409631, @tsoome wrote:

In D19140#409630, @pkelsey wrote:

In D19140#409629, @tsoome wrote:

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

If the zfs on-disk format keeps things sector-aligned / sector-multiple, then why was vdev_read complicated with code to handle non-sector aligned / non-sector multiple reads to begin with?

Yea, you are right:)

In D19140#409629, @tsoome wrote:

The reason why I am asking is, I am trying to understand if we actually *can* get into the situation with overrun, I got the impression the zfs on disk format should keep things sector aligned, but it really is easy to get confused there... And it does feel safer if we do have proper checks in place.

In D19140#409448, @tsoome wrote:

I have two questions:

1. how was it tested - was there some corruption case?

In D19124#409229, @kristof wrote:

I have no objections to the patch, but I don't know enough about HFSC to meaningfully review this, I'm afraid.

Added ifname parameter to getifflags(), which now uses that instead of the global name var.

In D18760#400670, @shurd wrote:

Looks good, only a tiny nitpick on the fail label.

In D18759#402279, @kristof wrote:

Also, I’d really, really like some basic tests for altq. It should be reasonably straightforward to do something based on the existing pf tests. At least, if altq can work on top of epairs.

That’s not a requirement to commit this patch, of course, but it’d be nice to be confident we don’t accidentally break altq.

In D18761#400698, @shurd wrote:

My only concern here is making IFLIB_MAX_RX_SEGS available to the driver...

IFLIB_MAX_RX_SEGS is more as an upper limit on what a sane driver would want rather than an upper limit of what a driver can use. If the driver actually wants (and can/will use) more, this may be the time to move ifr_frags to the end of struct iflib_rxq and use a dynamic size so the driver can have as many as it actually wants instead... that would allow other drivers to use a smaller rxq struct. ixgbe for example appears to be limited to 8 via an MPASS(), and it looks like no e1000 devices support larger than 16k frame sizes either. Dropping this to 8 for most drivers and having it at 32 or 64 for ones that support hardware 64k frames or LRO/GRO then setting this driver to use whatever is best for it could be worth the effort.

To clarify 'the symptom is that a kernel with CBQ support and not CODEL fails to load a QoS policy with the obscure error "pfctl: DIOCADDALTQ: Cannot allocate memory."', to experience this failure it is also required that the queue configuration include the BORROW flag ('borrow' keyword in pf.conf), as that sets the bit that is misinterpreted by rmclass as requesting CODEL.

Looks good to me.

In D16852#358874, @jmallett wrote:

This seems straightforward, reasonable, and sufficient. Longer-term, I'd wonder about some sort of arithmetic approach here rather than this kind of hand-scaling?

Incorporated all reviewer comments up to this point.

In D16782#357098, @kristof wrote:

It'd be really awesome if you could also see about writing some basic tests (both as pfctl parser tests, to check we can now set higher bitrates for shaping, and as a basic test that shaping actually happens), but that's certainly not blocking.

Going to commit this soon as the review has been open for nearly one month and has been idle for two weeks.

Addressed reviewer comments.

Going to commit this soon as there has not been any further review comments in nearly three weeks.

Looks good. Thanks.