Re: [PATCH v7 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Inconsistent initialization of the `fulltrack` flag between the module parameter and sysfs store, breaking the intended default behavior.
- [Low] Lockless concurrent updates to `ft_bias` and `fulltrack` by multiple writers cause persistent state inconsistency.
--

--- Patch [13]: [PATCH v7 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode ---
commit 377e4de782323378dcdb6afeafb0086a91d9e269
Author: Stefan Haberland <[email protected]>

s390/dasd: Add full_track_bias to control fulltrack write mode

This patch adds a sysfs attribute full_track_bias to control full-track
writes on ESE devices. It also adds a module parameter to set the default
bias applied to devices at online time.

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

[ ... ]

> +static ssize_t full_track_bias_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct dasd_device *device;
> +	unsigned int val;
> +
> +	if (kstrtouint(buf, 0, &val) || val > DASD_FT_BIAS_MAX)
> +		return -EINVAL;
> +
> +	device = dasd_device_from_cdev(to_ccwdev(dev));
> +	if (IS_ERR(device))
> +		return -ENODEV;
> +
> +	device->ft_bias = val;
> +	device->fulltrack = val ? 1 : 0;

[Severity: Low]
Can concurrent writes to this sysfs attribute cause a persistent state
inconsistency? If two threads write different values (like 100 and 0) at the
same time, could the lockless updates to device->ft_bias and device->fulltrack
interleave, leaving ft_bias = 0 but fulltrack = 1?

> +
> +	dasd_put_device(device);
> +	return count;
> +}

[ ... ]

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

[ ... ]

> @@ -2148,6 +2160,11 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
>  	device->path_interval = DASD_ECKD_PATH_INTERVAL;
>  	device->aq_timeouts = DASD_RETRIES_MAX;
>  
> +	/* 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;

[Severity: Medium]
Does this initialization miss the intended default behavior described in the
commit message?

With the default ft_bias of 50, the ternary operator evaluates to 0, which
disables the feature by default during device initialization. However, in
full_track_bias_store() in dasd_devmap.c, the code uses:

    device->fulltrack = val ? 1 : 0;

meaning if an administrator manually writes the exact same default value of 50
to sysfs, the flag flips to 1. Should this initialization match the sysfs store
logic to enable full-track writes out-of-the-box for any non-zero value?

> +
>  	if (private->conf.gneq) {
>  		value = 1;
>  		for (i = 0; i < private->conf.gneq->timeout.value; i++)

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