Re: [PATCH v7 08/19] s390/dasd: Add infrastructure for ESE full-track write

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Block Layer Double Completion due to incorrect handling of aborted origin requests
--

--- Patch [8]: [PATCH v7 08/19] s390/dasd: Add infrastructure for ESE full-track write ---
commit f91cc6aad616495ff0e65f3a49886e5137daade3
Author: Stefan Haberland <[email protected]>

s390/dasd: Add infrastructure for ESE full-track write

This commit adds the driver internals to build WRITE_FULL_TRACK FCX channel
programs in response to unformatted tracks on ESE devices. It introduces
new CQR states (DASD_CQR_ABORT/ABORTED) intended to retire the origin CQR
of a replaced write without completing it to the block layer.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -1884,6 +1924,17 @@ static void __dasd_process_cqr(struct dasd_device *device,
>  	case DASD_CQR_CLEARED:
>  		cqr->status = DASD_CQR_TERMINATED;
>  		break;
> +	case DASD_CQR_ABORT:
> +		cqr->status = DASD_CQR_ABORTED;
> +		/*
> +		 * ABORT is only set on the block-layer origin write that a
> +		 * full-track format replaces. Clear the callback so the request
> +		 * is not completed here - the replacement completes it. Internal
> +		 * requests never take this path, so no sleep_on waiter is left
> +		 * without its wakeup.
> +		 */
> +		cqr->callback = NULL;

[Severity: High]
Will clearing cqr->callback actually prevent the block layer completion?

Clearing the callback avoids the immediate wakeup triggered at the end of
__dasd_process_cqr(), but it appears the request might still be completed
by the block tasklet.

> +		break;
>  	default:
>  		dev_err(&device->cdev->dev,
>  			"Unexpected CQR status %02x", cqr->status);
[ ... ]
> @@ -2767,7 +2819,8 @@ static void __dasd_process_block_ccw_queue(struct dasd_block *block,
>  		if (cqr->status != DASD_CQR_DONE &&
>  		    cqr->status != DASD_CQR_FAILED &&
>  		    cqr->status != DASD_CQR_NEED_ERP &&
> -		    cqr->status != DASD_CQR_TERMINATED)
> +		    cqr->status != DASD_CQR_TERMINATED &&
> +		    cqr->status != DASD_CQR_ABORTED)
>  			continue;
>  
>  		if (cqr->status == DASD_CQR_TERMINATED) {

[Severity: High]
If DASD_CQR_ABORTED requests fall through this check, they are moved to the
final_queue. When dasd_block_tasklet() executes, it iterates over
final_queue and calls __dasd_cleanup_cqr() unconditionally for each request:

dasd_block_tasklet()
    list_for_each_entry_safe(cqr, n, &final_queue, blocklist)
        __dasd_cleanup_cqr(cqr);

Since __dasd_cleanup_cqr() ignores cqr->callback and goes on to call
blk_mq_end_request(req, error), could this cause a double-completion in the
block layer, given that the replacement format CQR is also expected to
complete this same request?

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