Re: [PATCH v6 09/18] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added DASD_ECKD_CCW_WRITE_FULL_TRACK case correctly initializes fields but they are unconditionally overwritten by trailing common code in prepare_itcw().
- [High] Uninitialized `start_trk` and `end_trk` in non-format requests cause false-positive collision detection when track 0 is formatted.
--

--- Patch [9]: [PATCH v6 09/18] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK ---
commit c073fedc91e7807d5ec588cea865d8b63661d671
Author: Stefan Haberland <[email protected]>

s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

This commit extends prepare_itcw() to build the FCX prefix block (PFX +
LRE) for the new DASD_ECKD_CCW_WRITE_FULL_TRACK track-mode I/O command. It
allocates the prefix buffer on the stack to prevent memory allocation
failures under pressure during the writeback path.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 77ceba151560b..01c6cdc8430fe 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -4383,11 +4383,13 @@ static int prepare_itcw(struct itcw *itcw,

[ ... ]

>  	switch (cmd) {
> @@ -4446,7 +4453,38 @@ static int prepare_itcw(struct itcw *itcw,
>  		 * data as well.
>  		 */
>  		if (dedata->ga_extended & 0x08 && dedata->ga_extended & 0x02)
> -			pfxdata.validity.time_stamp = 1; /* 'Time Stamp Valid' */
> +			pfxdata->validity.time_stamp = 1; /* 'Time Stamp Valid' */
> +		pfx_cmd = DASD_ECKD_CCW_PFX;
> +		break;
> +	case DASD_ECKD_CCW_WRITE_FULL_TRACK:
> +		dedata->mask.perm = 0x3;
> +		dedata->mask.auth = 0x00;
> +		dedata->attributes.operation = basepriv->attrib.operation;
> +		dedata->blk_size = blksize;
> +		dedata->ga_extended |= 0x42;
> +		rc = set_timestamp(NULL, dedata, basedev);
> +		lredata->operation.orientation = 0x0;
> +		lredata->operation.operation = 0x3F;
> +		lredata->extended_operation = 0x11;
> +		lredata->auxiliary.check_bytes = 0x2;
> +		lredata->extended_parameter_length = 0x02;
> +		if (count > 8) {
> +			lredata->extended_parameter[0] = 0xFF;
> +			lredata->extended_parameter[1] = 0xFF;
> +			lredata->extended_parameter[1] <<= (16 - count);
> +		} else {
> +			lredata->extended_parameter[0] = 0xFF;
> +			lredata->extended_parameter[0] <<= (8 - count);
> +			lredata->extended_parameter[1] = 0x00;
> +		}
> +		sector = 0xFF;
> +		/*
> +		 * If XRC is supported the System Time Stamp is set. The
> +		 * validity of the time stamp must be reflected in the prefix
> +		 * data as well.
> +		 */
> +		if (dedata->ga_extended & 0x08 && dedata->ga_extended & 0x02)
> +			pfxdata->validity.time_stamp = 1; /* 'Time Stamp Valid' */
>  		pfx_cmd = DASD_ECKD_CCW_PFX;
>  		break;
>  	case DASD_ECKD_CCW_READ_COUNT_MT:

[Severity: High]
Does the trailing code in prepare_itcw() overwrite these initialized fields
for the new full-track write command?

In the trailing code further down in prepare_itcw():

	if (cmd == DASD_ECKD_CCW_READ_COUNT_MT) {
		lredata->auxiliary.length_valid = 0;
		lredata->auxiliary.length_scope = 0;
		lredata->sector = 0xff;
	} else {
		lredata->auxiliary.length_valid = 1;
		lredata->auxiliary.length_scope = 1;
		lredata->sector = sector;
	}

If rec_on_trk is non-zero, it will recalculate sector and overwrite the 0xFF
initialized above, and it will unconditionally set length_valid = 1 which is
incorrect for WRITE_FULL_TRACK. Should the exception condition be updated to
also exclude the new full-track write command?

[ ... ]

Additional note regarding track-based collision detection introduced in this
series:

[Severity: High]
Can uninitialized start_trk and end_trk in non-format requests cause
false-positive collision detection when track 0 is formatted?

Standard read/write requests are allocated via dasd_smalloc_request(), which
zeroes the request struct, but start_trk and end_trk are never initialized for
normal I/O requests (leaving them at 0).

When a format request completes on track 0 (format->start_trk == 0),
dasd_return_cqr_cb() evaluates dasd_req_conflict() against all in-flight
requests:

drivers/s390/block/dasd_int.h:dasd_req_conflict() {
	...
	return !(cqr1->format->end_trk < cqr2->start_trk ||
		 cqr2->end_trk < cqr1->format->start_trk);
}

For uninitialized standard I/O requests, the overlap condition
!(0 < 0 || 0 < 0) evaluates to true, erroneously flagging all in-flight
standard I/O requests with collision = true. 

If those requests later receive an NRF error and require formatting, their
collision flag incorrectly tells test_and_set_format_track() they collided,
causing an immediate I/O retry without actually formatting the track. Does
this result in infinite retry loops and I/O failures on track 0?

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