Re: [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Inconsistent initialization of the `fulltrack` flag disables full-track writes by default, contradicting the intended behavior and sysfs store logic.
- [Medium] Full-track writes are applied unconditionally to non-ESE devices, exposing them to unintended performance penalties.
--

--- Patch [11]: [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode ---
commit c5f39fbb283ed6068ac10c59d09a116557c8bbf1
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add full_track_bias to control fulltrack write mode

This patch adds a per-device full_track_bias sysfs attribute to control
full-track write routing for s390 DASD devices. It introduces a module
parameter to set the default bias applied at online time. The default
value is intended to enable full-track writes out of the box to avoid
ESE format penalties.

> 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 DEVICE_ATTR(expires, 0644, dasd_expires_show, dasd_expires_store);
>  
> +/* ESE fulltrack write aggressiveness knob (0..100, see DASD_FT_BIAS_*) */
> +static ssize_t
> +full_track_bias_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{

[ ... ]

> +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;
> +
> +	dasd_put_device(device);
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(full_track_bias);
> +
>  static ssize_t
>  dasd_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {

[ ... ]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 4fe1b78366b04..cc5a4791cb8f4 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]
Is this initialization of device->fulltrack contradicting the logic in
full_track_bias_store()?

In full_track_bias_store() above, device->fulltrack is set to 1 for any
non-zero value (device->fulltrack = val ? 1 : 0). However, here it is only
set to 1 if device->ft_bias >= DASD_FT_BIAS_MAX.

Since the default full_track_bias is DASD_FT_BIAS_DEFAULT (which is 50), this
yields 0 during initialization. This appears to contradict the commit message
which states: "The default (DASD_FT_BIAS_DEFAULT) enables full-track writes so
the ESE format penalty is avoided out of the box."

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

[ ... ]

> @@ -5194,11 +5211,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) {

[Severity: Medium]
Should there be a check here to ensure the device is an ESE volume before
routing the IO to the full-track write path?

The commit message mentions that "The WRITE_FULL_TRACK command has an
advantage on sparse formatted ESE devices but it has an overall penalty for
maximum throughput compared to usual track based IO."

However, neither the initialization in dasd_eckd_check_characteristics() nor
the sysfs store function guards the bias assignment with an ESE check (e.g.,
dasd_eckd_is_ese(basedev)). 

If basedev->fulltrack is enabled on a non-ESE device either via sysfs or by
fixing the default initialization, could it suffer the performance penalty
without the intended benefits?

> +			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);
> +		} else {
> +			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);
> +		}

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