Re: [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The adaptive heuristic parameters are never initialized during device startup because `dasd_ft_bias_apply()` is called before the device's ESE capability is known, causing the heuristic to evaluate on every single I/O.
- [High] The NRF rate calculation can overflow a 32-bit signed integer, causing the heuristic to improperly remain in plain write mode during heavy unformatted I/O.
- [High] The lockless heuristic evaluation and sysfs updates fail to serialize state machine transitions, leading to state divergence where the device permanently sticks in fulltrack mode.
--

--- Patch [12]: [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit fc455715bb1f927b75206de59511e6f8a3abe72f
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

This patch turns the middle of the ft_bias range into an adaptive heuristic
that switches between fulltrack write and plain write depending on device
sparseness. It measures the NRF rate in short probe windows to determine
when to flip back to fulltrack write mode.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index cc5a4791cb8f4..1df861f80c268 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2160,6 +2160,7 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
>  
>  	/* default ESE fulltrack write aggressiveness from the module parameter */
>  	device->ft_bias = min_t(unsigned int, full_track_bias, DASD_FT_BIAS_MAX);
> -	/* only the "always" endpoint forces fulltrack unconditionally here */
> -	device->fulltrack = (device->ft_bias >= DASD_FT_BIAS_MAX) ? 1 : 0;
> +	dasd_ft_bias_apply(device);

[Severity: High]
Does calling dasd_ft_bias_apply() here happen before the device's ESE
capability is known?

In dasd_eckd_check_characteristics(), dasd_ft_bias_apply() is called early.
This function relies on dasd_ese_adaptive(), which checks is_ese(). However,
is_ese() depends on private->vsq, which is populated later when
dasd_eckd_read_vol_info() is called. 

Because vsq is empty at this point, dasd_ese_adaptive() returns false, and
the heuristic parameters (like ese_heu_start_interval and
ese_heu_probe_window) are left initialized to 0. Will this cause the
heuristic interval to pass immediately and the FSM to evaluate on every
single I/O?

[ ... ]

> @@ -3235,6 +3241,65 @@ static void clear_format_track(...)
> +static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
> +{
[ ... ]
> +	case DASD_ESE_HEU_PROBING:
> +	case DASD_ESE_HEU_FT0_STABLE:
> +		nrf  = atomic_xchg(&basedev->ese_nrf_window, 0);
> +		rate = nrf * 1000 / ios;
> +		if (rate > (int)basedev->ese_heu_nrf_high) {

[Severity: High]
Can this rate calculation overflow a 32-bit signed integer?

During the FT0_STABLE state, the ios interval backs off up to max_interval
(500,000 IOs). If a workload performs large sequential writes into
unformatted space, an I/O can span multiple tracks, triggering multiple NRF
events. 

If the number of formatted tracks reaches ~2,147,483 within the 500,000 IO
window, nrf * 1000 will exceed INT_MAX and overflow to a negative value.
Could this cause the check to evaluate to false and improperly keep the
device stuck in plain write mode during heavy unformatted I/O?

> +			/* NRF rate high: device still sparse, ft1 is better */
> +			basedev->fulltrack          = 1;
> +			basedev->ese_probe_state    = DASD_ESE_HEU_FT1_ACTIVE;
> +			basedev->ese_probe_interval = basedev->ese_heu_start_interval;
> +		} else if (basedev->ese_probe_state == DASD_ESE_HEU_PROBING) {
> +			/*
> +			 * NRF rate low: device mostly formatted, ft0 is faster.
> +			 * Re-probe frequently at first, then back off below.
> +			 */
> +			basedev->ese_probe_state    = DASD_ESE_HEU_FT0_STABLE;
> +			basedev->ese_probe_interval = basedev->ese_heu_probe_window;

[Severity: High]
Can this lockless state transition race with a sysfs update and permanently
stick the device in fulltrack mode?

If a thread is transitioning from PROBING to FT0_STABLE (evaluating
basedev->ese_probe_state == DASD_ESE_HEU_PROBING as true) but is preempted
before writing the state, a concurrent sysfs update via
full_track_bias_store() -> dasd_ft_bias_apply() could reset the FSM:

drivers/s390/block/dasd_int.h:dasd_ft_bias_apply() {
    ...
    device->ese_probe_state    = DASD_ESE_HEU_FT1_ACTIVE;
    device->fulltrack          = 1;
    ...
}

When the preempted thread resumes, it would set ese_probe_state to
DASD_ESE_HEU_FT0_STABLE but leave fulltrack as 1. Since fulltrack = 1
suppresses NRF events, the measured rate stays at 0 and the FSM interval
backs off without ever resetting fulltrack to 0, completely defeating the
adaptive heuristic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=12