Re: [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional enablement of `WRITE_FULL_TRACK` via the `full_track_bias` module parameter causes a severe throughput regression for non-ESE standard ECKD devices.
- [Medium] Using `rq_data_dir(req) == WRITE` incorrectly routes dataless operations to `dasd_eckd_build_cp_tpm_writefulltrack()`, violating the subsystem guide and risking a NULL pointer dereference.
- [Medium] The `fillsize` calculation in `dasd_eckd_build_cp_tpm_writefulltrack()` lacks padding headroom, causing sporadic request build failures.
- [Low] Concurrent writes to the `full_track_bias` sysfs attribute can leave `ft_bias` and `fulltrack` in an inconsistent state.
--

--- Patch [7]: [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode ---
commit ba2fbebc6514d0ac9b7370c40e5b4ab38ea6caff
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode

This commit adds a full_track_bias sysfs attribute to control the full-track
write mode for ESE devices. It sets an initial default bias from a module
parameter and routes aligned write requests to the full-track write path.

> 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);

[ ... ]

> +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 leave ft_bias and fulltrack
in an inconsistent state?

These fields are updated sequentially without lock protection. If two
threads write different values simultaneously, it might result in a
mismatched state.

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

[ ... ]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index afbc915ea4d0c..c10fe20e7d3ff 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -2118,6 +2130,10 @@ 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);
> +	device->fulltrack = device->ft_bias ? 1 : 0;

[Severity: High]
Does this unconditionally enable fulltrack writes for standard non-ESE ECKD
devices?

The fulltrack flag is enabled based on the default bias, without checking
if the device is actually ESE capable. This appears to force the
WRITE_FULL_TRACK command for standard ECKD devices, which has an overall
penalty for maximum throughput.

> +
>  	if (private->conf.gneq) {
>  		value = 1;

[ ... ]

> @@ -5165,11 +5181,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]
Does this incorrectly route dataless operations like REQ_OP_WRITE_ZEROES
to the full-track write path?

The rq_data_dir() macro checks the lowest bit of the operation, which means
it evaluates to true for dataless operations. If this routes a dataless
request to dasd_eckd_build_cp_tpm_writefulltrack(), iterating over bio
segments can result in a NULL pointer dereference because dataless
operations lack a bi_io_vec.

The block layer subsystem guidelines explicitly forbid using op_is_write()
as a guard for data fields, and require bio_has_data() instead.

[Severity: Medium]
When dasd_eckd_build_cp_tpm_writefulltrack() is called, is the fillsize
calculation large enough to account for page alignment padding?

Inside dasd_eckd_build_cp_tpm_writefulltrack(), the fillsize is calculated
exactly:

fillsize = trkcount * (sizeof(struct eckd_r0) +
                       (sizeof(struct eckd_count) * (blk_per_trk + 2)));

However, reserve_nocross() skips padding bytes when encountering a page
boundary and subtracts the padding from the remaining space:

pad = PAGE_SIZE - off;
if (*space < pad)
        return NULL; /* out of space */
*p      = (void *)((char *)*p + pad);
*space -= pad;

Can this lack of padding headroom cause the preallocated filldata buffer to
run out of space prematurely, forcing a failure and a fallback to a slower
I/O mode?

> +			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/20260723134743.672413-1-sth@linux.ibm.com?part=7