Re: [PATCH v6 12/18] s390/dasd: Add full_track_bias to control fulltrack write mode

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `full_track_bias` parameter is unconditionally applied to all ECKD devices, inflicting performance penalties on standard non-ESE volumes.
- [Medium] Inconsistent handling of `full_track_bias` values 1-99 between device initialization and the sysfs store function.
- [Medium] Race condition in sysfs store method allows a torn update to permanently desync the `ft_bias` and `fulltrack` state.
--

--- Patch [12]: [PATCH v6 12/18] s390/dasd: Add full_track_bias to control fulltrack write mode ---
commit cca10efd2af8b0fe1b751764fcdbf7d7f54c4ef1
Author: Stefan Haberland <[email protected]>

s390/dasd: Add full_track_bias to control fulltrack write mode

Add a per-device full_track_bias sysfs attribute to gate the full-track
write path for ESE devices. A module parameter sets the initial default value
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
> @@ -1630,6 +1630,44 @@ dasd_expires_store(struct device *dev, struct device_attribute *attr,
>  

[ ... ]

> +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: Medium]
Is it possible for concurrent sysfs writes to leave these variables out of
sync?

Because the sysfs core does not serialize concurrent writes to the same file
if opened via separate file descriptors, could an interleaved execution from
two threads writing 100 and 0 concurrently result in a torn update where
ft_bias is 0 but fulltrack remains 1?

[Severity: Medium]
Does this sysfs store function contradict the initialization logic for values
between 1 and 99?

During device initialization, dasd_eckd_check_characteristics() restricts
device->fulltrack to 0 for a value like 50 (the default). However, if the
exact same value (50) is written to this sysfs attribute, this line
unconditionally sets device->fulltrack to 1.

Could this cause a sudden unprompted change in I/O behavior upon rewriting
the current configuration value?

> +
> +	dasd_put_device(device);
> +	return count;
> +}
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 5e7a531707a0c..9003e28a4ebbe 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: High]
Will this module parameter be applied to all ECKD devices, even standard
non-ESE volumes?

The commit message states the bias should be applied to every ESE volume at
online time. However, this appears to unconditionally copy the module
parameter into every ECKD device's state regardless of whether the volume
is actually ESE, without checking dasd_eckd_is_ese(device).

> @@ -5199,11 +5216,20 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
>  		/* do nothing, just fall through to the cmd mode single case */
>  	} else if ((data_size <= private->fcx_max_data)
>  		   && (fcx_multitrack || (first_trk == last_trk))) {
> -		cqr = dasd_eckd_build_cp_tpm_track(startdev, block, req,
> -						    first_rec, last_rec,
> -						    first_trk, last_trk,
> -						    first_offs, last_offs,
> -						    blk_per_trk, blksize);
> +		if (!first_offs && (last_offs + 1 == blk_per_trk) &&
> +		    rq_data_dir(req) == WRITE && basedev->fulltrack) {
> +			cqr = dasd_eckd_build_cp_tpm_writefulltrack(startdev, block, req,
> +								    first_rec, last_rec,
> +								    first_trk, last_trk,
> +								    first_offs, last_offs,
> +								    blk_per_trk, blksize, NULL);

[Severity: High]
If basedev->fulltrack is set for a non-ESE device, will this downgrade
standard track-based I/O to WRITE_FULL_TRACK?

If an administrator sets dasd_eckd_mod.full_track_bias=100 to optimize
their Extent Space Efficient (ESE) volumes, it looks like all standard
(non-ESE) volumes will indiscriminately inherit this setting and suffer a
severe maximum throughput degradation penalty.

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