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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] sysfs attribute `ese_heuristic_state` incorrectly reports 'fulltrack active' for non-ESE devices.
- [Medium] Flawed lockless synchronization in `dasd_ese_heuristic_tick` allows multiple threads to bypass the early return, resulting in spurious state transitions based on tiny probe windows.
--

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

s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

Turn the middle of the ft_bias range (1..99) into an adaptive heuristic
that switches between fulltrack write (ft1) and plain write ft0 depending
on how sparse the device still is.

A sparse device benefits from fulltrack writes (it avoids the format/retry
cycle); once enough tracks are formatted the per-write overhead of
ft1 outweighs that. An state machine  measures the NRF rate in short ft0
probe windows and flips back to ft1 when it is high
(FT1_ACTIVE -> PROBING -> FT0_STABLE, with a backing-off reprobe interval).

> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> index 035c022255b68..50301e545bbe8 100644
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c

[ ... ]

> @@ -1668,6 +1674,41 @@ static ssize_t full_track_bias_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(full_track_bias);
>  
> +static const char * const dasd_ese_heu_state_names[] = {
> +	[DASD_ESE_HEU_FT1_ACTIVE] = "fulltrack active",
> +	[DASD_ESE_HEU_PROBING]    = "probing",
> +	[DASD_ESE_HEU_FT0_STABLE] = "fulltrack inactive",
> +};
> +
> +/* read-only: current full-track mode / adaptive FSM state, for observability */
> +static ssize_t
> +ese_heuristic_state_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct dasd_device *device;
> +	unsigned int state;
> +	int len;
> +
> +	device = dasd_device_from_cdev(to_ccwdev(dev));
> +	if (IS_ERR(device))
> +		return -ENODEV;
> +	if (device->ft_bias == 0) {
> +		len = sysfs_emit(buf, "fulltrack deactivated\n");
> +	} else if (device->ft_bias >= DASD_FT_BIAS_MAX) {
> +		len = sysfs_emit(buf, "fulltrack permanent active\n");
> +	} else {
> +		state = device->ese_probe_state;
> +		if (state < ARRAY_SIZE(dasd_ese_heu_state_names))
> +			len = sysfs_emit(buf, "%s\n", dasd_ese_heu_state_names[state]);
> +		else
> +			len = sysfs_emit(buf, "unknown\n");
> +	}

[Severity: Medium]
Will this incorrectly report "fulltrack active" for non-ESE devices?

If ft_bias is set in the adaptive range (which defaults to 50), this falls
into the else block and reads ese_probe_state. For standard non-ESE
devices, dasd_ft_bias_apply() initializes this state to
DASD_ESE_HEU_FT1_ACTIVE (0). 

Should this check dasd_ese_adaptive(device) before printing the state to
avoid exposing misleading observability statistics to user space?

> +	dasd_put_device(device);
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(ese_heuristic_state);

[ ... ]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 0247e9a6d7929..dc30c4395b0cc 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -3230,6 +3239,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]
Is it possible for multiple threads to bypass the early return and cause
spurious state transitions?

If multiple threads concurrently evaluate
atomic_inc_return(&basedev->ese_io_cnt) < interval and see a value >= interval,
they both proceed. The first thread zeroes the counter via atomic_xchg.

If another I/O increments the counter to 1 before the second thread performs
its atomic_xchg, the second thread will read 1, bypass the ios <= 0 check,
and evaluate the heuristic using ios = 1.

Could this lead to the ESE heuristic state machine evaluating on tiny probe
windows, causing thrashing between FT0 and FT1 under concurrent I/O submission?

> +
> +	switch (basedev->ese_probe_state) {
> +	case DASD_ESE_HEU_FT1_ACTIVE:

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