Re: [PATCH 14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent IOs bypass the lockless exclusion in `dasd_ese_heuristic_tick`, causing multiple threads to execute state transitions and corrupting the heuristic's state and backoff intervals.
--

--- Patch [14]: [PATCH 14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit e9a2e43413ef73d4ffb3fe2ea3f44d65c5f79bff
Author: Stefan Haberland <[email protected]>

s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

Derives adaptive ESE fulltrack heuristic parameters from ft_bias to
locklessly switch between fulltrack and plain writes based on device
sparseness.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 067ab66209b65..62c03c4787c83 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -3231,6 +3240,69 @@ static void clear_format_track(struct dasd_format_entry *format,
>  	spin_unlock_irqrestore(&block->format_lock, flags);
>  }
>  
> +/*
> + * Adaptive ft_bias heuristic, called once per IO from dasd_eckd_build_cp().
> + * Probes the device formatting state by briefly switching to ft0 and measuring
> + * the NRF rate; parameters are derived from ft_bias.
> + */
> +static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
> +{
> +	int ios, nrf, rate;
> +
> +	if (atomic_inc_return(&basedev->ese_io_cnt) < (int)basedev->ese_probe_interval)
> +		return;
> +
> +	/*
> +	 * One wins the race to evaluate, the rest see ios == 0 after the
> +	 * xchg and return early, preventing redundant state transitions.
> +	 */
> +	ios = atomic_xchg(&basedev->ese_io_cnt, 0);
> +	if (ios <= 0)
> +		return;

[Severity: Medium]
Can this lockless exclusion logic be bypassed if a third thread increments
the counter right after the first thread clears it?

If Thread A and Thread B both pass the atomic_inc_return() threshold check
concurrently, they will both proceed to the atomic_xchg().

If Thread A executes atomic_xchg() first, it resets the counter to 0 and
proceeds to evaluate the state machine.

If Thread C then calls dasd_ese_heuristic_tick() and increments the counter
from 0 to 1, it will return early because it is below the threshold. However,
Thread B could then execute its atomic_xchg() and receive 1.

Since 1 is greater than 0, Thread B will also pass the ios <= 0 check and
evaluate the state machine concurrently with Thread A.

Could this concurrent execution corrupt the heuristic state and backoff
intervals, causing premature transitions (for example, from FT1_ACTIVE to
PROBING)?

> +
> +	switch (basedev->ese_probe_state) {
> +	case DASD_ESE_HEU_FT1_ACTIVE:
> +		/* Start ft0 probe window, reset NRF counter for clean measurement */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14