iscsi: send only 3 iscsi_pings in the timeout interval

[rscheff]

Summary

Previously, iscsi would have a fixed once per second
callout, to either time out the iscsid / login phases,
or to transmit a new iSCSI ping.

In order to reduce the number of callouts while waiting
for iscsid/login, only schedule one callout for when that
has expired.

Schedule the callouts while connected at 1/3 of the ping
timeout interval. This will send Pings only 3 times before
expiring - and at larger intervals, reducing overhead,
especially when larger timeouts are configured.

The expiry of an unresponsive session still happens
as before (after the timeout) but with less burden on the
system.

[jhb]

(I think this diff contains the prior change as well?) I would also suggest mstosbt() here as well.

[rscheff]

• use mstosbt()

[rscheff]

In D34230#775828, @jhb wrote:

(I think this diff contains the prior change as well?) I would also suggest mstosbt() here as well.

Yes, this is a diff against D34222, which would have to be applied first. I believe this is slightly different behavior since the change to git, where arcanist/phabricator will show the full diff here against main, while a patch created would only give the delta...

But only the intended delta is marked red/green.

[jhb]

As a general point, I wonder if it really makes sense to overload 3 separate events into a single callout vs having separate functions for the different callouts. I think while you are waiting for iscsid you don't want to trigger pings anyway, and this would avoid needing is_timeout at all for the non-ping timeouts (which is now a bit more confusing as it is no longer a count of seconds, but a count of timeouts). There's also no reason to even schedule the timeout when waiting for iscsid I think if iscsid_timeout is zero?

[rscheff]

I didn't want to modify the iscsi_callout() function completely. I guess the current design was picked to keep the housekeeping overhead low. Splitting iscsi_callout into a iscsid timeout, login timeout and ping hello function is certainly possible.

However, I propose to do

1. per-session timeouts (D34198) and allow these to be
2. configured at milliseconds granularity (D32540 once re-worked) first - leaving all the sysctl global settings unchanged. Anyone wanting sub-second timeouts has to configure this on a per-session basis.
3. This Diff around changing the core behavior (three pings over "timeout" interval instead one ping every second when no other IO was performed) would follow last.
4. I would want to so some socket sendbuffer probing also, between pings - for a faster resumption after a loss of connectivity

[mav]

sys/dev/iscsi/iscsi.c

592

is_timeout is unsigned, so checking it for "> 0" just after incrementing it looks like a waste. Same below.

1431

This should not be needed also.

1435

I guess the only reason to schedule timeout if login_timeout == 0 is if it will be changed live? The oddity is that following callouts will be rescheduled with ping_timeout / 3 period. May be iscsi_callout() splitting as John proposed could be cleaner.

1978

Not directly related to this commit, but I wonder what's the point to schedule and reschedule the callout if (is->is_conf.isc_enable == 0 && is->is_conf.isc_discovery == 0) below. Though I've never disabled sessions myself...

[rscheff]

• use the correct timeout value in log output