I have no objections in general, if you find it useful, just a few on the implementation.

I also wonder if we should make the same tunable to affect other forms of sleep, or not. Is this the most pathological in implementations? May be so. considering there were no complains about others.

The additional flags are normally used only for queued requests. So some of places here are likely not affected. But it should not harm probably any way.

In D39017#894567, @asomers wrote:

Ok, I'll commit for now. However, this vendor is sometimes responsive. They might be willing to choose a better error type if I ask. Do you have any particular SCSI sense code in mind?

In D39141#894563, @asomers wrote:

Ok. How about we merge this PR now, and I'll have a new one ready for you to test when you get back from Asia?

In D39017#894555, @asomers wrote:

@mav given my comments above, are you ok with merging this? Do you have any better ideas for how to fix the problem?

In D39141#894560, @asomers wrote:

@mav do you have any of those dual-actuator HDDs? If so, would you be willing to test a patch that adds their LUN numbers to the physical path string? I can send that separately.

The original physical path format came from Solaris/Illumos. While I agree that it is ugly and not very documented, I am not a big fan of incompatible changes. I'd think twice.

ses_process_control_request() triggered by ses_set_enc_status() already calls ses_poll_status(), so most of what you are doing should already work. Not sure only about ENCIOC_SETSTRING.

It may be OK, but I am not sure I like this approach. It creates additional kernel requests, it is potentially racy. Doesn't the enclosure return some reasonable status that kernel could convert into EACCESS or something more reasonable than EINVAL? At least it would be good to do both.

I have no objections, just some comments to polish.

Makes sense on a brief look.

From perspective of ZFS I am all for the removal of mmap() use. ZFS does not benefit from it, but only suffers badly from created buffer cache pressure. We have it disabled on TrueNAS. I was tempted to remove it, but didn't want to open large discussion. I guess it may be beneficial for UFS and any FS using buffer cache, avoiding one memory copy. But I think it should be handled by copy_file_range() inside the kernel in a way the most efficient for specific file system(s).

In D38130#866505, @asomers wrote:

And I see that you can print those with the -p option. But there's not currently any way to get all of them, is there?

I suppose that request would give you all traffic of the LUNs accessible through that target, even one going though different targets. As alternative, CTL collects per-port statistics, that would closer match targets, if desired, but then without separation between different LUNs.

In D38130#866215, @asomers wrote:

Prometheus is expecting that each LUN has one associated target. If one LUN can be shared by multiple targets, then that's certainly a problem. Could you please share an example of such a configuration?

I've never before looked on this code, but all this targdata.targets[lun] logic looks wrong to me. Any LUN may be mapped into multiple different ports, think that this code does not allow. LUNs not mapped into any port is only a one minor case of generally broken design. I have no idea what data this Prometheus is expecting to receive, but right now it obviously does not receive the full picture. This particular patch is not bad, but it just does not fix whole problem, only a minor part.

In D37594#856094, @hselasky wrote:

3. enter kgdb and dump the nexthard value and check if it is divisible by tick_sbt as it should.

For SMP the value that holds the next hardclock event is per-CPU, and what prevents those per-CPU values from sliding between CPUs? During boot, the dummy timer will put "random" starting values in there? Or am I wrong?

In D37594#855759, @hselasky wrote:

@mav : What about synchronous execution between CPUs. When you have one CPU it doesn't matter that much, but when you have multiple CPUs, it might make sense to execute the timers aligned to eachother, instead of sliding randomly?

In D37321#855300, @glebius wrote:

For this change I'm going to take a conservative approach and just store the precision returned by callout_when(). Then I will discuss with TCP guys what we actually want. On our last discussion we touched that briefly and they noticed that our TCP timing is not as precise as it is on Linux and this (could be) a problem (sometimes). IMHO, most likely we want to have different precision for different timers. Anyway, this to happen in a different changeset.

I've never used libxo to closely review this, but I find it amusing that kernel produces XML for libgeom to parse for geom tool to be able to produce another XML (or JSON or whatever). :)

In D37321#855280, @glebius wrote:

Is this returned precision going to be the same for any ticks argument or not? For example I call callout_when for TT_DELACK and store the result, but then I see that TT_PERSIST is going to be run earlier. Can I use precision returned by callout_when() for TT_DELACK to schedule TT_PERSIST?

In D37321#855260, @glebius wrote:

In D37321#855218, @mav wrote:

Even old callout_reset_on() is not that strict by using system-wide precision of 5%.

I don't see that. In sys/callout.h:

In D37594#855242, @hselasky wrote:

Yes, I removed that, but the "sbt" (timeout) itself is not guaranteed to be aligned - right ?

I was thinking about removing 64bit divisions in the hot path and moving them into the first initialization, instead you've added more... We should not bother about alignment in callout_when() if hardclocktime is aligned. Time passed by caller must be aligned to tick_sbt already by him, otherwise he should not use C_HARDCLOCK.

In D37594#855163, @hselasky wrote:

Maybe when setting/updating tick_sbt, there should be a callback to re-align the hardclocktime? What do you think?

In D37321#855118, @glebius wrote:

Do I understand it correct that the "precision" argument to callout_reset_sbt_on() becomes pretty useless if we specify C_HARDLOCK?

Why do you think this is needed? All C_HARDCLOCK consumers are supposed to pass only periods multiple of tick_sbt, and hardclocktime above should be aligned to hardclock also. In what cases do you see misaligned values?

Looks good to me, but I am not sure it makes sense to still have the paragraph about 11.0. 11 branch is out of support now, plus that -T option of geli seems to disable TRIM, not enable it, that would have some more sense to document.

In D37309#847746, @asomers wrote:

Maybe something like "geom -c <consumer-name>" to complement the existing "geom -p <provider-name>"?

Not that I have specific objections against this, but I think it would be much better to implement a generic way to print all consumers connected to specified GEOM provider. This code does not look anyhow specific to gmultipath.

On TrueNAS we do not upgrade pools automatically to allow users to make sure system operates properly to keep possibility of downgrade if anything go wrong. But if this works only on first boot after install it should probably be fine.

I agree with Konstatin's comments, otherwise looks good to me. Thinking about the HZ_MAXIMUM value I wonder where is " >> 6" coming from? Is it useful to allow hz > 100KHz? But then above ~15KHz " >> 6" will loose precision. If HZ_MAXIMUM is set to ~68K, then " >> 5" could be better here.

I always wondered whether kernel could try harder to reduce the number of segments below the number of pages to help hardware that can benefit from it. But you are right that current kernel does not even try to allocate the bounce pages consequently, so allocating more pages than segments supported does not make much sense. Though many drivers should already request maximum map size as maximum number of segments multiplied by page size. So this should affect only drivers that are not doing it.

I think this may create a race between new command completion added into the queue and already running handler, when interrupt arriving last moment get dropped, but newly completed command is not yet processed.

Since it is under the queue lock I don't see much difference in the order, only removal of the condition makes sense to me.

In D36843#836270, @hselasky wrote:

If periodic == 0 and first == 0, which I see can happen, then fdiv is set to t->div which is zero. Do you see?

The original idea was to keep the potentially expensive clock reading operation out of the lock scope, which in case of global timer, like HPET (which I guess you are using according to other review), may be quite congested. You are right that reading time counter closer to event timer programming should reduce the jitter. I just have difficulties to believe that spin on the lock may be long enough to be possible to hear (thinking whether there can be a context switch somewhere?). But if you say so, I have no big objections. I don't know any modern system with global event timer or slow enough time counter any more, so don't care so much about this aspect.

et_start() should guarantee that this code receive first and/or period big enough for meaningful fdiv. Do you expect this code to loop enough times for overflow the fdiv turning it zero?

Looks good to me, though I am not really qualified to choose. I just saw CUBIC recovering much faster after packet losses in datacenter environments without flow control and WAN/WiFi. With NewReno we saw regular pauses in a traffic up to a second IIRC.