Re: [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] An infinite loop occurs when a WRITE_FULL_TRACK CQR encounters an INV_TRACK_FORMAT error because the ERP request fails to inherit the `filldata` pointer, bypassing the intended permanent-failure path.
- [High] dasd_eckd_build_cp_tpm_writefulltrack() iterates over bio segments without verifying if the request has data buffers, risking a NULL pointer dereference for data-less requests.
- [Medium] Non-transient errors (like -EINVAL) from `dasd_eckd_build_cp_tpm_writefulltrack` cause wasteful retry loops instead of failing fast.
- [Medium] Double counting of I/O statistics for aborted ESE format replacement requests.
--

--- Patch [10]: [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit 9457d60af3530b5a71a57c6a68b8e701a0e817f4
Author: Stefan Haberland <[email protected]>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

This commit modifies the ESE unformatted track handler to use
WRITE_FULL_TRACK. It changes dasd_eckd_ese_format() to return void and
stage the CQR directly, while marking the origin CQR as aborted.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 35f722e20c6d3..19a04155d271c 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1737,26 +1737,23 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
>  			dasd_schedule_device_bh(device);
>  			return;
>  		}
> -		fcqr = device->discipline->ese_format(device, cqr, irb);
> -		if (IS_ERR(fcqr)) {
> -			if (PTR_ERR(fcqr) == -EINVAL) {
> -				cqr->status = DASD_CQR_ERROR;
> -				return;
> -			}
> +		if (cqr->filldata) {
>  			/*
> -			 * If we can't format now, let the request go
> -			 * one extra round. Maybe we can format later.
> +			 * A WRITE_FULL_TRACK cqr carries the complete
> +			 * track image; INV_TRACK_FORMAT here means the
> +			 * generated image or the media itself is bad, not
> +			 * that the track still needs formatting - retrying
> +			 * via ese_format() would just resubmit the same
> +			 * write. Let it fail instead.
>  			 */
> -			cqr->status = DASD_CQR_QUEUED;
> -			dasd_schedule_device_bh(device);
> -			return;
> -		} else {
> -			fcqr->status = DASD_CQR_QUEUED;
> -			cqr->status = DASD_CQR_QUEUED;
> -			list_add(&fcqr->devlist, &device->ccw_queue);
> +			cqr->status = DASD_CQR_ERROR;
> +			cqr->stopclk = now;
> +			dasd_device_clear_timer(device);
>  			dasd_schedule_device_bh(device);
>  			return;
>  		}
> +		device->discipline->ese_format(device, cqr, irb);
> +		return;
>  	}

[Severity: High]
Could this cause an infinite loop during Error Recovery (ERP)?

When a WRITE_FULL_TRACK request fails with INV_TRACK_FORMAT, the ERP
engine creates a retry request but doesn't seem to copy the filldata
pointer from the base request. If this ERP request fails again with
INV_TRACK_FORMAT, will the interrupt handler see a NULL filldata here
and incorrectly call ese_format() again? This would generate a new base
request instead of failing, potentially looping infinitely.

[ ... ]
> @@ -2752,6 +2756,9 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
>  			error = BLK_STS_IOERR;
>  			break;
>  		}
> +	} else if (status == DASD_CQR_ABORTED) {
> +		/* aborted requests are replaced with a new one so do not complete this */
> +		return;
>  	}
>  
>  	/*

[Severity: Medium]
Could this result in double-counting I/O statistics?

dasd_profile_end() is called unconditionally at the beginning of
__dasd_cleanup_cqr():

__dasd_cleanup_cqr()
{
	...
	dasd_profile_end(cqr->block, cqr, req);
	...
	if (status == DASD_CQR_ABORTED) {
		return;
	}
}

When the replacement format request eventually completes, won't it account
for the same block request again?

[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8d8bd40ba896b..4fe1b78366b04 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> +	fcqr = dasd_eckd_build_cp_tpm_writefulltrack(startdev, block, req,
> +						     first_rec, last_rec,
> +						     first_trk, last_trk,
> +						     first_offs, last_offs,
> +						     recs_per_trk, blksize, cqr);

[Severity: High]
Can this cause a NULL pointer dereference for data-less requests?

If a REQ_OP_DISCARD or REQ_OP_WRITE_ZEROES operation fails with an ESE
format error, it seems ese_format() will unconditionally pass it to
dasd_eckd_build_cp_tpm_writefulltrack().

Since dasd_eckd_build_cp_tpm_writefulltrack() uses rq_for_each_segment()
which accesses bio data fields, and data-less requests have a NULL
bi_io_vec, does this need a bio_has_data() check before iterating?

>  	if (IS_ERR(fcqr)) {
>  		if (format)
> -			clear_format_track(format, block);
> -		return fcqr;
> +			clear_format_track(format, cqr->block);
> +		/*
> +		 * Transient conditions - the XRC clock is not in sync (-EAGAIN)
> +		 * or the format request pool is momentarily exhausted under load
> +		 * (-ENOMEM). Retry the origin without counting down its retries.
> +		 */
> +		if (PTR_ERR(fcqr) == -EAGAIN || PTR_ERR(fcqr) == -ENOMEM)
> +			cqr->retries++;
> +		goto out_retry;
>  	}

[Severity: Medium]
Does this cause non-transient errors like -EINVAL to be retried
unnecessarily?

By falling through to out_retry without failing immediately, it appears
permanent build errors might loop until the original request's retry
counter is exhausted. The original code explicitly checked for -EINVAL
and returned it to fail fast.

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