Re: [PATCH] s390/stp: Drop CLOCK_SYNC_STP

[Stefan Haberland <sth@linux.ibm.com>]

Am 09.09.26 um 14:29 schrieb Sven Schnelle:
> Stefan Haberland <sth@linux.ibm.com> writes:
> 
>> Am 14.08.26 um 09:34 schrieb sashiko-bot@kernel.org:
>>> [Severity: High]
>>> Could this change cause a boot hang on systems with XRC-capable DASDs
>>> but no active STP network?
>>>
>>> Because stp_online is initialized to true, if the machine supports the
>>> STP facility but is not connected to a usable STP time source, time
>>> synchronization won't occur and stp_online will remain true.
>>>
>>> With stp_online remaining true, get_phys_clock() will continuously fall
>>> through and return -EAGAIN, rather than safely returning -EACCES as it
>>> did prior to this patch.
>>>
>>> The DASD driver ignores -EACCES but propagates -EAGAIN:
>>>
>>> drivers/s390/block/dasd_eckd.c:set_timestamp() {
>>>     ...
>>> 	if ((rc && !private->rdc_data.facilities.XRC_supported) ||
>>> 	    rc == -EOPNOTSUPP || rc == -EACCES)
>>> 		return 0;
>>>     ...
>>> }
>>>
>>> If -EAGAIN is propagated up, will the block layer endlessly retry the I/O,
>>> resulting in a complete hang during boot?
>>>
>>
>> From what I see, Sashiko is right. This will break how the DASD driver
>> actually checks if the clock is enabled and in sync.
>>
>> The DASD driver relies on -EACCES to know that the clock exists but STP
>> sync is not online/applicable which in turn will skip it, whereas
>> -EAGAIN means the clock does exist but is not in sync yet and we should
>> wait and retry, which in turn leads to endless loop of request never
>> being built because we never get a valid timestamp.
>>
>> So I guess we will still need a third bit to differentiate those states.
> 
> If I understood the code correctly, it would only block until STP is in
> sync, so this is expected behaviour - looking at z/VM documentation (especially
> the XRC_OPTional flag), DASD I/O should be blocked until time is in
> sync.
> 
> I think the correct way would be to add a patch on top that adds the
> XRC_OPTional parameter to either block DASD I/O when STP is in unsynchronized
> state (current behaviour) or just omits the timestamp when XRC_OPTional
> is set.

Not sure XRC_OPTional fully covers the case here, though.
The existing "don't count -EAGAIN against retries" logic assumes the
clock will eventually converge, a real STP network mid-sync.

But the scenario here is different: no CTN/network attached at all, so
there's nothing to converge the state only changes via admin action, not
by waiting.

Wouldn't a mandatory-XRC device on a machine with no STP network at all
still block forever either way, regardless of XRC_OPTional? That flag
decides whether a given volume needs a timestamp, but not whether the
system can ever produce one.

Could it make sense for get_phys_clock() to distinguish "converging"
from "no usable source at all" itself independent of the XRC_OPTional patch?