[PATCH 00/19] s390/dasd: ESE Performance improvements

[PATCH 00/19] s390/dasd: ESE Performance improvements

[Stefan Haberland]

Hello Jens,

please apply the patch series for the upcoming merge window.
It applies onto your for-next branch.

The series significantly improves the performance and overall usability of
Extent Space Efficient (ESE, aka thin provisioned) DASD devices.

On a freshly provisioned ESE volume I get the following throughput
improvements, depending on workload and setup:

    workload                        change
    raw seq 1 MiB writes             ~53x
    fs bulk-load (xfs, buffered)     ~49x
    raw seq full-track (48k)        ~3.7x
    raw random 4k writes            ~1.3x
    control: fully-allocated vol      ~0% (no regression)
(fresh quick-formatted ESE volume, 4 jobs, iodepth 16, median of 3)

The first 4 patches are fixes for existing bugs in this area, afterwards
the infrastructure for and usage of Full Track Writes is added to the
driver as well as an improved collision detection. The other patches add
some tuning parameters and output as well as a new on disk format label.

FWIW: The series has been reviewed by Sashiko before. The remaining
findings are pre existing bugs that will be assessed and addressed
separately, false positives that are not a bug or not valid for ECKD
DASD devices, in-between-patch findings that are resolved at the tip
of the series, or accepted limitations.

Thanks,
Stefan

Stefan Haberland (19):
  s390/dasd: Do not complete a failed ESE read as successful
  s390/dasd: Propagate partial completion length across ERP recovery
  s390/dasd: Guard sysfs discipline callbacks against unallocated
    private data
  s390/dasd: Snapshot intrc before freeing the request block
  s390/dasd: Optimize max blocks per request for track alignment
  s390/dasd: Use GFP_KERNEL in dasd_alloc_device()
  s390/dasd: Add defines for the Extended Address Volume track address
  s390/dasd: Add infrastructure for ESE full-track write
  s390/dasd: Add range-based format-track collision detection
  s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
  s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
  s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
  s390/dasd: Add full_track_bias to control fulltrack write mode
  s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
  s390/dasd: Stamp a format label into newly formatted volumes
  s390/dasd: Detect ESE volumes from the on-disk format label
  s390/dasd: Report ESE capability and format mode at device online
  s390/dasd: Re-enable discard support for ESE volumes
  s390/dasd: Read cached unit address and LSS in the CCW build path

 drivers/s390/block/dasd.c          |  242 ++++--
 drivers/s390/block/dasd_3990_erp.c |    1 +
 drivers/s390/block/dasd_devmap.c   |   90 +-
 drivers/s390/block/dasd_eckd.c     | 1263 +++++++++++++++++++++++++---
 drivers/s390/block/dasd_eckd.h     |   68 +-
 drivers/s390/block/dasd_erp.c      |   11 +-
 drivers/s390/block/dasd_int.h      |  149 +++-
 7 files changed, 1625 insertions(+), 199 deletions(-)

-- 
2.53.0

[PATCH 01/19] s390/dasd: Do not complete a failed ESE read as successful

[Stefan Haberland]

dasd_int_handler() completes an NRF read of an unallocated ESE track by
calling ese_read() and unconditionally marking the request
DASD_CQR_SUCCESS. dasd_eckd_ese_read() can return an error before it has
zeroed the destination buffer: a failed sense-data parse or a current
track outside the requested range both return early, leaving the
destination pages untouched. The request is still completed successfully,
so the block layer is handed stale / uninitialized memory instead of
zeros.

Check the ese_read() return value and fail the request through the normal
error path instead of forcing DASD_CQR_SUCCESS.

Fixes: 5e6bdd37c552 ("s390/dasd: fix data corruption for thin provisioned devices")
Cc: stable@vger.kernel.org
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index d8d912a3b3fe..56ef38243f82 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -1698,8 +1698,10 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
 			return;
 		}
 		if (rq_data_dir(req) == READ) {
-			device->discipline->ese_read(cqr, irb);
-			cqr->status = DASD_CQR_SUCCESS;
+			if (device->discipline->ese_read(cqr, irb))
+				cqr->status = DASD_CQR_ERROR;
+			else
+				cqr->status = DASD_CQR_SUCCESS;
 			cqr->stopclk = now;
 			dasd_device_clear_timer(device);
 			dasd_schedule_device_bh(device);
-- 
2.53.0

Re: [PATCH 01/19] s390/dasd: Do not complete a failed ESE read as successful

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] When dasd_page_cache is enabled, successful ese_read operations for thin-provisioned tracks result in an information leak of uninitialized kernel slab memory to userspace.
- [High] Returning early with DASD_CQR_ERROR without clearing the timer and scheduling the bottom half causes a queue hang for ERP requests.
- [High] A failure to allocate an ERP request when retries are exhausted causes the block request to hang indefinitely on the ccw_queue.
--

--- Patch [1]: [PATCH 01/19] s390/dasd: Do not complete a failed ESE read as successful ---
commit 63ead90543228523947461ca90de80a29ba45fc6
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Do not complete a failed ESE read as successful

The patch modifies dasd_int_handler() to check the return value of
ese_read() and fail the request via the normal error path if it fails.
This prevents stale or uninitialized memory from being handed to the block
layer when reading an unallocated ESE track.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index d8d912a3b3fec..56ef38243f823 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1692,8 +1692,10 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
>  	if (dasd_ese_needs_format(cqr->block, irb)) {
>  		req = dasd_get_callback_data(cqr);
>  		if (!req) {
>  			cqr->status = DASD_CQR_ERROR;
>  			return;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this early return bypass
timer clearing and queue scheduling when req is NULL?

If an ERP request triggers a NO_REC_FOUND condition on an ESE device,
dasd_ese_needs_format() returns true. dasd_int_handler() then retrieves a
NULL req and returns early without calling dasd_device_clear_timer() or
dasd_schedule_device_bh(). Can this stall the queue until the device timer
eventually expires?

>  		if (rq_data_dir(req) == READ) {
> -			device->discipline->ese_read(cqr, irb);
> -			cqr->status = DASD_CQR_SUCCESS;
> +			if (device->discipline->ese_read(cqr, irb))
> +				cqr->status = DASD_CQR_ERROR;
> +			else
> +				cqr->status = DASD_CQR_SUCCESS;
>  			cqr->stopclk = now;
>  			dasd_device_clear_timer(device);
>  			dasd_schedule_device_bh(device);

I also noticed a few other pre-existing issues in this area.

> drivers/s390/block/dasd_eckd.c:dasd_eckd_free_cp()
> 	if (dst != cda) {
> 		if (rq_data_dir(req) == READ)
> 			memcpy(dst, cda, bv.bv_len);

[Severity: Critical]
This is a pre-existing issue, but does this unconditionally copy the
uninitialized bounce buffer over the original bio page, destroying the zeroes
written by ese_read()?

If dasd_page_cache is enabled, dasd_eckd_build_cp_cmd_single() allocates a
bounce buffer without __GFP_ZERO. If the device returns NO_REC_FOUND,
dasd_int_handler() calls ese_read(), which zero-fills the original bio
pages. On completion, does this copy overwrite the zeroes with uninitialized
memory, potentially causing an information leak to userspace?


> drivers/s390/block/dasd.c:__dasd_process_block_ccw_queue()
> 	if (cqr->status == DASD_CQR_NEED_ERP) {
> 		erp_fn = base->discipline->erp_action(cqr);
> 		if (IS_ERR(erp_fn(cqr)))
> 			continue;
> 		goto restart;
> 	}

[Severity: High]
This is a pre-existing issue, but does the continue statement here bypass
the necessary cleanup and queue movement for the failed request?

If dasd_alloc_erp_request() fails to allocate memory under memory pressure
and retries are exhausted, it sets cqr->status = DASD_CQR_FAILED and returns
an ERR_PTR. Because the code continues to the next loop iteration without
calling list_move_tail(), does the request remain on block->ccw_queue,
causing a permanent block device queue hang?

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

[PATCH 02/19] s390/dasd: Propagate partial completion length across ERP recovery

[Stefan Haberland]

dasd_default_erp_postaction() copies the timing and device state from
the finished ERP request back to the original request but drops
proc_bytes. A request that was partially completed, an ESE read of a
not-yet-allocated track returns fewer bytes than requested, and then
recovered through the ERP chain loses its partial-completion length.
__dasd_cleanup_cqr() then sees proc_bytes == 0 and completes the whole
request instead of requeueing the remainder, silently returning zeroed
data for the part that was never read.

Carry proc_bytes over to the original request like the other
per-request state.

Fixes: 5e6bdd37c552 ("s390/dasd: fix data corruption for thin provisioned devices")
Cc: stable@vger.kernel.org
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_erp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
index 89d7516b9ec8..468f0b2cc342 100644
--- a/drivers/s390/block/dasd_erp.c
+++ b/drivers/s390/block/dasd_erp.c
@@ -123,6 +123,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 	int success;
 	unsigned long startclk, stopclk;
 	struct dasd_device *startdev;
+	unsigned int proc_bytes;
 
 	BUG_ON(cqr->refers == NULL || cqr->function == NULL);
 
@@ -130,6 +131,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 	startclk = cqr->startclk;
 	stopclk = cqr->stopclk;
 	startdev = cqr->startdev;
+	proc_bytes = cqr->proc_bytes;
 
 	/* free all ERPs - but NOT the original cqr */
 	while (cqr->refers != NULL) {
@@ -147,6 +149,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 	cqr->startclk = startclk;
 	cqr->stopclk = stopclk;
 	cqr->startdev = startdev;
+	cqr->proc_bytes = proc_bytes;
 	if (success)
 		cqr->status = DASD_CQR_DONE;
 	else {
-- 
2.53.0

Re: [PATCH 02/19] s390/dasd: Propagate partial completion length across ERP recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] NULL pointer dereference in dasd_eckd_ese_read() when handling ERP requests.
--

--- Patch [2]: [PATCH 02/19] s390/dasd: Propagate partial completion length across ERP recovery ---
commit 3cc15afbf5f352896f1c63d6a8c8e105bd9920f3
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Propagate partial completion length across ERP recovery

dasd_default_erp_postaction() copies the timing and device state from
the finished ERP request back to the original request but drops
proc_bytes. A request that was partially completed, an ESE read of a
not-yet-allocated track returns fewer bytes than requested, and then
recovered through the ERP chain loses its partial-completion length.

> diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
> index 89d7516b9ec86..468f0b2cc3423 100644
> --- a/drivers/s390/block/dasd_erp.c
> +++ b/drivers/s390/block/dasd_erp.c
[ ... ]
> @@ -147,6 +149,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
>  	cqr->startclk = startclk;
>  	cqr->stopclk = stopclk;
>  	cqr->startdev = startdev;
> +	cqr->proc_bytes = proc_bytes;
>  	if (success)
>  		cqr->status = DASD_CQR_DONE;
>  	else {

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but while looking at ERP recovery, can a NULL pointer dereference
occur in dasd_eckd_ese_read() when an ERP request encounters a 'No Record
Found' error?

In drivers/s390/block/dasd.c:dasd_int_handler(), the code resolves the base
request but passes the ERP request directly to ese_read():

		req = dasd_get_callback_data(cqr);
		...
		if (rq_data_dir(req) == READ) {
			if (device->discipline->ese_read(cqr, irb))

Inside drivers/s390/block/dasd_eckd.c:dasd_eckd_ese_read(), it directly
reads callback_data:

	req = (struct request *) cqr->callback_data;
	...
	first_trk = first_blk = blk_rq_pos(req) >> block->s2b_shift;

Since ERP requests do not have callback_data assigned, could evaluating req
as NULL lead to an immediate dereference in blk_rq_pos()? Would it be safer
if dasd_eckd_ese_read() used dasd_get_callback_data(cqr) instead?

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

[PATCH 03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data

[Stefan Haberland]

Several sysfs show/store handlers call a discipline callback that
dereferences device->private, either directly or through the
DASD_DEFINE_ATTR() macro. During dasd_generic_set_online() the discipline
is assigned before check_device() allocates device->private, so an
unprivileged read of one of these world-readable attributes in that window
dereferences a NULL pointer and panics.

Guard the dereference inside each callback that actually touches
device->private.

Fixes: c729696bcf8b ("s390/dasd: Recognise data for ESE volumes")
Cc: stable@vger.kernel.org
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 40 ++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index d356a9f8f016..b64ca714b53e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1492,6 +1492,8 @@ static void dasd_eckd_reset_path(struct dasd_device *device, __u8 pm)
 	struct dasd_eckd_private *private = device->private;
 	unsigned long flags;
 
+	if (!private)
+		return;
 	if (!private->fcx_max_data)
 		private->fcx_max_data = get_fcx_max_data(device);
 	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
@@ -1647,6 +1649,9 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->vsq.vol_info.ese;
 }
 
@@ -1654,6 +1659,9 @@ static int dasd_eckd_ext_pool_id(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->vsq.extent_pool_id;
 }
 
@@ -1667,6 +1675,9 @@ static int dasd_eckd_space_configured(struct dasd_device *device)
 	struct dasd_eckd_private *private = device->private;
 	int rc;
 
+	if (!private)
+		return 0;
+
 	rc = dasd_eckd_read_vol_info(device);
 
 	return rc ? : private->vsq.space_configured;
@@ -1681,6 +1692,9 @@ static int dasd_eckd_space_allocated(struct dasd_device *device)
 	struct dasd_eckd_private *private = device->private;
 	int rc;
 
+	if (!private)
+		return 0;
+
 	rc = dasd_eckd_read_vol_info(device);
 
 	return rc ? : private->vsq.space_allocated;
@@ -1690,6 +1704,9 @@ static int dasd_eckd_logical_capacity(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->vsq.logical_capacity;
 }
 
@@ -1832,7 +1849,11 @@ static int dasd_eckd_read_ext_pool_info(struct dasd_device *device)
 static int dasd_eckd_ext_size(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
-	struct dasd_ext_pool_sum eps = private->eps;
+	struct dasd_ext_pool_sum eps;
+
+	if (!private)
+		return 0;
+	eps = private->eps;
 
 	if (!eps.flags.extent_size_valid)
 		return 0;
@@ -1848,6 +1869,9 @@ static int dasd_eckd_ext_pool_warn_thrshld(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->eps.warn_thrshld;
 }
 
@@ -1855,6 +1879,9 @@ static int dasd_eckd_ext_pool_cap_at_warnlevel(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->eps.flags.capacity_at_warnlevel;
 }
 
@@ -1865,6 +1892,9 @@ static int dasd_eckd_ext_pool_oos(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->eps.flags.pool_oos;
 }
 
@@ -5938,8 +5968,11 @@ static int dasd_eckd_query_host_access(struct dasd_device *device,
 	struct ccw1 *ccw;
 	int rc;
 
+	if (!private)
+		return -ENODEV;
+
 	/* not available for HYPER PAV alias devices */
-	if (!device->block && private->lcu->pav == HYPER_PAV)
+	if (!device->block && private->lcu && private->lcu->pav == HYPER_PAV)
 		return -EOPNOTSUPP;
 
 	/* may not be supported by the storage server */
@@ -6804,6 +6837,9 @@ static int dasd_eckd_hpf_enabled(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
+	if (!private)
+		return 0;
+
 	return private->fcx_max_data ? 1 : 0;
 }
 
-- 
2.53.0

Re: [PATCH 03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but a Time-Of-Check to Time-Of-Use (TOCTOU) Use-After-Free vulnerability exists in world-readable sysfs handlers when the initialization function `dasd_eckd_check_characteristics()` encounters an error.
- [High] The patch fixes NULL pointer dereferences in world-readable sysfs show handlers by guarding them against an unallocated `device->private` during `dasd_generic_set_online()`. However, it misses the `copy_pair` sysfs store handler, leaving it vulnerable to a NULL pointer dereference.
- [High] The patch leaves a vulnerable window where `device->private` is allocated but its internal pointers (like `private->conf.gneq`) are still NULL, causing a NULL pointer dereference when a sysfs attribute is written.
--

--- Patch [3]: [PATCH 03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit 3d2b943842a29d7f0ee2ab694c84c9d52b911329
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Guard sysfs discipline callbacks against unallocated private data

Several sysfs show/store handlers call a discipline callback that
dereferences device->private, either directly or through the
DASD_DEFINE_ATTR() macro. During dasd_generic_set_online() the discipline
is assigned before check_device() allocates device->private, so an
unprivileged read of one of these world-readable attributes in that window
dereferences a NULL pointer and panics.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index d356a9f8f016f..b64ca714b53e7 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -1492,6 +1492,8 @@ static void dasd_eckd_reset_path(struct dasd_device *device, __u8 pm)
>  	struct dasd_eckd_private *private = device->private;
>  	unsigned long flags;
>  
> +	if (!private)
> +		return;
>  	if (!private->fcx_max_data)
>  		private->fcx_max_data = get_fcx_max_data(device);

[Severity: High]
This isn't a bug introduced by this patch, but does this leave a window
where private->conf.gneq is dereferenced while NULL?

In dasd_eckd_check_characteristics(), device->private is allocated and
assigned, but then blocking I/O (dasd_eckd_read_conf()) occurs before
private->conf.gneq is set.

If a sysfs write for reset_path occurs during this sleep, the new !private
check passes and get_fcx_max_data() is called:

drivers/s390/block/dasd_eckd.c:get_fcx_max_data() {
    ...
    fcx_in_gneq = private->conf.gneq->reserved2[7] & 0x04;
    ...
}

Can this cause a kernel panic if private->conf.gneq is still NULL?

[ ... ]

> @@ -1647,6 +1649,9 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
>  {
>  	struct dasd_eckd_private *private = device->private;
>  
> +	if (!private)
> +		return 0;
> +
>  	return private->vsq.vol_info.ese;
>  }

[Severity: Critical]
This is a pre-existing issue, but does this lockless check introduce a
Time-Of-Check to Time-Of-Use (TOCTOU) Use-After-Free?

If dasd_generic_set_online() is running concurrently and
dasd_eckd_check_characteristics() encounters an error (like an I/O failure),
it manually frees private:

drivers/s390/block/dasd_eckd.c:dasd_eckd_check_characteristics() {
    ...
out_err1:
    dasd_eckd_clear_conf_data(device);
    dasd_path_remove_kobjects(device);
    kfree(device->private);
    device->private = NULL;
    ...
}

Since sysfs readers like dasd_eckd_is_ese() don't hold a lock protecting
device->private, could private be freed immediately after the !private
check but before private->vsq.vol_info.ese is accessed?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the copy_pair
sysfs store handler is missing a similar safeguard.

If an administrator writes to copy_pair concurrently with
dasd_generic_set_online(), it executes dasd_copy_pair_store() which
locklessly accesses the discipline:

drivers/s390/block/dasd_devmap.c:dasd_copy_pair_store() {
    ...
    if (device->discipline->pprc_enabled) {
        pprc_enabled = device->discipline->pprc_enabled(device);
    ...
}

If device->discipline is NULL during the window before it is assigned,
could this cause a NULL pointer dereference?

Furthermore, even if discipline is assigned, dasd_eckd_pprc_enabled()
unconditionally dereferences device->private without checking if it is
allocated:

drivers/s390/block/dasd_eckd.c:dasd_eckd_pprc_enabled() {
    struct dasd_eckd_private *private = device->private;
    
    return private->rdc_data.facilities.PPRC_enabled;
}

Should these locations be guarded as well?

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

[PATCH 04/19] s390/dasd: Snapshot intrc before freeing the request block

[Stefan Haberland]

__dasd_cleanup_cqr() maps the completion result to a block status by
reading cqr->intrc, but only after discipline->free_cp() has returned the
request block to its memory pool (dasd_eckd_free_cp() ends in
dasd_sfree_request()). On SMP another CPU can reallocate that block and
overwrite cqr->intrc before it is read, completing the request with the
wrong error. proc_bytes is already snapshotted before free_cp() for the
same reason; do the same for intrc.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 56ef38243f82..34b563917098 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -2699,17 +2699,23 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
 	struct request *req;
 	blk_status_t error = BLK_STS_OK;
 	unsigned int proc_bytes;
-	int status;
+	int status, intrc;
 
 	req = (struct request *) cqr->callback_data;
 	dasd_profile_end(cqr->block, cqr, req);
 
+	/*
+	 * free_cp() returns the request block to its memory pool, so snapshot
+	 * everything still needed from cqr before calling it - another CPU can
+	 * reallocate and overwrite the block right after.
+	 */
 	proc_bytes = cqr->proc_bytes;
+	intrc = cqr->intrc;
 	status = cqr->block->base->discipline->free_cp(cqr, req);
 	if (status < 0)
 		error = errno_to_blk_status(status);
 	else if (status == 0) {
-		switch (cqr->intrc) {
+		switch (intrc) {
 		case -EPERM:
 			/*
 			 * DASD doesn't implement SCSI/NVMe reservations, but it
-- 
2.53.0

Re: [PATCH 04/19] s390/dasd: Snapshot intrc before freeing the request block

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing `blk_rq_has_data()` / `bio_has_data()` guard in `dasd_fba_free_cp` causes NULL pointer dereference for dataless requests.
- [High] Accessing `req->q` after calling `blk_mq_end_request()` results in a use-after-free.
- [High] Dereferencing `cqr->dq` to release a lock after `cqr` has been freed leads to a use-after-free in `dasd_flush_block_queue()`.
- [High] Block layer request use-after-free in `dasd_generic_requeue_all_requests` error path.
--

--- Patch [4]: [PATCH 04/19] s390/dasd: Snapshot intrc before freeing the request block ---
commit 8cd8a0d1124e90e3f5df17a63495a14c3de26d80
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Snapshot intrc before freeing the request block
    
This patch fixes a race condition where the request block's intrc field is
read after the block is returned to the memory pool. By snapshotting intrc
before calling free_cp(), it prevents concurrent reallocations from
overwriting the completion status.

> 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
> @@ -2699,17 +2699,23 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
>  	struct request *req;
>  	blk_status_t error = BLK_STS_OK;
>  	unsigned int proc_bytes;
> -	int status;
> +	int status, intrc;
>  
>  	req = (struct request *) cqr->callback_data;
>  	dasd_profile_end(cqr->block, cqr, req);
>  
> +	/*
> +	 * free_cp() returns the request block to its memory pool, so snapshot
> +	 * everything still needed from cqr before calling it - another CPU can
> +	 * reallocate and overwrite the block right after.
> +	 */
>  	proc_bytes = cqr->proc_bytes;
> +	intrc = cqr->intrc;
>  	status = cqr->block->base->discipline->free_cp(cqr, req);
>  	if (status < 0)
>  		error = errno_to_blk_status(status);
>  	else if (status == 0) {
> -		switch (cqr->intrc) {
> +		switch (intrc) {
>  		case -EPERM:
>  			/*
>  			 * DASD doesn't implement SCSI/NVMe reservations, but it

[Severity: High]
This is a pre-existing issue, but since we are fixing a use-after-free
around free_cp() returning the block to the pool, does
dasd_flush_block_queue() have a similar problem?

drivers/s390/block/dasd.c:dasd_flush_block_queue() {
    ...
    __dasd_cleanup_cqr(cqr);
    spin_unlock_irqrestore(&cqr->dq->lock, flags);
    ...
}

Once __dasd_cleanup_cqr(cqr) completes, free_cp() has been called and
the cqr memory block might be reallocated by another thread. Does the
subsequent access to cqr->dq->lock dereference freed memory?

[Severity: High]
This is also a pre-existing issue, but is there a use-after-free on req->q 
later in __dasd_cleanup_cqr() and dasd_request_done()?

drivers/s390/block/dasd.c:__dasd_cleanup_cqr() {
    ...
    blk_mq_end_request(req, error);
    blk_mq_run_hw_queues(req->q, true);
    ...
}

The blk_mq_end_request() call completes the request, meaning the block
layer can instantly reallocate or poison the memory backing req. Does
accessing req->q on the next line dereference this potentially freed
memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in 
dasd_generic_requeue_all_requests() expose the request to a concurrent 
use-after-free?

drivers/s390/block/dasd.c:dasd_generic_requeue_all_requests() {
    ...
    _dasd_requeue_request(cqr);
    list_del_init(&cqr->blocklist);
    cqr->block->base->discipline->free_cp(cqr, 
            (struct request *) cqr->callback_data);
    ...
}

The _dasd_requeue_request(cqr) call invokes blk_mq_requeue_request(req, true),
handing the request back to the block layer and asynchronously kicking the
hardware queue. Can another thread immediately dispatch and complete the
request before free_cp() iterates over the request's bios?

[Severity: Critical]
This is a pre-existing issue, but can dataless requests cause a null pointer 
dereference in dasd_fba_free_cp()?

drivers/s390/block/dasd_fba.c:dasd_fba_free_cp() {
    ...
    rq_for_each_segment(bv, req, iter) {
        dst = bvec_virt(&bv);
        for (off = 0; off < bv.bv_len; off += blksize) {
    ...
}

Operations like REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES have no data
buffers (bio->bi_io_vec is null) but can have a non-zero bi_size. The
rq_for_each_segment() macro expands to bio_for_each_segment(), which
attempts to fetch the current segment from the null bi_io_vec. Should
this iteration be guarded with a check for blk_rq_has_data(req)?

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

[PATCH 05/19] s390/dasd: Optimize max blocks per request for track alignment

[Stefan Haberland]

With 4096-byte blocks a full ECKD track holds exactly 12 records. Lower
DASD_ECKD_MAX_BLOCKS from 190 to 180 so requests align to track
boundaries (15 full tracks); full-track I/O is more efficient than
partial-track writes, and 190 had no alignment significance and could
let a request cross a track boundary.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index f9299bd184ba..763733bcc4d2 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -131,7 +131,7 @@
 /*
  * Maximum number of blocks to be chained
  */
-#define DASD_ECKD_MAX_BLOCKS		 190
+#define DASD_ECKD_MAX_BLOCKS		 180
 #define DASD_ECKD_MAX_BLOCKS_RAW	 256
 
 /*****************************************************************************
-- 
2.53.0

Re: [PATCH 05/19] s390/dasd: Optimize max blocks per request for track alignment

[sashiko-bot]

> With 4096-byte blocks a full ECKD track holds exactly 12 records. Lower
> DASD_ECKD_MAX_BLOCKS from 190 to 180 so requests align to track
> boundaries (15 full tracks); full-track I/O is more efficient than
> partial-track writes, and 190 had no alignment significance and could
> let a request cross a track boundary.
> 
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH 06/19] s390/dasd: Use GFP_KERNEL in dasd_alloc_device()

[Stefan Haberland]

dasd_alloc_device() runs in process context (device set_online), so its
pool allocations do not need GFP_ATOMIC. Use GFP_KERNEL instead, which is
more reliable, especially for the larger DMA allocations that later ESE
full-track work adds here.

No functional change intended.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 34b563917098..12fa04537fb0 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -87,25 +87,25 @@ struct dasd_device *dasd_alloc_device(void)
 {
 	struct dasd_device *device;
 
-	device = kzalloc_obj(struct dasd_device, GFP_ATOMIC);
+	device = kzalloc_obj(struct dasd_device, GFP_KERNEL);
 	if (!device)
 		return ERR_PTR(-ENOMEM);
 
 	/* Get two pages for normal block device operations. */
-	device->ccw_mem = (void *) __get_free_pages(GFP_ATOMIC | GFP_DMA, 1);
+	device->ccw_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 1);
 	if (!device->ccw_mem) {
 		kfree(device);
 		return ERR_PTR(-ENOMEM);
 	}
 	/* Get one page for error recovery. */
-	device->erp_mem = (void *) get_zeroed_page(GFP_ATOMIC | GFP_DMA);
+	device->erp_mem = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
 	if (!device->erp_mem) {
 		free_pages((unsigned long) device->ccw_mem, 1);
 		kfree(device);
 		return ERR_PTR(-ENOMEM);
 	}
 	/* Get two pages for ese format. */
-	device->ese_mem = (void *)__get_free_pages(GFP_ATOMIC | GFP_DMA, 1);
+	device->ese_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 1);
 	if (!device->ese_mem) {
 		free_page((unsigned long) device->erp_mem);
 		free_pages((unsigned long) device->ccw_mem, 1);
-- 
2.53.0

Re: [PATCH 06/19] s390/dasd: Use GFP_KERNEL in dasd_alloc_device()

[sashiko-bot]

> dasd_alloc_device() runs in process context (device set_online), so its
> pool allocations do not need GFP_ATOMIC. Use GFP_KERNEL instead, which is
> more reliable, especially for the larger DMA allocations that later ESE
> full-track work adds here.
> 
> No functional change intended.
> 
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH 07/19] s390/dasd: Add defines for the Extended Address Volume track address

[Stefan Haberland]

The track address of an Extended Address Volume (more than 65520
cylinders) carries the high cylinder bits that do not fit the 16-bit cyl
field in the upper part of the head field. set_ch_t() open-codes the
corresponding shifts; name them so the encoding is explicit and can be
reused.

No functional change.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 4 ++--
 drivers/s390/block/dasd_eckd.h | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index b64ca714b53e..52d008b59561 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -200,8 +200,8 @@ recs_per_track(struct dasd_eckd_characteristics * rdc,
 static void set_ch_t(struct ch_t *geo, __u32 cyl, __u8 head)
 {
 	geo->cyl = (__u16) cyl;
-	geo->head = cyl >> 16;
-	geo->head <<= 4;
+	geo->head = cyl >> DASD_EAV_CYL_HI_SHIFT;
+	geo->head <<= DASD_EAV_HEAD_HI_SHIFT;
 	geo->head |= head;
 }
 
diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index 763733bcc4d2..bad7ba666370 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -146,6 +146,14 @@ struct eckd_count {
 	__u16 dl;
 } __attribute__ ((packed));
 
+/*
+ * Extended Address Volume track address: the head field carries the actual
+ * head in its low-order 4 bits; the cylinder bits that do not fit the 16-bit
+ * cyl field are shifted in just above them.
+ */
+#define DASD_EAV_CYL_HI_SHIFT	16	/* cylinder bits beyond the 16-bit cyl field */
+#define DASD_EAV_HEAD_HI_SHIFT	4	/* head occupies the low-order 4 bits of head */
+
 struct ch_t {
 	__u16 cyl;
 	__u16 head;
-- 
2.53.0

Re: [PATCH 07/19] s390/dasd: Add defines for the Extended Address Volume track address

[sashiko-bot]

> The track address of an Extended Address Volume (more than 65520
> cylinders) carries the high cylinder bits that do not fit the 16-bit cyl
> field in the upper part of the head field. set_ch_t() open-codes the
> corresponding shifts; name them so the encoding is explicit and can be
> reused.
> 
> No functional change.
> 
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[Stefan Haberland]

Add the driver internals to build WRITE_FULL_TRACK FCX channel programs
in response to unformatted tracks on ESE devices.

struct dasd_ccw_req: filldata, a pointer to the per-track metadata (an R0
record and the count records) that the WRITE_FULL_TRACK TIDAWs point at,
and format/start_trk/end_trk/collision that link a request to its
format-track guard entry so an overlapping format request can be detected.

struct dasd_device: fill_mem/fill_chunks pool for those buffers and a
zeroed nulldata page used as the data source for pad records.

struct dasd_block: ese_staging/ese_lock, a hardirq-safe staging list. An
ESE format CQR is created in the interrupt handler but has to be enqueued
on ccw_queue under queue_lock; taking queue_lock while the ccwdev_lock is
held there would invert the lock order, so the CQR is staged under ese_lock
and dasd_block_tasklet splices it onto ccw_queue. Existing locking is
unchanged.

Add CQR states DASD_CQR_ABORT/ABORTED to retire the origin CQR of a
replaced write without completing it to the block layer, and struct
eckd_r0 for the track header record.

The CCW and ESE format pools are enlarged (a full-track ITCW is roughly
twice a plain track-mode one) to keep two maximum-size requests in flight.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      | 100 ++++++++++++++++++++++++++++-----
 drivers/s390/block/dasd_eckd.c |  11 ++++
 drivers/s390/block/dasd_eckd.h |   5 ++
 drivers/s390/block/dasd_int.h  |  19 +++++++
 4 files changed, 120 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 12fa04537fb0..761d559101a9 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -91,31 +91,53 @@ struct dasd_device *dasd_alloc_device(void)
 	if (!device)
 		return ERR_PTR(-ENOMEM);
 
-	/* Get two pages for normal block device operations. */
-	device->ccw_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 1);
+	/*
+	 * Four pages: a full-track ITCW is roughly twice the size of a plain
+	 * track-mode one, so this keeps two maximum-size requests in flight.
+	 */
+	device->ccw_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 2);
 	if (!device->ccw_mem) {
 		kfree(device);
 		return ERR_PTR(-ENOMEM);
 	}
+	/* per-request track-filler buffers (R0 + count records) */
+	device->fill_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 1);
+	if (!device->fill_mem) {
+		free_pages((unsigned long)device->ccw_mem, 2);
+		kfree(device);
+		return ERR_PTR(-ENOMEM);
+	}
 	/* Get one page for error recovery. */
 	device->erp_mem = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
 	if (!device->erp_mem) {
-		free_pages((unsigned long) device->ccw_mem, 1);
+		free_pages((unsigned long)device->fill_mem, 1);
+		free_pages((unsigned long)device->ccw_mem, 2);
 		kfree(device);
 		return ERR_PTR(-ENOMEM);
 	}
-	/* Get two pages for ese format. */
-	device->ese_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 1);
+	/* sized like ccw_chunks: two max-size NRF format requests in flight */
+	device->ese_mem = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA, 2);
 	if (!device->ese_mem) {
-		free_page((unsigned long) device->erp_mem);
-		free_pages((unsigned long) device->ccw_mem, 1);
+		free_page((unsigned long)device->erp_mem);
+		free_pages((unsigned long)device->fill_mem, 1);
+		free_pages((unsigned long)device->ccw_mem, 2);
+		kfree(device);
+		return ERR_PTR(-ENOMEM);
+	}
+	device->nulldata = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
+	if (!device->nulldata) {
+		free_page((unsigned long)device->erp_mem);
+		free_pages((unsigned long)device->fill_mem, 1);
+		free_pages((unsigned long)device->ccw_mem, 2);
+		free_pages((unsigned long)device->ese_mem, 2);
 		kfree(device);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	dasd_init_chunklist(&device->ccw_chunks, device->ccw_mem, PAGE_SIZE*2);
+	dasd_init_chunklist(&device->ccw_chunks, device->ccw_mem, PAGE_SIZE * 4);
+	dasd_init_chunklist(&device->fill_chunks, device->fill_mem, PAGE_SIZE * 2);
 	dasd_init_chunklist(&device->erp_chunks, device->erp_mem, PAGE_SIZE);
-	dasd_init_chunklist(&device->ese_chunks, device->ese_mem, PAGE_SIZE * 2);
+	dasd_init_chunklist(&device->ese_chunks, device->ese_mem, PAGE_SIZE * 4);
 	spin_lock_init(&device->mem_lock);
 	atomic_set(&device->tasklet_scheduled, 0);
 	tasklet_init(&device->tasklet, dasd_device_tasklet,
@@ -138,9 +160,11 @@ struct dasd_device *dasd_alloc_device(void)
 void dasd_free_device(struct dasd_device *device)
 {
 	kfree(device->private);
-	free_pages((unsigned long) device->ese_mem, 1);
-	free_page((unsigned long) device->erp_mem);
-	free_pages((unsigned long) device->ccw_mem, 1);
+	free_pages((unsigned long)device->ese_mem, 2);
+	free_page((unsigned long)device->erp_mem);
+	free_pages((unsigned long)device->fill_mem, 1);
+	free_pages((unsigned long)device->ccw_mem, 2);
+	free_page((unsigned long)device->nulldata);
 	kfree(device);
 }
 
@@ -164,6 +188,8 @@ struct dasd_block *dasd_alloc_block(void)
 	spin_lock_init(&block->queue_lock);
 	INIT_LIST_HEAD(&block->format_list);
 	spin_lock_init(&block->format_lock);
+	INIT_LIST_HEAD(&block->ese_staging);
+	spin_lock_init(&block->ese_lock);
 	timer_setup(&block->timer, dasd_block_timeout, 0);
 	spin_lock_init(&block->profile.lock);
 
@@ -364,7 +390,8 @@ int _wait_for_empty_queues(struct dasd_device *device)
 {
 	if (device->block)
 		return list_empty(&device->ccw_queue) &&
-			list_empty(&device->block->ccw_queue);
+			list_empty(&device->block->ccw_queue) &&
+			list_empty(&device->block->ese_staging);
 	else
 		return list_empty(&device->ccw_queue);
 }
@@ -1224,7 +1251,18 @@ void dasd_sfree_request(struct dasd_ccw_req *cqr, struct dasd_device *device)
 	unsigned long flags;
 
 	spin_lock_irqsave(&device->mem_lock, flags);
-	dasd_free_chunk(&device->ccw_chunks, cqr->mem_chunk);
+	/*
+	 * Free the request block from the pool it came from: smalloc() sets
+	 * mem_chunk (ccw_chunks), fmalloc() leaves it NULL (ese_chunks). A
+	 * full-track request also frees its track-filler buffer.
+	 */
+	if (cqr->filldata)
+		dasd_free_chunk(&device->fill_chunks, cqr->filldata);
+	if (cqr->mem_chunk)
+		dasd_free_chunk(&device->ccw_chunks, cqr->mem_chunk);
+	else
+		dasd_free_chunk(&device->ese_chunks, cqr);
+
 	spin_unlock_irqrestore(&device->mem_lock, flags);
 	dasd_put_device(device);
 }
@@ -1235,6 +1273,8 @@ void dasd_ffree_request(struct dasd_ccw_req *cqr, struct dasd_device *device)
 	unsigned long flags;
 
 	spin_lock_irqsave(&device->mem_lock, flags);
+	if (cqr->filldata)
+		dasd_free_chunk(&device->fill_chunks, cqr->filldata);
 	dasd_free_chunk(&device->ese_chunks, cqr);
 	spin_unlock_irqrestore(&device->mem_lock, flags);
 	dasd_put_device(device);
@@ -1885,6 +1925,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;
+		break;
 	default:
 		dev_err(&device->cdev->dev,
 			"Unexpected CQR status %02x", cqr->status);
@@ -2212,6 +2263,7 @@ EXPORT_SYMBOL(dasd_add_request_tail);
 void dasd_wakeup_cb(struct dasd_ccw_req *cqr, void *data)
 {
 	spin_lock_irq(get_ccwdev_lock(cqr->startdev->cdev));
+	cqr->endclk = get_tod_clock();
 	cqr->callback_data = DASD_SLEEPON_END_TAG;
 	spin_unlock_irq(get_ccwdev_lock(cqr->startdev->cdev));
 	wake_up(&generic_waitq);
@@ -2779,7 +2831,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) {
@@ -2890,6 +2943,14 @@ static void dasd_block_tasklet(unsigned long data)
 	atomic_set(&block->tasklet_scheduled, 0);
 	INIT_LIST_HEAD(&final_queue);
 	spin_lock_irq(&block->queue_lock);
+	/*
+	 * Splice the hardirq-staged ESE format CQRs onto ccw_queue. Splice to
+	 * the tail so an aborted origin request (already on ccw_queue) is
+	 * retired before its format-CQR replacement completes and requeues it.
+	 */
+	spin_lock(&block->ese_lock);
+	list_splice_tail_init(&block->ese_staging, &block->ccw_queue);
+	spin_unlock(&block->ese_lock);
 	/* Finish off requests on ccw queue */
 	__dasd_process_block_ccw_queue(block, &final_queue);
 	spin_unlock_irq(&block->queue_lock);
@@ -2949,6 +3010,15 @@ static int _dasd_requests_to_flushqueue(struct dasd_block *block,
 	int rc, i;
 
 	spin_lock_irqsave(&block->queue_lock, flags);
+	/*
+	 * Splice any hardirq-staged ESE format CQRs onto ccw_queue first so
+	 * they are seen and canceled by the walk below instead of being
+	 * orphaned across this flush / state transition. Mirrors the splice
+	 * in dasd_block_tasklet().
+	 */
+	spin_lock(&block->ese_lock);
+	list_splice_tail_init(&block->ese_staging, &block->ccw_queue);
+	spin_unlock(&block->ese_lock);
 	rc = 0;
 restart:
 	list_for_each_entry_safe(cqr, n, &block->ccw_queue, blocklist) {
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 52d008b59561..8a1eac8aa63e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -205,6 +205,17 @@ static void set_ch_t(struct ch_t *geo, __u32 cyl, __u8 head)
 	geo->head |= head;
 }
 
+static __maybe_unused void set_chr_t(void *addr, __u32 cyl, __u8 head, __u8 record)
+{
+	struct chr_t *geo = addr;
+
+	geo->cyl = (__u16)cyl;
+	geo->head = cyl >> DASD_EAV_CYL_HI_SHIFT;
+	geo->head <<= DASD_EAV_HEAD_HI_SHIFT;
+	geo->head |= head;
+	geo->record = record;
+}
+
 /*
  * calculate failing track from sense data depending if
  * it is an EAV device or not
diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index bad7ba666370..0fdb92fdddc8 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -146,6 +146,11 @@ struct eckd_count {
 	__u16 dl;
 } __attribute__ ((packed));
 
+struct eckd_r0 {
+	struct eckd_count count;
+	__u8 data[8];
+} __packed;
+
 /*
  * Extended Address Volume track address: the head field carries the actual
  * head in its low-order 4 bits; the cylinder bits that do not fit the 16-bit
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 81cfb5c89681..cab16907ea5a 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -159,6 +159,11 @@ struct dasd_ccw_req {
 	void *callback_data;
 	unsigned int proc_bytes;	/* bytes for partial completion */
 	unsigned int trkcount;		/* count formatted tracks */
+	void *filldata;			/* address of filler data */
+	struct dasd_format_entry *format;
+	sector_t start_trk;
+	sector_t end_trk;
+	bool collision;
 };
 
 /*
@@ -170,6 +175,7 @@ struct dasd_ccw_req {
 #define DASD_CQR_IN_ERP 	0x03	/* request is in recovery */
 #define DASD_CQR_FAILED 	0x04	/* request is finally failed */
 #define DASD_CQR_TERMINATED	0x05	/* request was stopped by driver */
+#define DASD_CQR_ABORTED	0x06	/* request was replaced and will be deleted */
 
 #define DASD_CQR_QUEUED 	0x80	/* request is queued to be processed */
 #define DASD_CQR_IN_IO		0x81	/* request is currently in IO */
@@ -177,6 +183,7 @@ struct dasd_ccw_req {
 #define DASD_CQR_CLEAR_PENDING	0x83	/* request is clear pending */
 #define DASD_CQR_CLEARED	0x84	/* request was cleared */
 #define DASD_CQR_SUCCESS	0x85	/* request was successful */
+#define DASD_CQR_ABORT		0x86	/* request was replaced and will not be handled */
 
 /* default expiration time*/
 #define DASD_EXPIRES	  300
@@ -573,9 +580,12 @@ struct dasd_device {
 	struct list_head ccw_queue;
 	spinlock_t mem_lock;
 	void *ccw_mem;
+	void *fill_mem;
 	void *erp_mem;
 	void *ese_mem;
+	void *nulldata;
 	struct list_head ccw_chunks;
+	struct list_head fill_chunks;
 	struct list_head erp_chunks;
 	struct list_head ese_chunks;
 
@@ -640,6 +650,15 @@ struct dasd_block {
 	struct list_head format_list;
 	spinlock_t format_lock;
 	atomic_t trkcount;
+
+	/*
+	 * ESE format CQRs staged from hardirq, spliced into
+	 * ccw_queue in dasd_block_tasklet under queue_lock. Direct enqueue from
+	 * the IRQ handler would invert the queue_lock / ccwdev_lock order.
+	 */
+	struct list_head ese_staging;
+	/* lock for ese_staging */
+	spinlock_t ese_lock;
 };
 
 struct dasd_attention_data {
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Double completion of aborted origin CQR due to flawed completion prevention logic.
- [High] Lockless TOCTOU race in `_wait_for_empty_queues` evaluates queues in the wrong order.
--

--- Patch [8]: [PATCH 08/19] s390/dasd: Add infrastructure for ESE full-track write ---
commit d9f946540847a2c32306276982008cd999ef5ab9
Author: Stefan Haberland <sth@linux.ibm.com>

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
track-filler buffers, a hardirq-safe staging list for ESE format CQRs,
and new CQR states to retire replaced origin writes.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 12fa04537fb02..761d559101a91 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -364,7 +390,8 @@ int _wait_for_empty_queues(struct dasd_device *device)
>  {
>  	if (device->block)
>  		return list_empty(&device->ccw_queue) &&
> -			list_empty(&device->block->ccw_queue);
> +			list_empty(&device->block->ccw_queue) &&
> +			list_empty(&device->block->ese_staging);

[Severity: High]
Does this lockless evaluation introduce a time-of-check to time-of-use race
by checking the downstream queue before the upstream queue?

If CPU A checks block->ccw_queue and finds it empty, and CPU B subsequently
splices a request from ese_staging into block->ccw_queue, CPU A will then
check ese_staging and also find it empty. CPU A would return true,
prematurely signaling that all queues are clear while an I/O request
is actually in flight on block->ccw_queue.

[ ... ]
> @@ -1885,6 +1925,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;
> +		break;
>  	default:
[ ... ]
> @@ -2779,7 +2831,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;

[Severity: High]
Can this lead to a double completion and use-after-free for the aborted
origin request?

In __dasd_process_cqr(), the aborted request has its cqr->callback set to
NULL to prevent it from completing. However, here in
__dasd_process_block_ccw_queue(), DASD_CQR_ABORTED is explicitly added to the
finished states, which routes the request to final_queue.

When final_queue is processed by __dasd_cleanup_cqr(), it unconditionally
completes the block request using cqr->callback_data, ignoring the fact that
cqr->callback is NULL.

When the replacement ESE format CQR later finishes, it will complete the same
request again, resulting in a double completion.

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

[PATCH 09/19] s390/dasd: Add range-based format-track collision detection

[Stefan Haberland]

Replace the single per-device format_entry slot with an array of 16
slots so multiple format requests can be in flight at once, and extend
struct dasd_format_entry with a start_trk/end_trk/cqr range (replacing
the single track field).

Rewrite test_and_set_format_track() to scan the array for range overlaps
instead of a trkcount snapshot, honour the early-collision flag, and
return the allocated slot to the caller.

Add dasd_req_conflict() and extend dasd_return_cqr_cb() to mark in-flight
data CQRs that overlap a just-completed format range, so the next
test_and_set_format_track() detects the conflict early.

Remove the now-obsolete trkcount snapshot in dasd_start_IO().

The detection added here only becomes active together with the
WRITE_FULL_TRACK ESE format handler later in the series: that patch routes
the format request through dasd_return_cqr_cb() (so completion runs the
overlap hook with cqr->format set) and records each request's
start_trk/end_trk range. Until then the array and the conflict check are in
place but dormant.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      | 29 ++++++++++++----
 drivers/s390/block/dasd_eckd.c | 62 +++++++++++++++++++++++-----------
 drivers/s390/block/dasd_int.h  | 19 +++++++++--
 3 files changed, 81 insertions(+), 29 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 761d559101a9..243dec4b4484 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -1402,13 +1402,6 @@ int dasd_start_IO(struct dasd_ccw_req *cqr)
 		if (!cqr->lpm)
 			cqr->lpm = dasd_path_get_opm(device);
 	}
-	/*
-	 * remember the amount of formatted tracks to prevent double format on
-	 * ESE devices
-	 */
-	if (cqr->block)
-		cqr->trkcount = atomic_read(&cqr->block->trkcount);
-
 	if (cqr->cpmode == 1) {
 		rc = ccw_device_tm_start(device->cdev, cqr->cpaddr,
 					 (long) cqr, cqr->lpm);
@@ -2880,6 +2873,28 @@ static void __dasd_process_block_ccw_queue(struct dasd_block *block,
 
 static void dasd_return_cqr_cb(struct dasd_ccw_req *cqr, void *data)
 {
+	struct dasd_ccw_req *temp_cqr;
+	struct dasd_block *block;
+
+	/* only format CQRs are candidates */
+	if (!cqr->block || unlikely(!cqr->format))
+		goto out;
+
+	block = cqr->block;
+	/*
+	 * Mark in-flight (IN_IO) CQRs that overlap this just-completed format
+	 * range so they re-check in test_and_set_format on completion; FILLED
+	 * or QUEUED CQRs re-check the format_list on their next round anyway.
+	 */
+	list_for_each_entry(temp_cqr, &block->ccw_queue, blocklist) {
+		if (temp_cqr != cqr &&
+		    temp_cqr->status != DASD_CQR_FILLED &&
+		    temp_cqr->status != DASD_CQR_QUEUED &&
+		    dasd_req_conflict(cqr, temp_cqr)) {
+			WRITE_ONCE(temp_cqr->collision, true);
+		}
+	}
+out:
 	dasd_schedule_block_bh(cqr->block);
 }
 
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 8a1eac8aa63e..be54c356dc24 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3151,32 +3151,44 @@ static int dasd_eckd_format_device(struct dasd_device *base,
 					     0, NULL);
 }
 
-static bool test_and_set_format_track(struct dasd_format_entry *to_format,
-				      struct dasd_ccw_req *cqr)
+static bool test_and_set_format_track(sector_t start, sector_t end,
+				      struct dasd_ccw_req *cqr,
+				      struct dasd_block *block,
+				      struct dasd_device *device,
+				      struct dasd_format_entry **entry)
 {
-	struct dasd_block *block = cqr->block;
-	struct dasd_format_entry *format;
+	struct dasd_format_entry *to_format, *format;
 	unsigned long flags;
 	bool rc = false;
+	int i = 0;
 
+	/* marked as a collision by dasd_return_cqr_cb last round: retry */
+	if (cqr && READ_ONCE(cqr->collision)) {
+		WRITE_ONCE(cqr->collision, false);
+		return true;
+	}
 	spin_lock_irqsave(&block->format_lock, flags);
-	if (cqr->trkcount != atomic_read(&block->trkcount)) {
-		/*
-		 * The number of formatted tracks has changed after request
-		 * start and we can not tell if the current track was involved.
-		 * To avoid data corruption treat it as if the current track is
-		 * involved
-		 */
+	while (i < DASD_NR_FORMAT_ENTRIES &&
+	       READ_ONCE(device->format_entry[i].cqr))
+		i++;
+
+	if (i >= DASD_NR_FORMAT_ENTRIES) {
 		rc = true;
 		goto out;
 	}
+
 	list_for_each_entry(format, &block->format_list, list) {
-		if (format->track == to_format->track) {
+		if (!(end < format->start_trk || format->end_trk < start)) {
 			rc = true;
 			goto out;
 		}
 	}
+	to_format = &device->format_entry[i];
+	to_format->start_trk = start;
+	to_format->end_trk = end;
+	to_format->cqr = cqr;
 	list_add_tail(&to_format->list, &block->format_list);
+	*entry = to_format;
 
 out:
 	spin_unlock_irqrestore(&block->format_lock, flags);
@@ -3184,13 +3196,13 @@ static bool test_and_set_format_track(struct dasd_format_entry *to_format,
 }
 
 static void clear_format_track(struct dasd_format_entry *format,
-			      struct dasd_block *block)
+			       struct dasd_block *block)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&block->format_lock, flags);
-	atomic_inc(&block->trkcount);
 	list_del_init(&format->list);
+	format->cqr = NULL;
 	spin_unlock_irqrestore(&block->format_lock, flags);
 }
 
@@ -3212,8 +3224,8 @@ static struct dasd_ccw_req *
 dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 		     struct irb *irb)
 {
+	struct dasd_format_entry *format = NULL;
 	struct dasd_eckd_private *private;
-	struct dasd_format_entry *format;
 	struct format_data_t fdata;
 	unsigned int recs_per_trk;
 	struct dasd_ccw_req *fcqr;
@@ -3232,7 +3244,6 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 	private = base->private;
 	blksize = block->bp_block;
 	recs_per_trk = recs_per_track(&private->rdc_data, 0, blksize);
-	format = &startdev->format_entry;
 
 	first_trk = blk_rq_pos(req) >> block->s2b_shift;
 	sector_div(first_trk, recs_per_trk);
@@ -3249,9 +3260,9 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 			      curr_trk, first_trk, last_trk);
 		return ERR_PTR(-EINVAL);
 	}
-	format->track = curr_trk;
+
 	/* test if track is already in formatting by another thread */
-	if (test_and_set_format_track(format, cqr)) {
+	if (test_and_set_format_track(curr_trk, curr_trk, cqr, block, startdev, &format)) {
 		/* this is no real error so do not count down retries */
 		cqr->retries++;
 		return ERR_PTR(-EEXIST);
@@ -3263,17 +3274,28 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 	fdata.intensity = private->uses_cdl ? DASD_FMT_INT_COMPAT : 0;
 
 	rc = dasd_eckd_format_sanity_checks(base, &fdata);
-	if (rc)
+	if (rc) {
+		if (format)
+			clear_format_track(format, block);
 		return ERR_PTR(-EINVAL);
+	}
 
 	/*
 	 * We're building the request with PAV disabled as we're reusing
 	 * the former startdev.
 	 */
 	fcqr = dasd_eckd_build_format(base, startdev, &fdata, 0);
-	if (IS_ERR(fcqr))
+	if (IS_ERR(fcqr)) {
+		if (format)
+			clear_format_track(format, block);
 		return fcqr;
+	}
 
+	if (format) {
+		/* occupancy marker; the free-slot scan reads it with READ_ONCE */
+		WRITE_ONCE(format->cqr, fcqr);
+		fcqr->format = format;
+	}
 	fcqr->callback = dasd_eckd_ese_format_cb;
 	fcqr->callback_data = (void *) format;
 
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index cab16907ea5a..b342237b84de 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -545,9 +545,17 @@ struct dasd_profile {
 	spinlock_t lock;
 };
 
+/*
+ * concurrent ESE format ranges in flight; also caps a WRITE_FULL_TRACK's
+ * track count, which the LRE track bitmask limits to 16
+ */
+#define DASD_NR_FORMAT_ENTRIES	16
+
 struct dasd_format_entry {
 	struct list_head list;
-	sector_t track;
+	struct dasd_ccw_req *cqr;
+	sector_t start_trk;
+	sector_t end_trk;
 };
 
 struct dasd_device {
@@ -617,7 +625,7 @@ struct dasd_device {
 	struct dentry *debugfs_dentry;
 	struct dentry *hosts_dentry;
 	struct dasd_profile profile;
-	struct dasd_format_entry format_entry;
+	struct dasd_format_entry format_entry[DASD_NR_FORMAT_ENTRIES];
 	struct kset *paths_info;
 	struct dasd_copy_relation *copy;
 	unsigned long aq_mask;
@@ -834,6 +842,13 @@ static inline void *dasd_get_callback_data(struct dasd_ccw_req *cqr)
 	return cqr->callback_data;
 }
 
+static inline bool dasd_req_conflict(struct dasd_ccw_req *cqr1,
+				     struct dasd_ccw_req *cqr2)
+{
+	return !(cqr1->format->end_trk < cqr2->start_trk ||
+		 cqr2->end_trk < cqr1->format->start_trk);
+}
+
 /* externals in dasd.c */
 #define DASD_PROFILE_OFF	 0
 #define DASD_PROFILE_ON 	 1
-- 
2.53.0

Re: [PATCH 09/19] s390/dasd: Add range-based format-track collision detection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Removing the `trkcount` snapshot protection before the new collision detection becomes active leaves a window for data corruption via double-formatting.
- [High] Defensive check `if (!cqr->block)` introduces a guaranteed NULL pointer dereference on its error path.
--

--- Patch [9]: [PATCH 09/19] s390/dasd: Add range-based format-track collision detection ---
commit 1e9c1dce6a9ae6f45c013547038cfe3ae41af93a
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Add range-based format-track collision detection

Replace the single per-device format_entry slot with an array of 16
slots so multiple format requests can be in flight at once. This also
rewrites test_and_set_format_track() to check range overlaps and removes
the older trkcount snapshot logic.

> 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
> @@ -1402,13 +1402,6 @@ int dasd_start_IO(struct dasd_ccw_req *cqr)
>  		if (!cqr->lpm)
>  			cqr->lpm = dasd_path_get_opm(device);
>  	}
> -	/*
> -	 * remember the amount of formatted tracks to prevent double format on
> -	 * ESE devices
> -	 */
> -	if (cqr->block)
> -		cqr->trkcount = atomic_read(&cqr->block->trkcount);
> -
>  	if (cqr->cpmode == 1) {

[Severity: Critical]
Does removing this trkcount snapshot before the new collision detection is
active introduce a regression where concurrent threads could double-format 
tracks?

The commit message notes the new collision detection only becomes active
later in the series. By removing the older trkcount protection here,
it seems we might be introducing a bisection hole. For instance, if
thread B completes a format and removes the track from format_list, could
thread A's ERP routine see an old No Record Found error and proceed to
format the track again, erasing thread B's data?

[ ... ]

> @@ -2880,6 +2873,28 @@ static void __dasd_process_block_ccw_queue(struct dasd_block *block,
>  
>  static void dasd_return_cqr_cb(struct dasd_ccw_req *cqr, void *data)
>  {
> +	struct dasd_ccw_req *temp_cqr;
> +	struct dasd_block *block;
> +
> +	/* only format CQRs are candidates */
> +	if (!cqr->block || unlikely(!cqr->format))
> +		goto out;
> +
> +	block = cqr->block;

[ ... ]

> +out:
>  	dasd_schedule_block_bh(cqr->block);
>  }

[Severity: High]
If cqr->block is NULL, the if statement above evaluates to true and jumps
to the out label. Would this result in a regression with a guaranteed NULL
pointer dereference when dasd_schedule_block_bh() unconditionally accesses
it?

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

[PATCH 10/19] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

[Stefan Haberland]

prepare_itcw() builds the FCX prefix block (PFX + LRE) for track-mode
I/O. Extend it to handle DASD_ECKD_CCW_WRITE_FULL_TRACK.

WRITE_FULL_TRACK needs two extra bytes appended to the LRE for that
bitmask. The prefix block is a scratch buffer copied into the TCCB by
itcw_add_dcw(), so keep it on the stack (sized for the two extra bytes)
rather than allocating it: this runs in the writeback path and must not
depend on an allocation that can fail under memory pressure.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 64 +++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index be54c356dc24..ae122fbdca3e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -4388,11 +4388,13 @@ static int prepare_itcw(struct itcw *itcw,
 			unsigned int tlf,
 			unsigned int blk_per_trk)
 {
-	struct PFX_eckd_data pfxdata;
+	u8 pfxbuf[sizeof(struct PFX_eckd_data) + 2] __aligned(8);
+	struct PFX_eckd_data *pfxdata = (struct PFX_eckd_data *)pfxbuf;
 	struct dasd_eckd_private *basepriv, *startpriv;
 	struct DE_eckd_data *dedata;
 	struct LRE_eckd_data *lredata;
 	struct dcw *dcw;
+	int pfxsize;
 
 	u32 begcyl, endcyl;
 	u16 heads, beghead, endhead;
@@ -4402,26 +4404,31 @@ static int prepare_itcw(struct itcw *itcw,
 	int sector = 0;
 	int dn, d;
 
+	pfxsize = sizeof(struct PFX_eckd_data);
+	/* prefix + LRE extended data */
+	if (cmd == DASD_ECKD_CCW_WRITE_FULL_TRACK)
+		pfxsize += 2;
+
+	memset(pfxbuf, 0, pfxsize);
 
 	/* setup prefix data */
 	basepriv = basedev->private;
 	startpriv = startdev->private;
-	dedata = &pfxdata.define_extent;
-	lredata = &pfxdata.locate_record;
+	dedata = &pfxdata->define_extent;
+	lredata = &pfxdata->locate_record;
 
-	memset(&pfxdata, 0, sizeof(pfxdata));
-	pfxdata.format = 1; /* PFX with LRE */
-	pfxdata.base_address = basepriv->conf.ned->unit_addr;
-	pfxdata.base_lss = basepriv->conf.ned->ID;
-	pfxdata.validity.define_extent = 1;
+	pfxdata->format = 1; /* PFX with LRE */
+	pfxdata->base_address = basepriv->conf.ned->unit_addr;
+	pfxdata->base_lss = basepriv->conf.ned->ID;
+	pfxdata->validity.define_extent = 1;
 
 	/* private uid is kept up to date, conf_data may be outdated */
 	if (startpriv->uid.type == UA_BASE_PAV_ALIAS)
-		pfxdata.validity.verify_base = 1;
+		pfxdata->validity.verify_base = 1;
 
 	if (startpriv->uid.type == UA_HYPER_PAV_ALIAS) {
-		pfxdata.validity.verify_base = 1;
-		pfxdata.validity.hyper_pav = 1;
+		pfxdata->validity.verify_base = 1;
+		pfxdata->validity.hyper_pav = 1;
 	}
 
 	switch (cmd) {
@@ -4451,7 +4458,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:
@@ -4530,7 +4568,7 @@ static int prepare_itcw(struct itcw *itcw,
 	lredata->search_arg.record = rec_on_trk;
 
 	dcw = itcw_add_dcw(itcw, pfx_cmd, 0,
-		     &pfxdata, sizeof(pfxdata), total_data_size);
+			   pfxdata, pfxsize, total_data_size);
 	return PTR_ERR_OR_ZERO(dcw);
 }
 
-- 
2.53.0

Re: [PATCH 10/19] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

[sashiko-bot]

> prepare_itcw() builds the FCX prefix block (PFX + LRE) for track-mode
> I/O. Extend it to handle DASD_ECKD_CCW_WRITE_FULL_TRACK.
> 
> WRITE_FULL_TRACK needs two extra bytes appended to the LRE for that
> bitmask. The prefix block is a scratch buffer copied into the TCCB by
> itcw_add_dcw(), so keep it on the stack (sized for the two extra bytes)
> rather than allocating it: this runs in the writeback path and must not
> depend on an allocation that can fail under memory pressure.
> 
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

[Stefan Haberland]

Add the channel program builder for WRITE_FULL_TRACK requests, used
by dasd_eckd_ese_format() (next patch) to format and write a set of
tracks atomically and avoid the format cycle on ESE devices.

The program is an ITCW with a TIDAW list. Per track it emits an eckd_r0
header, an eckd_count + data pair for every record (pad records before
and after the caller's data window use device->nulldata, records in the
window point into the bio payload), and a terminating 0xFF pseudo-count
with TIDAW_FLAGS_INSERT_CBC. The descriptors come from the per-device
fill_chunks pool so they can be freed in bulk in __dasd_cleanup_cqr().

Add inline helpers crosses_page() and reserve_nocross(), to keep each
descriptor within one page since TIDAW addressing must not cross a page
boundary.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 344 ++++++++++++++++++++++++++++++++-
 1 file changed, 343 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index ae122fbdca3e..8379c8a40382 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -124,6 +124,14 @@ static int prepare_itcw(struct itcw *, unsigned int, unsigned int, int,
 			unsigned int, unsigned int);
 static int dasd_eckd_query_pprc_status(struct dasd_device *,
 				       struct dasd_pprc_data_sc4 *);
+static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *,
+								  struct dasd_block *,
+								  struct request *,
+								  sector_t, sector_t,
+								  sector_t, sector_t,
+								  unsigned int, unsigned int,
+								  unsigned int, unsigned int,
+								  struct dasd_ccw_req *);
 
 /* initial attempt at a probe function. this can be simplified once
  * the other detection code is gone */
@@ -205,7 +213,7 @@ static void set_ch_t(struct ch_t *geo, __u32 cyl, __u8 head)
 	geo->head |= head;
 }
 
-static __maybe_unused void set_chr_t(void *addr, __u32 cyl, __u8 head, __u8 record)
+static void set_chr_t(void *addr, __u32 cyl, __u8 head, __u8 record)
 {
 	struct chr_t *geo = addr;
 
@@ -4745,6 +4753,340 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
 	return ERR_PTR(ret);
 }
 
+static __always_inline bool crosses_page(const void *addr, size_t len)
+{
+	return len && (offset_in_page(addr) + len > PAGE_SIZE);
+}
+
+static __always_inline void *reserve_nocross(char **p, size_t *space, size_t len)
+{
+	size_t pad = crosses_page(*p, len) ? PAGE_SIZE - offset_in_page(*p) : 0;
+	void *ret;
+
+	if (*space < pad + len)
+		return NULL;	/* out of space */
+
+	*p += pad;
+	*space -= pad;
+	ret = *p;
+	*p += len;
+	*space -= len;
+	return ret;
+}
+
+/*
+ * Helpers for dasd_eckd_build_cp_tpm_writefulltrack(): append the TIDAWs for
+ * one track-image element (R0 header, a count + data record, or the trailing
+ * pseudo track end count) to the itcw. Return the last TIDAW, or NULL on failure.
+ */
+static struct tidaw *add_track_r0(struct itcw *itcw, char **fill,
+				  size_t *fillsize, u32 cyl, u16 head)
+{
+	struct tidaw *tidaw;
+	struct eckd_r0 *r0;
+
+	r0 = reserve_nocross(fill, fillsize, sizeof(*r0));
+	if (WARN_ON_ONCE(!r0))
+		return NULL;
+	set_chr_t(r0, cyl, head, 0);
+	r0->count.dl = 8;
+	tidaw = itcw_add_tidaw(itcw, 0, r0, sizeof(*r0));
+	return IS_ERR_OR_NULL(tidaw) ? NULL : tidaw;
+}
+
+static struct tidaw *add_track_record(struct itcw *itcw, char **fill,
+				      size_t *fillsize, u32 cyl, u16 head,
+				      u8 rec, void *data, u32 dl)
+{
+	struct eckd_count *count;
+	struct tidaw *tidaw;
+
+	count = reserve_nocross(fill, fillsize, sizeof(*count));
+	if (WARN_ON_ONCE(!count))
+		return NULL;
+	set_chr_t(count, cyl, head, rec);
+	count->dl = dl;
+	tidaw = itcw_add_tidaw(itcw, 0, count, sizeof(*count));
+	if (IS_ERR_OR_NULL(tidaw))
+		return NULL;
+	tidaw = itcw_add_tidaw(itcw, 0, data, dl);
+	return IS_ERR_OR_NULL(tidaw) ? NULL : tidaw;
+}
+
+static struct tidaw *add_track_end(struct itcw *itcw, char **fill,
+				   size_t *fillsize)
+{
+	struct eckd_count *count;
+	struct tidaw *tidaw;
+
+	count = reserve_nocross(fill, fillsize, sizeof(*count));
+	if (WARN_ON_ONCE(!count))
+		return NULL;
+	count->cyl = 0xffff;
+	count->head = 0xffff;
+	count->dl = 0xffff;
+	count->record = 0xff;
+	count->kl = 0xff;
+	tidaw = itcw_add_tidaw(itcw, TIDAW_FLAGS_INSERT_CBC, count, sizeof(*count));
+	return IS_ERR_OR_NULL(tidaw) ? NULL : tidaw;
+}
+
+static __maybe_unused struct dasd_ccw_req *
+dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
+				      struct dasd_block *block,
+				      struct request *req,
+				      sector_t first_rec,
+				      sector_t last_rec,
+				      sector_t first_trk,
+				      sector_t last_trk,
+				      unsigned int first_offs,
+				      unsigned int last_offs,
+				      unsigned int blk_per_trk,
+				      unsigned int blksize,
+				      struct dasd_ccw_req *ocqr)
+{
+	struct dasd_eckd_private *private = block->base->private;
+	unsigned int seg_len, part_len, len_to_track_end;
+	unsigned int count, count_to_trk_end, offs;
+	unsigned int trkcount, ctidaw, tlf;
+	int itcw_op, rec_count, datasize;
+	struct tidaw *last_tidaw = NULL;
+	sector_t recid, trkid, curr_trk;
+	unsigned char cmd, new_track;
+	struct dasd_device *basedev;
+	size_t itcw_size, fillsize;
+	struct dasd_ccw_req *cqr;
+	struct req_iterator iter;
+	char *dst, *filldata;
+	unsigned long flags;
+	struct itcw *itcw;
+	struct bio_vec bv;
+	int ret = -EINVAL;
+	void *nullrecord;
+	u16 heads, head;
+	u32 cyl;
+	u8 rec;
+
+	basedev = block->base;
+	cmd = DASD_ECKD_CCW_WRITE_FULL_TRACK;
+	itcw_op = ITCW_OP_WRITE;
+
+	/*
+	 * trackbased I/O needs address all memory via TIDAWs,
+	 * not just for 64 bit addresses. This allows us to map
+	 * each segment directly to one tidaw.
+	 * In the case of write requests, additional tidaws may
+	 * be needed when a segment crosses a track boundary.
+	 * Per track we emit one R0 tidaw, two tidaws per record (count field
+	 * plus data - a record never crosses a track or page boundary, as
+	 * part_len is clamped to both blksize and the track end), and one track
+	 * end tidaw: 2 * blk_per_trk + 2.
+	 * Round the +2 up to blk_per_trk-independent headroom via 2 * (blk_per_trk + 2).
+	 */
+	trkcount = last_trk - first_trk + 1;
+	ctidaw = trkcount * 2 * (blk_per_trk + 2);
+
+	/*
+	 * build_cp (ocqr == NULL): the request owns its CCW program - block in
+	 * the pdu, ITCW in ccw_chunks. ese_format (ocqr != NULL): the failing
+	 * origin still owns its pdu, so take the replacement from ese_chunks.
+	 */
+	itcw_size = itcw_calc_size(0, ctidaw, 0);
+	if (ocqr)
+		cqr = dasd_fmalloc_request(DASD_ECKD_MAGIC, 0, itcw_size, startdev);
+	else
+		cqr = dasd_smalloc_request(DASD_ECKD_MAGIC, 0, itcw_size, startdev,
+					   blk_mq_rq_to_pdu(req));
+	if (IS_ERR(cqr))
+		return cqr;
+	fillsize = trkcount * (sizeof(struct eckd_r0) +
+			       (sizeof(struct eckd_count) * (blk_per_trk + 2)));
+	/*
+	 * reserve_nocross() pads elements away from page boundaries and draws
+	 * that padding from fillsize; budget one element per page the buffer
+	 * may span so it never runs short.
+	 */
+	fillsize += (fillsize / PAGE_SIZE + 1) * sizeof(struct eckd_r0);
+	spin_lock_irqsave(&startdev->mem_lock, flags);
+	filldata  = dasd_alloc_chunk(&startdev->fill_chunks, fillsize);
+	spin_unlock_irqrestore(&startdev->mem_lock, flags);
+	if (!filldata) {
+		ret = -ENOMEM;
+		goto out_error;
+	}
+	memset(filldata, 0, fillsize);
+	cqr->filldata = filldata;
+
+	nullrecord = startdev->nulldata;
+
+	/* count + data for each record, plus r0 and the pseudo count */
+	tlf = blk_per_trk * (blksize + sizeof(struct eckd_count));
+	tlf += sizeof(struct eckd_r0) + sizeof(struct eckd_count);
+
+	itcw = itcw_init(cqr->data, itcw_size, itcw_op, 0, ctidaw, 0);
+	if (IS_ERR(itcw)) {
+		ret = -EINVAL;
+		goto out_error;
+	}
+	cqr->cpaddr = itcw_get_tcw(itcw);
+	datasize = trkcount * tlf;
+	if (prepare_itcw(itcw, first_trk, last_trk,
+			 cmd, basedev, startdev,
+			 0,
+			 trkcount, blksize,
+			 datasize,
+			 tlf,
+			 blk_per_trk) == -EAGAIN) {
+		/* Clock not in sync and XRC is enabled.
+		 * Try again later.
+		 */
+		ret = -EAGAIN;
+		goto out_error;
+	}
+	heads = private->rdc_data.trk_per_cyl;
+	/*
+	 * A tidaw can address 4k of memory, but must not cross page boundaries
+	 * We can let the block layer handle this by setting seg_boundary_mask
+	 * to page boundaries and max_segment_size to page size when setting up
+	 * the request queue.
+	 */
+	curr_trk = first_trk;
+	recid = first_rec;
+	trkid = recid;
+	offs = sector_div(trkid, blk_per_trk);
+	count = blk_per_trk;
+	len_to_track_end = count * blksize;
+	recid += count - first_offs;
+	new_track = 0;
+
+	/* the R0 header of the first track */
+	cyl = curr_trk / heads;
+	head = curr_trk % heads;
+	last_tidaw = add_track_r0(itcw, &filldata, &fillsize, cyl, head);
+	if (!last_tidaw)
+		goto out_error;
+
+	/* empty records before the first data record */
+	for (int i = 1; i <= first_offs; i++) {
+		len_to_track_end -= blksize;
+		last_tidaw = add_track_record(itcw, &filldata, &fillsize,
+					      cyl, head, i, nullrecord, blksize);
+		if (!last_tidaw)
+			goto out_error;
+	}
+
+	/* process data records */
+	rec = first_offs + 1;
+	rec_count = 0;
+	rq_for_each_segment(bv, req, iter) {
+		dst = bvec_virt(&bv);
+		seg_len = bv.bv_len;
+		while (seg_len) {
+			if (new_track) {
+				trkid = recid;
+				offs = sector_div(trkid, blk_per_trk);
+				count_to_trk_end = blk_per_trk - offs;
+				count = min((last_rec - recid + 1),
+					    (sector_t)count_to_trk_end);
+				/*
+				 * Size to the physical track end: a short last
+				 * track is padded in out_skip, so the track-end
+				 * marker must not be emitted early here.
+				 */
+				len_to_track_end = count_to_trk_end * blksize;
+				recid += count;
+				new_track = 0;
+				/* the R0 header of the next track */
+				cyl = curr_trk / heads;
+				head = curr_trk % heads;
+				last_tidaw = add_track_r0(itcw, &filldata,
+							  &fillsize, cyl, head);
+				if (!last_tidaw)
+					goto out_error;
+				rec = 1;
+			}
+			/*
+			 * One count + data record per block: a bvec segment can
+			 * be up to a page, so clamp to blksize - otherwise the
+			 * count field would describe one oversized record instead
+			 * of several blksize ones for sub-page block sizes.
+			 */
+			part_len = min(seg_len, len_to_track_end);
+			part_len = min(part_len, blksize);
+			seg_len -= part_len;
+			len_to_track_end -= part_len;
+			/*
+			 * This block ends the track; the next one starts a new
+			 * track. The track-end marker emitted below carries the
+			 * CBC flag.
+			 */
+			if (!len_to_track_end)
+				new_track = 1;
+
+			last_tidaw = add_track_record(itcw, &filldata, &fillsize,
+						      cyl, head, rec, dst, part_len);
+			if (!last_tidaw)
+				goto out_error;
+
+			if (new_track) {
+				/* add track end marker */
+				last_tidaw = add_track_end(itcw, &filldata,
+							   &fillsize);
+				if (!last_tidaw)
+					goto out_error;
+				curr_trk++;
+			}
+			rec++;
+			dst += part_len;
+			rec_count++;
+			if (rec_count >= (last_rec - first_rec + 1))
+				goto out_skip;
+		}
+	}
+
+out_skip:
+	new_track = 0;
+	/* empty records after the last data record */
+	for (int i = last_offs + 2; i <= blk_per_trk; i++) {
+		len_to_track_end -= blksize;
+		last_tidaw = add_track_record(itcw, &filldata, &fillsize,
+					      cyl, head, i, nullrecord, blksize);
+		if (!last_tidaw)
+			goto out_error;
+		new_track = 1;
+	}
+
+	/* add track end marker */
+	if (new_track) {
+		last_tidaw = add_track_end(itcw, &filldata, &fillsize);
+		if (!last_tidaw)
+			goto out_error;
+	}
+
+	last_tidaw->flags |= TIDAW_FLAGS_LAST;
+	last_tidaw->flags &= ~TIDAW_FLAGS_INSERT_CBC;
+	itcw_finalize(itcw);
+
+	if (blk_noretry_request(req) ||
+	    block->base->features & DASD_FEATURE_FAILFAST)
+		set_bit(DASD_CQR_FLAGS_FAILFAST, &cqr->flags);
+	cqr->cpmode = 1;
+	cqr->startdev = startdev;
+	cqr->memdev = startdev;
+	cqr->block = block;
+	cqr->expires = startdev->default_expires * HZ;	/* default 5 minutes */
+	cqr->lpm = dasd_path_get_ppm(startdev);
+	cqr->retries = startdev->default_retries;
+	cqr->buildclk = get_tod_clock();
+	cqr->status = DASD_CQR_FILLED;
+
+	return cqr;
+out_error:
+	/* dasd_sfree_request frees from the right pool via cqr->mem_chunk */
+	dasd_sfree_request(cqr, startdev);
+	return ERR_PTR(ret);
+}
+
 static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 					       struct dasd_block *block,
 					       struct request *req)
-- 
2.53.0

Re: [PATCH 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Split memory segments (bvecs) result in truncated physical records, corrupting the track format.
- [High] Missing `bio_has_data()` guard before iterating over request segments.

Pre-existing issues:
- [High] Negative bitwise shift and truncated extended parameter length when formatting >16 tracks.
--

--- Patch [11]: [PATCH 11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 756c6129bbfe82549e28cc85e3b42100ba732cd0
Author: Stefan Haberland <sth@linux.ibm.com>
Subject: s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

Adds the channel program builder for WRITE_FULL_TRACK requests, used
by dasd_eckd_ese_format() to format and write a set of tracks
atomically.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index ae122fbdca3ed..8379c8a40382f 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> +static __maybe_unused struct dasd_ccw_req *
> +dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
> +				      struct dasd_block *block,
> +				      struct request *req,
[ ... ]
> +	if (prepare_itcw(itcw, first_trk, last_trk,
> +			 cmd, basedev, startdev,
> +			 0,
> +			 trkcount, blksize,
> +			 datasize,
> +			 tlf,
> +			 blk_per_trk) == -EAGAIN) {

[Severity: High]
This is a pre-existing issue in prepare_itcw(), but formatting more than
16 tracks could cause undefined behavior here.

When DASD_ECKD_CCW_WRITE_FULL_TRACK is processed and trkcount (passed as
count to prepare_itcw) exceeds 16, the expression (16 - count) inside
prepare_itcw() evaluates to a negative number:

drivers/s390/block/dasd_eckd.c:prepare_itcw() {
    ...
    if (count > 8) {
        lredata->extended_parameter[0] = 0xFF;
        lredata->extended_parameter[1] = 0xFF;
        lredata->extended_parameter[1] <<= (16 - count);
    ...
}

Does shifting left by a negative amount cause a corrupted extended parameter
bitmask?

Additionally, since extended_parameter_length is hardcoded to 2 bytes,
requests spanning more than 16 tracks may truncate the track bitmask.

> +		/* Clock not in sync and XRC is enabled.
> +		 * Try again later.
> +		 */
> +		ret = -EAGAIN;
> +		goto out_error;
> +	}
[ ... ]
> +	/* process data records */
> +	rec = first_offs + 1;
> +	rec_count = 0;
> +	rq_for_each_segment(bv, req, iter) {

[Severity: High]
Could this cause a NULL pointer dereference if a data-less operation (like
REQ_OP_WRITE_ZEROES) is routed here?

The block subsystem guidelines mention that accesses to bio data fields
require a bio_has_data() guard before iteration, as operations like discard
or write zeroes lack data buffers.

> +		dst = bvec_virt(&bv);
> +		seg_len = bv.bv_len;
> +		while (seg_len) {
[ ... ]
> +			/*
> +			 * This block ends the track; the next one starts a new
> +			 * track. The track-end marker emitted below carries the
> +			 * CBC flag.
> +			 */
> +			if (!len_to_track_end)
> +				new_track = 1;
> +
> +			last_tidaw = add_track_record(itcw, &filldata, &fillsize,
> +						      cyl, head, rec, dst, part_len);

[Severity: Critical]
If a logical block is fragmented across memory boundaries (for example, a
4096-byte block split into two 2048-byte segments due to page boundary
crossing), does this erroneously emit multiple eckd_count headers with
truncated lengths?

Instead of chaining TIDAWs for a single physical record, calling
add_track_record() for each segment piece might advance the record
counter twice and permanently corrupt the track format.

> +			if (!last_tidaw)
> +				goto out_error;
> +

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

[PATCH 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

[Stefan Haberland]

Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler.

dasd_eckd_ese_format() now returns void (matching the revised discipline
hook): it computes the failing track/record range, trims a partially
covered last track when several tracks are involved (the block layer
re-issues the remainder), claims the range with
test_and_set_format_track(), builds a writefulltrack CQR, copies
callback_data/proc_bytes from the origin, and stages it on
block->ese_staging. The origin CQR is set to DASD_CQR_ABORT so
__dasd_process_cqr() retires it without the normal completion.

Drop dasd_eckd_ese_format_cb(); the format-entry slot is now released by
dasd_eckd_free_alias_cp() via clear_format_track() when the CQR is freed.

dasd_int_handler() calls the void hook directly and, for writefulltrack
CQRs (cqr->filldata set), returns DASD_CQR_ERROR instead of looping on
the NRF.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c          |  59 ++++++---
 drivers/s390/block/dasd_3990_erp.c |   1 +
 drivers/s390/block/dasd_eckd.c     | 202 ++++++++++++++++++++---------
 drivers/s390/block/dasd_erp.c      |   8 +-
 drivers/s390/block/dasd_int.h      |   3 +-
 5 files changed, 193 insertions(+), 80 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 243dec4b4484..18f5f097d2b3 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -1618,7 +1618,7 @@ static int dasd_ese_oos_cond(u8 *sense)
 void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
 		      struct irb *irb)
 {
-	struct dasd_ccw_req *cqr, *next, *fcqr;
+	struct dasd_ccw_req *cqr, *next;
 	struct dasd_device *device;
 	unsigned long now;
 	int nrf_suppressed = 0;
@@ -1740,26 +1740,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;
 	}
 
 	/* Check for clear pending */
@@ -2733,6 +2730,13 @@ static void __dasd_process_erp(struct dasd_device *device,
 
 	if (cqr->status == DASD_CQR_DONE)
 		DBF_DEV_EVENT(DBF_NOTICE, device, "%s", "ERP successful");
+	else if (cqr->status == DASD_CQR_ABORTED)
+		/*
+		 * ESE format aborts the request and replaces it with a format
+		 * CQR - this is not an ERP failure.
+		 */
+		DBF_DEV_EVENT(DBF_NOTICE, device, "%s",
+			      "ERP request aborted, replaced by ESE format");
 	else
 		dev_err(&device->cdev->dev, "ERP failed for the DASD\n");
 	erp_fn = device->discipline->erp_postaction(cqr);
@@ -2779,6 +2783,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;
 	}
 
 	/*
@@ -3185,6 +3192,13 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 		}
 		goto out;
 	}
+	if (!cqr) {
+		/* build_cp may collapse a non-transient build error to NULL */
+		DBF_DEV_EVENT(DBF_ERR, basedev,
+			      "CCW creation returned NULL on request %p", req);
+		rc = BLK_STS_IOERR;
+		goto out;
+	}
 	/*
 	 *  Note: callback is set to dasd_return_cqr_cb in
 	 * __dasd_block_start_head to cover erp requests as well
@@ -3978,6 +3992,19 @@ int dasd_generic_requeue_all_requests(struct dasd_device *device)
 			 */
 			goto restart_cb;
 		}
+		/*
+		 * An aborted request was replaced by a full-track write and is
+		 * retired by that replacement; do not requeue it, just release
+		 * it (mirrors the DASD_CQR_ABORTED handling in
+		 * __dasd_cleanup_cqr()).
+		 */
+		if (cqr->status == DASD_CQR_ABORTED) {
+			struct request *req = cqr->callback_data;
+
+			list_del_init(&cqr->blocklist);
+			cqr->block->base->discipline->free_cp(cqr, req);
+			continue;
+		}
 		_dasd_requeue_request(cqr);
 		list_del_init(&cqr->blocklist);
 		cqr->block->base->discipline->free_cp(
diff --git a/drivers/s390/block/dasd_3990_erp.c b/drivers/s390/block/dasd_3990_erp.c
index d0aa267462c5..736459477c19 100644
--- a/drivers/s390/block/dasd_3990_erp.c
+++ b/drivers/s390/block/dasd_3990_erp.c
@@ -2400,6 +2400,7 @@ static struct dasd_ccw_req *dasd_3990_erp_add_erp(struct dasd_ccw_req *cqr)
 	erp->startdev = device;
 	erp->memdev   = device;
 	erp->block    = cqr->block;
+	erp->filldata = cqr->filldata;
 	erp->magic    = cqr->magic;
 	erp->expires  = cqr->expires;
 	erp->retries  = device->default_retries;
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 8379c8a40382..da488c0775fc 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3214,36 +3214,24 @@ static void clear_format_track(struct dasd_format_entry *format,
 	spin_unlock_irqrestore(&block->format_lock, flags);
 }
 
-/*
- * Callback function to free ESE format requests.
- */
-static void dasd_eckd_ese_format_cb(struct dasd_ccw_req *cqr, void *data)
-{
-	struct dasd_device *device = cqr->startdev;
-	struct dasd_eckd_private *private = device->private;
-	struct dasd_format_entry *format = data;
-
-	clear_format_track(format, cqr->basedev->block);
-	private->count--;
-	dasd_ffree_request(cqr, device);
-}
-
-static struct dasd_ccw_req *
-dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
-		     struct irb *irb)
+static void dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
+				 struct irb *irb)
 {
 	struct dasd_format_entry *format = NULL;
+	unsigned int first_offs, last_offs;
 	struct dasd_eckd_private *private;
-	struct format_data_t fdata;
-	unsigned int recs_per_trk;
+	struct dasd_ccw_req *base_cqr;
+	sector_t first_rec, last_rec;
+	sector_t first_trk, last_trk;
+	unsigned int proc_bytes = 0;
 	struct dasd_ccw_req *fcqr;
+	unsigned int recs_per_trk;
 	struct dasd_device *base;
 	struct dasd_block *block;
 	unsigned int blksize;
 	struct request *req;
-	sector_t first_trk;
-	sector_t last_trk;
 	sector_t curr_trk;
+	unsigned int diff;
 	int rc;
 
 	req = dasd_get_callback_data(cqr);
@@ -3253,50 +3241,94 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 	blksize = block->bp_block;
 	recs_per_trk = recs_per_track(&private->rdc_data, 0, blksize);
 
-	first_trk = blk_rq_pos(req) >> block->s2b_shift;
-	sector_div(first_trk, recs_per_trk);
-	last_trk =
-		(blk_rq_pos(req) + blk_rq_sectors(req) - 1) >> block->s2b_shift;
-	sector_div(last_trk, recs_per_trk);
-	rc = dasd_eckd_track_from_irb(irb, base, &curr_trk);
-	if (rc)
-		return ERR_PTR(rc);
+	/* Calculate record id of first and last block. */
+	first_rec = blk_rq_pos(req) >> block->s2b_shift;
+	first_trk = first_rec;
+	first_offs = sector_div(first_trk, recs_per_trk);
+	last_rec = (blk_rq_pos(req) + blk_rq_sectors(req) - 1) >> block->s2b_shift;
+	last_trk = last_rec;
+	last_offs = sector_div(last_trk, recs_per_trk);
 
+	/*
+	 * detect if some data has already been processed and the unformatted track is
+	 * within the request.
+	 * If so, finish the request first with the already processed bytes and let the
+	 * blocklayer only redrive unformatted part.
+	 * With this we ensure that there is no overlap of existing data with unformatted
+	 * zero blocks
+	 */
+	rc = dasd_eckd_track_from_irb(irb, base, &curr_trk);
+	if (rc) {
+		/* sense data could not be parsed - this will not resolve by retrying */
+		cqr->status = DASD_CQR_ERROR;
+		goto out;
+	}
+	if (curr_trk >= (sector_t)private->real_cyl * private->rdc_data.trk_per_cyl) {
+		DBF_DEV_EVENT(DBF_WARNING, startdev,
+			      "ESE error track %llu exceeds device geometry\n",
+			      curr_trk);
+		cqr->status = DASD_CQR_ERROR;
+		goto out;
+	}
 	if (curr_trk < first_trk || curr_trk > last_trk) {
 		DBF_DEV_EVENT(DBF_WARNING, startdev,
 			      "ESE error track %llu not within range %llu - %llu\n",
 			      curr_trk, first_trk, last_trk);
-		return ERR_PTR(-EINVAL);
-	}
-
-	/* test if track is already in formatting by another thread */
-	if (test_and_set_format_track(curr_trk, curr_trk, cqr, block, startdev, &format)) {
-		/* this is no real error so do not count down retries */
-		cqr->retries++;
-		return ERR_PTR(-EEXIST);
+		cqr->status = DASD_CQR_ERROR;
+		goto out;
 	}
-
-	fdata.start_unit = curr_trk;
-	fdata.stop_unit = curr_trk;
-	fdata.blksize = blksize;
-	fdata.intensity = private->uses_cdl ? DASD_FMT_INT_COMPAT : 0;
-
-	rc = dasd_eckd_format_sanity_checks(base, &fdata);
-	if (rc) {
-		if (format)
-			clear_format_track(format, block);
-		return ERR_PTR(-EINVAL);
+	if (curr_trk != first_trk) {
+		proc_bytes = ((curr_trk - first_trk) * recs_per_trk - first_offs) * blksize;
+		cqr->proc_bytes = proc_bytes;
+		cqr->status = DASD_CQR_SUCCESS;
+		cqr->stopclk = get_tod_clock();
+		goto out;
 	}
 
 	/*
-	 * We're building the request with PAV disabled as we're reusing
-	 * the former startdev.
+	 * If there are multiple tracks to be format-written, we can not write
+	 * the partial last track since we do not know if it is already formatted
+	 * or not so skip the partial last track for now. Return the partial
+	 * completion to blocklayer and let it redo the remainder
 	 */
-	fcqr = dasd_eckd_build_format(base, startdev, &fdata, 0);
+	if (first_trk != last_trk && last_offs + 1 < recs_per_trk) {
+		diff = last_offs + 1;
+		last_rec = last_rec - diff;
+		last_trk = last_rec;
+		last_offs = sector_div(last_trk, recs_per_trk);
+		proc_bytes = (last_rec - first_rec + 1) * blksize;
+	}
+	if (first_offs > 0 || last_offs + 1 < recs_per_trk) {
+		/* test if tracks are already in formatting by another thread */
+		if (test_and_set_format_track(first_trk, last_trk, cqr,
+					      cqr->block, cqr->startdev, &format)) {
+			/* this is no real error so do not count down retries */
+			cqr->retries++;
+			goto out_retry;
+		}
+	}
+
+	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);
 	if (IS_ERR(fcqr)) {
 		if (format)
-			clear_format_track(format, block);
-		return fcqr;
+			clear_format_track(format, cqr->block);
+		if (PTR_ERR(fcqr) == -EINVAL) {
+			/* permanent build failure - fail instead of retrying */
+			cqr->status = DASD_CQR_ERROR;
+			goto out;
+		}
+		/*
+		 * 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;
 	}
 
 	if (format) {
@@ -3304,10 +3336,44 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 		WRITE_ONCE(format->cqr, fcqr);
 		fcqr->format = format;
 	}
-	fcqr->callback = dasd_eckd_ese_format_cb;
-	fcqr->callback_data = (void *) format;
 
-	return fcqr;
+	/*
+	 * cqr may be an ERP request; dq and the owning request are only set on
+	 * the base request at the end of the ERP chain, so copy from there.
+	 */
+	base_cqr = cqr;
+	while (base_cqr->refers)
+		base_cqr = base_cqr->refers;
+	fcqr->dq = base_cqr->dq;
+	fcqr->callback_data = base_cqr->callback_data;
+	if (proc_bytes)
+		fcqr->proc_bytes = proc_bytes;
+	fcqr->status = DASD_CQR_FILLED;
+	((struct dasd_eckd_private *)fcqr->memdev->private)->count++;
+	/*
+	 * stage under ese_lock; dasd_block_tasklet splices it into ccw_queue.
+	 * Direct enqueue here would invert queue_lock / ccwdev_lock.
+	 */
+	spin_lock(&block->ese_lock);
+	list_add(&fcqr->blocklist, &block->ese_staging);
+	spin_unlock(&block->ese_lock);
+	/* mark origin CQR as aborted; ccwdev_lock is held by the IRQ handler */
+	cqr->status = DASD_CQR_ABORT;
+	goto out;
+
+out_retry:
+	/*
+	 * If we can't format now, let the request go
+	 * one extra round. Maybe we can format later.
+	 * re-queue at the end to let potential format collision finish first
+	 */
+	list_move_tail(&cqr->devlist, &cqr->startdev->ccw_queue);
+	cqr->status = DASD_CQR_QUEUED;
+out:
+	dasd_device_clear_timer(startdev);
+	dasd_schedule_block_bh(block);
+	dasd_schedule_device_bh(startdev);
+	return;
 }
 
 /*
@@ -4831,7 +4897,7 @@ static struct tidaw *add_track_end(struct itcw *itcw, char **fill,
 	return IS_ERR_OR_NULL(tidaw) ? NULL : tidaw;
 }
 
-static __maybe_unused struct dasd_ccw_req *
+static struct dasd_ccw_req *
 dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
 				      struct dasd_block *block,
 				      struct request *req,
@@ -5122,7 +5188,7 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 
 	fcx_multitrack = private->features.feature[40] & 0x20;
 	data_size = blk_rq_bytes(req);
-	if (data_size % blksize)
+	if (data_size % blksize || data_size == 0)
 		return ERR_PTR(-EINVAL);
 	/* tpm write request add CBC data on each track boundary */
 	if (rq_data_dir(req) == WRITE)
@@ -5164,6 +5230,11 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 						    first_trk, last_trk,
 						    first_offs, last_offs,
 						    blk_per_trk, blksize);
+
+	if (!IS_ERR(cqr)) {
+		cqr->start_trk = first_trk;
+		cqr->end_trk = last_trk;
+	}
 	return cqr;
 }
 
@@ -5331,7 +5402,17 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
 	sector_t recid;
 	int status;
 
-	if (!dasd_page_cache)
+	/*
+	 * A format-aborted request finished nothing - its replacement
+	 * completes the block request - so report ABORTED instead of DONE,
+	 * but still release its bounce buffers like any other request.
+	 */
+	if (cqr->status == DASD_CQR_ABORTED)
+		status = DASD_CQR_ABORTED;
+	else
+		status = cqr->status == DASD_CQR_DONE;
+	/* transport mode has no dasd_page_cache bounce buffers to release */
+	if (!dasd_page_cache || cqr->cpmode)
 		goto out;
 	private = cqr->block->base->private;
 	blksize = cqr->block->bp_block;
@@ -5366,7 +5447,6 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
 		}
 	}
 out:
-	status = cqr->status == DASD_CQR_DONE;
 	dasd_sfree_request(cqr, cqr->memdev);
 	return status;
 }
@@ -5443,6 +5523,8 @@ static int dasd_eckd_free_alias_cp(struct dasd_ccw_req *cqr,
 	private = cqr->memdev->private;
 	private->count--;
 	spin_unlock_irqrestore(get_ccwdev_lock(cqr->memdev->cdev), flags);
+	if (cqr->format)
+		clear_format_track(cqr->format, cqr->block);
 	return dasd_eckd_free_cp(cqr, req);
 }
 
diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
index 468f0b2cc342..05d5366484d7 100644
--- a/drivers/s390/block/dasd_erp.c
+++ b/drivers/s390/block/dasd_erp.c
@@ -120,7 +120,7 @@ dasd_default_erp_action(struct dasd_ccw_req *cqr)
  */
 struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 {
-	int success;
+	int success, aborted;
 	unsigned long startclk, stopclk;
 	struct dasd_device *startdev;
 	unsigned int proc_bytes;
@@ -128,6 +128,7 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 	BUG_ON(cqr->refers == NULL || cqr->function == NULL);
 
 	success = cqr->status == DASD_CQR_DONE;
+	aborted = cqr->status == DASD_CQR_ABORTED;
 	startclk = cqr->startclk;
 	stopclk = cqr->stopclk;
 	startdev = cqr->startdev;
@@ -150,7 +151,10 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
 	cqr->stopclk = stopclk;
 	cqr->startdev = startdev;
 	cqr->proc_bytes = proc_bytes;
-	if (success)
+	if (aborted)
+		/* base request is owned by the ESE format replacement CQR */
+		cqr->status = DASD_CQR_ABORTED;
+	else if (success)
 		cqr->status = DASD_CQR_DONE;
 	else {
 		cqr->status = DASD_CQR_FAILED;
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index b342237b84de..e1ffa20db10a 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -413,8 +413,7 @@ struct dasd_discipline {
 	int (*ext_pool_warn_thrshld)(struct dasd_device *);
 	int (*ext_pool_oos)(struct dasd_device *);
 	int (*ext_pool_exhaust)(struct dasd_device *, struct dasd_ccw_req *);
-	struct dasd_ccw_req *(*ese_format)(struct dasd_device *,
-					   struct dasd_ccw_req *, struct irb *);
+	void (*ese_format)(struct dasd_device *, struct dasd_ccw_req *, struct irb *);
 	int (*ese_read)(struct dasd_ccw_req *, struct irb *);
 	int (*pprc_status)(struct dasd_device *, struct	dasd_pprc_data_sc4 *);
 	bool (*pprc_enabled)(struct dasd_device *);
-- 
2.53.0

Re: [PATCH 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Full-track format requests bypass `test_and_set_format_track()` locking, leading to TOCTOU data corruption from concurrent partial-track writes.
- [High] Setting cqr->callback = NULL for aborted requests during teardown overwrites the flush queue wakeup callback, causing an indefinite deadlock during device offline.
--

--- Patch [12]: [PATCH 12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit c79b7434acd3a79782b3c648eaa1de078edf01da
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Use WRITE_FULL_TRACK in ESE format handler

Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8379c8a40382f..da488c0775fcd 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -3253,50 +3241,94 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
> [ ... ]
> +	if (first_trk != last_trk && last_offs + 1 < recs_per_trk) {
> +		diff = last_offs + 1;
> +		last_rec = last_rec - diff;
> +		last_trk = last_rec;
> +		last_offs = sector_div(last_trk, recs_per_trk);
> +		proc_bytes = (last_rec - first_rec + 1) * blksize;
> +	}
> +	if (first_offs > 0 || last_offs + 1 < recs_per_trk) {
> +		/* test if tracks are already in formatting by another thread */
> +		if (test_and_set_format_track(first_trk, last_trk, cqr,
> +					      cqr->block, cqr->startdev, &format)) {

[Severity: Critical]
Does bypassing test_and_set_format_track() for full-track requests allow
concurrent partial-track writes to the same track to proceed without
detecting a conflict?

If a partial-track format CCW chain executes after the full-track write, its
zero-padding could silently overwrite the valid data just written by the
full-track write.

> +			/* this is no real error so do not count down retries */
> +			cqr->retries++;
> +			goto out_retry;
> +		}
> +	}
> +
> [ ... ]
> @@ -3304,10 +3336,44 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
> [ ... ]
> +	/*
> +	 * stage under ese_lock; dasd_block_tasklet splices it into ccw_queue.
> +	 * Direct enqueue here would invert queue_lock / ccwdev_lock.
> +	 */
> +	spin_lock(&block->ese_lock);
> +	list_add(&fcqr->blocklist, &block->ese_staging);
> +	spin_unlock(&block->ese_lock);
> +	/* mark origin CQR as aborted; ccwdev_lock is held by the IRQ handler */
> +	cqr->status = DASD_CQR_ABORT;

[Severity: High]
If a device is taken offline while an I/O request is being aborted by an ESE
format operation, _dasd_requests_to_flushqueue() sets
cqr->callback = _dasd_wake_block_flush_cb.

Will marking the status as DASD_CQR_ABORT here trigger a path in
__dasd_process_cqr() that unconditionally clears cqr->callback = NULL?

This would strip the wakeup callback installed by the offline process,
causing the offline process to sleep forever since wake_up(&dasd_flush_wq)
is never called.

> +	goto out;
> +
> +out_retry:
> +	/*
> +	 * If we can't format now, let the request go
> +	 * one extra round. Maybe we can format later.

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

[PATCH 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode

[Stefan Haberland]

Add a single per-device 'full_track_bias' sysfs attribute (0..100) that
gates the full-track write path. 0 disables it, 100 routes every aligned,
full-track write through dasd_eckd_build_cp_tpm_writefulltrack(). Values in
between are reserved for the adaptive heuristic added in the next patch.
For now any non-zero value simply enables full-track writes. Internally the
value is kept in the per-device 'ft_bias' field.

This will control the default IO path only.
In case we get an unformatted track error it will always be used to format
and write the track in one go.

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.

The attribute lives at /sys/bus/ccw/devices/<devid>/full_track_bias and
accepts 0..100. The default is DASD_FT_BIAS_DEFAULT; together with the
adaptive heuristic added in the next patch it uses full-track writes only
where they pay off, avoiding the ESE format penalty out of the box while
keeping the throughput cost off already-formatted volumes.

A 'full_track_bias' module parameter sets the initial value applied to
every device at online time; individual volumes can still be re-tuned
through their sysfs attribute afterwards.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c | 39 ++++++++++++++++++++++++++++++++
 drivers/s390/block/dasd_eckd.c   | 36 +++++++++++++++++++++++++----
 drivers/s390/block/dasd_int.h    | 18 +++++++++++++++
 3 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 381d616ad433..035c022255b6 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)
+{
+	struct dasd_device *device;
+	int len;
+
+	device = dasd_device_from_cdev(to_ccwdev(dev));
+	if (IS_ERR(device))
+		return -ENODEV;
+	len = sysfs_emit(buf, "%u\n", device->ft_bias);
+	dasd_put_device(device);
+	return len;
+}
+
+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)
 {
@@ -2425,6 +2463,7 @@ static struct attribute * dasd_attrs[] = {
 	&dev_attr_erplog.attr,
 	&dev_attr_failfast.attr,
 	&dev_attr_expires.attr,
+	&dev_attr_full_track_bias.attr,
 	&dev_attr_retries.attr,
 	&dev_attr_timeout.attr,
 	&dev_attr_reservation_policy.attr,
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index da488c0775fc..067ab66209b6 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -48,6 +48,18 @@
 MODULE_DESCRIPTION("S/390 DASD ECKD Disks device driver");
 MODULE_LICENSE("GPL");
 
+/*
+ * Default full-track write bias applied to every ESE volume at online time;
+ * individual volumes can be re-tuned afterwards through their per-device
+ * full_track_bias sysfs attribute. 0 disables full-track writes, 100 always
+ * uses them, 50 (the default) enables the adaptive heuristic. Values above
+ * DASD_FT_BIAS_MAX are capped when applied.
+ */
+static unsigned int full_track_bias = DASD_FT_BIAS_DEFAULT;
+module_param(full_track_bias, uint, 0644);
+MODULE_PARM_DESC(full_track_bias,
+		 "Default ESE full-track write bias 0..100 (0=off, 1..99=adaptive, 100=always)");
+
 static struct dasd_discipline dasd_eckd_discipline;
 
 /* The ccw bus type uses this table to find devices that it sends to
@@ -2149,6 +2161,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;
+
 	if (private->conf.gneq) {
 		value = 1;
 		for (i = 0; i < private->conf.gneq->timeout.value; i++)
@@ -5204,11 +5221,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) {
+			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);
+		}
 		if (IS_ERR(cqr) && (PTR_ERR(cqr) != -EAGAIN) &&
 		    (PTR_ERR(cqr) != -ENOMEM))
 			cqr = NULL;
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index e1ffa20db10a..b8a3190c9c93 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -629,6 +629,10 @@ struct dasd_device {
 	struct dasd_copy_relation *copy;
 	unsigned long aq_mask;
 	unsigned int aq_timeouts;
+
+	/* ESE fulltrack write control (see full_track_bias sysfs attribute) */
+	unsigned int ft_bias;	/* aggressiveness 0..100: 0=off, 100=always */
+	unsigned int fulltrack;	/* internal: use WRITE_FULL_TRACK for aligned writes */
 };
 
 struct dasd_block {
@@ -686,6 +690,20 @@ struct dasd_queue {
 #define DASD_STOPPED_PPRC    32        /* PPRC swap */
 #define DASD_STOPPED_NOSPC   128       /* no space left */
 
+/*
+ * ESE fulltrack write aggressiveness (full_track_bias sysfs attribute), 0..100:
+ *   0   - never use proactively WRITE_FULL_TRACK
+ *   100 - always use proactively WRITE_FULL_TRACK, no probing
+ *   1..99 - adaptive; higher means switch to ft more eagerly
+ * WRITE_FULL_TRACK has an advantage on sparse formatted ESE devices
+ * but it has an overall penalty for maximum throughput for fully
+ * formatted devices.
+ * The default of 50 tries to balance both and do some probing in between
+ * to choose the best mode for default IO.
+ */
+#define DASD_FT_BIAS_MAX	100
+#define DASD_FT_BIAS_DEFAULT	50
+
 /* per device flags */
 #define DASD_FLAG_OFFLINE	3	/* device is in offline processing */
 #define DASD_FLAG_EER_SNSS	4	/* A SNSS is required */
-- 
2.53.0

Re: [PATCH 13/19] s390/dasd: Add full_track_bias to control fulltrack write mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Using `rq_data_dir(req) == WRITE` to gate data buffer accesses allows payload-less requests (such as DISCARD or WRITE_ZEROES) to bypass type safety, leading to a potential NULL pointer dereference.
- [Medium] Inconsistent initialization of `fulltrack` between the module parameter logic and the sysfs store function, leading to divergent behavior for the exact same configuration value.
- [Medium] Concurrent sysfs writes to `full_track_bias` can leave `ft_bias` and `fulltrack` in a permanently inconsistent state.
--

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

s390/dasd: Add full_track_bias to control fulltrack write mode

This patch introduces a per-device full_track_bias sysfs attribute to control
the full-track write path for DASD ECKD devices. It routes aligned full-track
writes through a new function based on the bias value. The patch also adds a
module parameter to set the default bias for all devices at online time.

> 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 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: Medium]
Can concurrent sysfs writes to full_track_bias leave ft_bias and fulltrack
in a permanently inconsistent state?

If CPU1 writes 100 and CPU2 writes 0 at the same time, could they interleave
their updates to these two fields, leaving ft_bias at 0 and fulltrack at 1,
or vice-versa?

[Severity: Medium]
Does the logic here mismatch how fulltrack is initialized during online time?

In dasd_eckd_check_characteristics(), fulltrack is set to 1 only when the
bias is >= DASD_FT_BIAS_MAX (100). However, in this sysfs store function,
fulltrack is set to 1 for any non-zero value. Should these conditions match
to prevent the exact same configuration value from producing different
behaviors?

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

[ ... ]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index da488c0775fcd..067ab66209b65 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -5204,11 +5221,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: High]
Can payload-less requests (such as DISCARD or WRITE_ZEROES) bypass type
safety here?

The rq_data_dir() check relies on op_is_write(), which returns true for
dataless operations. If dasd_eckd_build_cp_tpm_writefulltrack() then iterates
over the bio segments without verifying bio_has_data(), could this cause a
NULL pointer dereference when accessing the data fields?

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

[PATCH 14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

[Stefan Haberland]

Turn the middle of the ft_bias range (1..99) into an adaptive heuristic
that switches between fulltrack write (ft1) and plain write ft0 depending
on how sparse the device still is.

A sparse device benefits from fulltrack writes (it avoids the format/retry
cycle); once enough tracks are formatted the per-write overhead of
ft1 outweighs that. An state machine  measures the NRF rate in short ft0
probe windows and flips back to ft1 when it is high
(FT1_ACTIVE -> PROBING -> FT0_STABLE, with a backing-off reprobe interval).

The four parameters are derived from ft_bias by linear interpolation,
anchored so ft_bias == 50 derives the following values:

ese_heu_start_interval - 2000 - IOs in ft1, before first ft0-Probe starts
ese_heu_probe_window - 100 - IOs in probe window
ese_heu_nrf_high - 10 ‰ (= 1 %) - TRACK_FORMAT rate that leads to ft1
ese_heu_max_interval - 500000 - Backoff-Cap: max. IOs between two probes

Higher is more eager to use ft1, and 0/100 skips the heuristic.
The NRF counter is bumped in dasd_eckd_ese_format() for both the classic
NRF sense and the HPF INV_TRACK_FORMAT equivalent.
The state machine resets to ft1 on check_characteristics, full format,
and release-space.

A read-only ese_heuristic_state sysfs attribute exposes the current mode.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c | 47 +++++++++++++++-
 drivers/s390/block/dasd_eckd.c   | 97 ++++++++++++++++++++++++++++++--
 drivers/s390/block/dasd_int.h    | 85 ++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 035c022255b6..f6aab94b7be6 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -1659,8 +1659,14 @@ static ssize_t full_track_bias_store(struct device *dev,
 	if (IS_ERR(device))
 		return -ENODEV;
 
+	/*
+	 * ft_bias is the tuning target; fulltrack is a best-effort mode hint
+	 * that the per-IO heuristic also updates locklessly. A racing writer can
+	 * at most leave a transient mismatch that self-corrects on the next IO,
+	 * never corruption, so the update is left unlocked.
+	 */
 	device->ft_bias = val;
-	device->fulltrack = val ? 1 : 0;
+	dasd_ft_bias_apply(device);
 
 	dasd_put_device(device);
 	return count;
@@ -1668,6 +1674,44 @@ static ssize_t full_track_bias_store(struct device *dev,
 
 static DEVICE_ATTR_RW(full_track_bias);
 
+static const char * const dasd_ese_heu_state_names[] = {
+	[DASD_ESE_HEU_FT1_ACTIVE] = "fulltrack active",
+	[DASD_ESE_HEU_PROBING]    = "probing",
+	[DASD_ESE_HEU_FT0_STABLE] = "fulltrack inactive",
+};
+
+/* read-only: current full-track mode / adaptive FSM state, for observability */
+static ssize_t
+ese_heuristic_state_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct dasd_device *device;
+	unsigned int state;
+	int len;
+
+	device = dasd_device_from_cdev(to_ccwdev(dev));
+	if (IS_ERR(device))
+		return -ENODEV;
+	if (device->ft_bias == 0) {
+		len = sysfs_emit(buf, "fulltrack deactivated\n");
+	} else if (device->ft_bias >= DASD_FT_BIAS_MAX) {
+		len = sysfs_emit(buf, "fulltrack permanent active\n");
+	} else if (!dasd_ese_adaptive(device)) {
+		/* adaptive range but not ESE: the heuristic does not run */
+		len = sysfs_emit(buf, "fulltrack deactivated\n");
+	} else {
+		state = device->ese_probe_state;
+		if (state < ARRAY_SIZE(dasd_ese_heu_state_names))
+			len = sysfs_emit(buf, "%s\n", dasd_ese_heu_state_names[state]);
+		else
+			len = sysfs_emit(buf, "unknown\n");
+	}
+	dasd_put_device(device);
+	return len;
+}
+
+static DEVICE_ATTR_RO(ese_heuristic_state);
+
 static ssize_t
 dasd_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -2464,6 +2508,7 @@ static struct attribute * dasd_attrs[] = {
 	&dev_attr_failfast.attr,
 	&dev_attr_expires.attr,
 	&dev_attr_full_track_bias.attr,
+	&dev_attr_ese_heuristic_state.attr,
 	&dev_attr_retries.attr,
 	&dev_attr_timeout.attr,
 	&dev_attr_reservation_policy.attr,
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 067ab66209b6..62c03c4787c8 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2161,11 +2161,6 @@ 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;
-
 	if (private->conf.gneq) {
 		value = 1;
 		for (i = 0; i < private->conf.gneq->timeout.value; i++)
@@ -2220,6 +2215,13 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 	/* Read Volume Information */
 	dasd_eckd_read_vol_info(device);
 
+	/*
+	 * is_ese() now reflects the hardware ESE state, so derive the default
+	 * fulltrack write bias from the module parameter.
+	 */
+	device->ft_bias = min_t(unsigned int, full_track_bias, DASD_FT_BIAS_MAX);
+	dasd_ft_bias_apply(device);
+
 	/* Read Extent Pool Information */
 	dasd_eckd_read_ext_pool_info(device);
 
@@ -3172,6 +3174,13 @@ static int dasd_eckd_format_process_data(struct dasd_device *base,
 static int dasd_eckd_format_device(struct dasd_device *base,
 				   struct format_data_t *fdata, int enable_pav)
 {
+	/*
+	 * A full format (start_unit == 0) returns the device to a fully sparse
+	 * state, so restart the heuristic from ft1 without an offline cycle.
+	 */
+	if (fdata->start_unit == 0)
+		dasd_ft_bias_apply(base);
+
 	return dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
 					     0, NULL);
 }
@@ -3231,6 +3240,69 @@ static void clear_format_track(struct dasd_format_entry *format,
 	spin_unlock_irqrestore(&block->format_lock, flags);
 }
 
+/*
+ * Adaptive ft_bias heuristic, called once per IO from dasd_eckd_build_cp().
+ * Probes the device formatting state by briefly switching to ft0 and measuring
+ * the NRF rate; parameters are derived from ft_bias.
+ */
+static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
+{
+	int ios, nrf, rate;
+
+	if (atomic_inc_return(&basedev->ese_io_cnt) < (int)basedev->ese_probe_interval)
+		return;
+
+	/*
+	 * One wins the race to evaluate, the rest see ios == 0 after the
+	 * xchg and return early, preventing redundant state transitions.
+	 */
+	ios = atomic_xchg(&basedev->ese_io_cnt, 0);
+	if (ios <= 0)
+		return;
+
+	switch (basedev->ese_probe_state) {
+	case DASD_ESE_HEU_FT1_ACTIVE:
+		/* Start ft0 probe window, reset NRF counter for clean measurement */
+		basedev->fulltrack          = 0;
+		basedev->ese_probe_state    = DASD_ESE_HEU_PROBING;
+		basedev->ese_probe_interval = basedev->ese_heu_probe_window;
+		atomic_set(&basedev->ese_nrf_window, 0);
+		break;
+
+	case DASD_ESE_HEU_PROBING:
+	case DASD_ESE_HEU_FT0_STABLE:
+		nrf  = atomic_xchg(&basedev->ese_nrf_window, 0);
+		rate = (int)((u64)nrf * 1000 / ios);
+		if (rate > (int)basedev->ese_heu_nrf_high) {
+			/* NRF rate high: device still sparse, ft1 is better */
+			basedev->fulltrack          = 1;
+			basedev->ese_probe_state    = DASD_ESE_HEU_FT1_ACTIVE;
+			basedev->ese_probe_interval = basedev->ese_heu_start_interval;
+		} else if (basedev->ese_probe_state == DASD_ESE_HEU_PROBING) {
+			/*
+			 * NRF rate low: device mostly formatted, ft0 is faster.
+			 * Re-probe frequently at first, then back off below.
+			 */
+			basedev->fulltrack          = 0;
+			basedev->ese_probe_state    = DASD_ESE_HEU_FT0_STABLE;
+			basedev->ese_probe_interval = basedev->ese_heu_probe_window;
+		} else {
+			/*
+			 * Still stable in ft0: re-assert plain-write mode so a
+			 * fulltrack value left behind by a racing sysfs write
+			 * self-corrects, and back off the re-probe interval
+			 * (double it, capped at max_interval) so a long-lived
+			 * formatted device is not probed more often than needed.
+			 */
+			basedev->fulltrack          = 0;
+			basedev->ese_probe_interval =
+				min(basedev->ese_probe_interval * 2,
+				    basedev->ese_heu_max_interval);
+		}
+		break;
+	}
+}
+
 static void dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
 				 struct irb *irb)
 {
@@ -3255,6 +3327,8 @@ static void dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_r
 	block = cqr->block;
 	base = block->base;
 	private = base->private;
+	if (dasd_ese_adaptive(base))
+		atomic_inc(&base->ese_nrf_window);
 	blksize = block->bp_block;
 	recs_per_trk = recs_per_track(&private->rdc_data, 0, blksize);
 
@@ -4020,6 +4094,14 @@ static int dasd_eckd_release_space_full(struct dasd_device *device)
 
 	rc = dasd_sleep_on_interruptible(cqr);
 
+	if (!rc) {
+		/*
+		 * Releasing all space (RAS) wipes every track and the device is fully
+		 * sparse again, so restart the heuristic from ft1.
+		 */
+		dasd_ft_bias_apply(device);
+	}
+
 	dasd_sfree_request(cqr, cqr->memdev);
 
 	return rc;
@@ -5188,6 +5270,11 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 	struct dasd_ccw_req *cqr;
 
 	basedev = block->base;
+	if (dasd_ese_adaptive(basedev))
+		dasd_ese_heuristic_tick(basedev);
+	else
+		/* re-assert the endpoint mode: a stale heuristic write cannot stick */
+		basedev->fulltrack = (basedev->ft_bias >= DASD_FT_BIAS_MAX) ? 1 : 0;
 	private = basedev->private;
 
 	/* Calculate number of blocks/records per track. */
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index b8a3190c9c93..59cce4e7dbc1 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -633,6 +633,15 @@ struct dasd_device {
 	/* ESE fulltrack write control (see full_track_bias sysfs attribute) */
 	unsigned int ft_bias;	/* aggressiveness 0..100: 0=off, 100=always */
 	unsigned int fulltrack;	/* internal: use WRITE_FULL_TRACK for aligned writes */
+	/* adaptive heuristic (active for ft_bias 1..99), derived from ft_bias */
+	unsigned int ese_probe_state;        /* heuristic FSM state */
+	unsigned int ese_probe_interval;     /* IOs between evaluations */
+	atomic_t ese_io_cnt;                 /* IO counter for current window */
+	atomic_t ese_nrf_window;             /* NRF/INV_TRACK_FORMAT events in window */
+	unsigned int ese_heu_start_interval; /* IOs before first probe */
+	unsigned int ese_heu_probe_window;   /* IOs in probe window */
+	unsigned int ese_heu_max_interval;   /* max IOs between probes (backoff cap) */
+	unsigned int ese_heu_nrf_high;       /* NRF per-mille threshold → activate ft1 */
 };
 
 struct dasd_block {
@@ -704,6 +713,25 @@ struct dasd_queue {
 #define DASD_FT_BIAS_MAX	100
 #define DASD_FT_BIAS_DEFAULT	50
 
+/* ESE fulltrack heuristic FSM states (adaptive range, ft_bias 1..99) */
+#define DASD_ESE_HEU_FT1_ACTIVE   0   /* fulltrack write active */
+#define DASD_ESE_HEU_PROBING      1   /* ft0 probe window, measuring NRF rate */
+#define DASD_ESE_HEU_FT0_STABLE   2   /* device formatted, ft0 active */
+
+/*
+ * Heuristic parameters are derived from ft_bias by linear interpolation,
+ * anchored so that ft_bias == 50 reproduces the previously shipped defaults
+ * and ft_bias == 100 is the most aggressive end of the range.
+ * probe_window is constant.
+ */
+#define DASD_ESE_HEU_PROBE_WINDOW	   100
+#define DASD_ESE_HEU_NRF_HIGH_A50	    10	/* NRF per-mille threshold */
+#define DASD_ESE_HEU_NRF_HIGH_A100	     1
+#define DASD_ESE_HEU_START_A50		  2000	/* IOs before first probe */
+#define DASD_ESE_HEU_START_A100		   500
+#define DASD_ESE_HEU_MAX_A50		500000	/* backoff cap */
+#define DASD_ESE_HEU_MAX_A100		 20000
+
 /* per device flags */
 #define DASD_FLAG_OFFLINE	3	/* device is in offline processing */
 #define DASD_FLAG_EER_SNSS	4	/* A SNSS is required */
@@ -866,6 +894,63 @@ static inline bool dasd_req_conflict(struct dasd_ccw_req *cqr1,
 		 cqr2->end_trk < cqr1->format->start_trk);
 }
 
+/*
+ * true when device is ese device and ft_bias selects the adaptive
+ * heuristic (neither hard endpoint)
+ */
+static inline bool dasd_ese_adaptive(struct dasd_device *device)
+{
+	return device->discipline &&
+		device->discipline->is_ese &&
+		device->discipline->is_ese(device) &&
+		device->ft_bias > 0 &&
+		device->ft_bias < DASD_FT_BIAS_MAX;
+}
+
+/*
+ * Linear interpolation of a heuristic parameter between its value at aggr==50
+ * (v50) and its value at aggr==100 (v100).
+ */
+static inline unsigned int dasd_ese_lerp(unsigned int v50, unsigned int v100,
+					 unsigned int aggr)
+{
+	return (unsigned int)((int)v50 +
+		((int)v100 - (int)v50) * ((int)aggr - 50) / 50);
+}
+
+/*
+ * Apply the ft_bias knob. For the hard endpoints just pin the mode; for the
+ * adaptive range derive the heuristic parameters from ft_bias and (re)start
+ * the FSM in ft1 so a freshly sparse device avoids the NRF penalty right away.
+ */
+static inline void dasd_ft_bias_apply(struct dasd_device *device)
+{
+	unsigned int a = device->ft_bias;
+
+	if (!dasd_ese_adaptive(device)) {
+		device->fulltrack = (a >= DASD_FT_BIAS_MAX) ? 1 : 0;
+		device->ese_probe_state = DASD_ESE_HEU_FT1_ACTIVE;
+		return;
+	}
+
+	device->ese_heu_nrf_high =
+		dasd_ese_lerp(DASD_ESE_HEU_NRF_HIGH_A50,
+			      DASD_ESE_HEU_NRF_HIGH_A100, a);
+	device->ese_heu_start_interval =
+		dasd_ese_lerp(DASD_ESE_HEU_START_A50,
+			      DASD_ESE_HEU_START_A100, a);
+	device->ese_heu_max_interval =
+		dasd_ese_lerp(DASD_ESE_HEU_MAX_A50,
+			      DASD_ESE_HEU_MAX_A100, a);
+	device->ese_heu_probe_window = DASD_ESE_HEU_PROBE_WINDOW;
+
+	device->ese_probe_state    = DASD_ESE_HEU_FT1_ACTIVE;
+	device->ese_probe_interval = device->ese_heu_start_interval;
+	device->fulltrack          = 1;
+	atomic_set(&device->ese_io_cnt, 0);
+	atomic_set(&device->ese_nrf_window, 0);
+}
+
 /* externals in dasd.c */
 #define DASD_PROFILE_OFF	 0
 #define DASD_PROFILE_ON 	 1
-- 
2.53.0

Re: [PATCH 14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent IOs bypass the lockless exclusion in `dasd_ese_heuristic_tick`, causing multiple threads to execute state transitions and corrupting the heuristic's state and backoff intervals.
--

--- Patch [14]: [PATCH 14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit e9a2e43413ef73d4ffb3fe2ea3f44d65c5f79bff
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias

Derives adaptive ESE fulltrack heuristic parameters from ft_bias to
locklessly switch between fulltrack and plain writes based on device
sparseness.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 067ab66209b65..62c03c4787c83 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -3231,6 +3240,69 @@ static void clear_format_track(struct dasd_format_entry *format,
>  	spin_unlock_irqrestore(&block->format_lock, flags);
>  }
>  
> +/*
> + * Adaptive ft_bias heuristic, called once per IO from dasd_eckd_build_cp().
> + * Probes the device formatting state by briefly switching to ft0 and measuring
> + * the NRF rate; parameters are derived from ft_bias.
> + */
> +static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
> +{
> +	int ios, nrf, rate;
> +
> +	if (atomic_inc_return(&basedev->ese_io_cnt) < (int)basedev->ese_probe_interval)
> +		return;
> +
> +	/*
> +	 * One wins the race to evaluate, the rest see ios == 0 after the
> +	 * xchg and return early, preventing redundant state transitions.
> +	 */
> +	ios = atomic_xchg(&basedev->ese_io_cnt, 0);
> +	if (ios <= 0)
> +		return;

[Severity: Medium]
Can this lockless exclusion logic be bypassed if a third thread increments
the counter right after the first thread clears it?

If Thread A and Thread B both pass the atomic_inc_return() threshold check
concurrently, they will both proceed to the atomic_xchg().

If Thread A executes atomic_xchg() first, it resets the counter to 0 and
proceeds to evaluate the state machine.

If Thread C then calls dasd_ese_heuristic_tick() and increments the counter
from 0 to 1, it will return early because it is below the threshold. However,
Thread B could then execute its atomic_xchg() and receive 1.

Since 1 is greater than 0, Thread B will also pass the ios <= 0 check and
evaluate the state machine concurrently with Thread A.

Could this concurrent execution corrupt the heuristic state and backoff
intervals, causing premature transitions (for example, from FT1_ACTIVE to
PROBING)?

> +
> +	switch (basedev->ese_probe_state) {
> +	case DASD_ESE_HEU_FT1_ACTIVE:
> +		/* Start ft0 probe window, reset NRF counter for clean measurement */

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

[PATCH 15/19] s390/dasd: Stamp a format label into newly formatted volumes

[Stefan Haberland]

When a CDL volume is formatted, write a small on-disk label so the format
can later be recognised by the kernel. The next patch will use this for
ESE detection.

The label records a magic, a version, whether the volume is ESE, and
whether it was formatted quick (space released, thin) or full.

It lives in track 0, head 0, record 4 (the first non-special CDL record).
R4 is written by the same channel program that formats track 0
- its WRITE_CKD transfers count + the label data instead of count-only -
so label and track format reach the disk atomically; a valid magic then
marks a completed format without a separate, racy write.

Quick vs full is derived from a full space release (RAS) preceding the
format: dasd_eckd_release_space_full() sets a per-device flag the next
format consumes. Non-ESE volumes and formats without a preceding full
release are recorded as full.

struct dasd_format_label is exactly 512 bytes (the smallest block size)
so it fits one record; larger blocks zero-pad the rest.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 81 +++++++++++++++++++++++++++++++---
 drivers/s390/block/dasd_eckd.h | 38 ++++++++++++++++
 2 files changed, 114 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 62c03c4787c8..25a9055b97a2 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
+#include <linux/utsname.h>
 #include <linux/io.h>
 #include <linux/overflow.h>
 
@@ -2723,6 +2724,28 @@ dasd_eckd_build_check(struct dasd_device *base, struct format_data_t *fdata,
 	return cqr;
 }
 
+/* Fill the format label into a R4 record buffer, zero-padded to blksize. */
+static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
+					unsigned int blksize)
+{
+	struct dasd_eckd_private *private = device->private;
+	struct dasd_format_label *label = data;
+
+	memset(label, 0, blksize);
+	label->magic = DASD_ESE_LABEL_MAGIC;
+	label->version = DASD_ESE_LABEL_VERSION;
+	if (dasd_eckd_is_ese(device))
+		label->flags |= DASD_ESE_LABEL_F_ESE;
+	if (private->ese_format_quick)
+		label->flags |= DASD_ESE_LABEL_F_QUICK;
+	else
+		label->flags |= DASD_ESE_LABEL_F_FULL;
+	label->blksize = blksize;
+	label->format_tod = get_tod_clock();
+	strscpy(label->kernel_version, init_utsname()->release,
+		sizeof(label->kernel_version));
+}
+
 static struct dasd_ccw_req *
 dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
 		       struct format_data_t *fdata, int enable_pav)
@@ -2741,6 +2764,7 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
 	int r0_perm;
 	int nr_tracks;
 	int use_prefix;
+	int write_label;
 
 	if (enable_pav)
 		startdev = dasd_alias_get_start_dev(base);
@@ -2774,6 +2798,15 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
 
 	use_prefix = base_priv->features.feature[8] & 0x01;
 
+	/*
+	 * Stamp the format label into R4 of the very first track. Only for CDL
+	 * (R4 is the first non-special record there), only when this request
+	 * covers track 0, only for the record-writing format intensities (not
+	 * track invalidation), and only if the track actually has an R4.
+	 */
+	write_label = (intensity & 0x08) && !((intensity & ~0x08) & 0x04) &&
+		      fdata->start_unit == 0 && rpt > 3;
+
 	switch (intensity) {
 	case 0x00:	/* Normal format */
 	case 0x08:	/* Normal format, use cdl. */
@@ -2820,6 +2853,10 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
 		return ERR_PTR(-EINVAL);
 	}
 
+	/* room for the label data that R4 carries in addition to its count */
+	if (write_label)
+		datasize += fdata->blksize;
+
 	fcp = dasd_fmalloc_request(DASD_ECKD_MAGIC, cplength, datasize, startdev);
 	if (IS_ERR(fcp))
 		return fcp;
@@ -2964,7 +3001,21 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
 					ccw->cmd_code =
 						DASD_ECKD_CCW_WRITE_CKD_MT;
 				ccw->flags = CCW_FLAG_SLI;
-				ccw->count = 8;
+				if (write_label && address.cyl == 0 &&
+				    address.head == 0 && i == 3) {
+					/*
+					 * R4 carries the label as its record
+					 * data; it follows ect contiguously so
+					 * the CCW transfers count + data.
+					 */
+					dasd_eckd_fill_format_label(base,
+								    data,
+								    fdata->blksize);
+					data += fdata->blksize;
+					ccw->count = 8 + fdata->blksize;
+				} else {
+					ccw->count = 8;
+				}
 				ccw->cda = virt_to_dma32(ect);
 				ccw++;
 			}
@@ -3174,6 +3225,9 @@ static int dasd_eckd_format_process_data(struct dasd_device *base,
 static int dasd_eckd_format_device(struct dasd_device *base,
 				   struct format_data_t *fdata, int enable_pav)
 {
+	struct dasd_eckd_private *private = base->private;
+	int rc;
+
 	/*
 	 * A full format (start_unit == 0) returns the device to a fully sparse
 	 * state, so restart the heuristic from ft1 without an offline cycle.
@@ -3181,8 +3235,18 @@ static int dasd_eckd_format_device(struct dasd_device *base,
 	if (fdata->start_unit == 0)
 		dasd_ft_bias_apply(base);
 
-	return dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
-					     0, NULL);
+	rc = dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
+					   0, NULL);
+
+	/*
+	 * The quick-format indicator was consumed by the label stamped into
+	 * track 0; clear it so a later format that is not preceded by a full
+	 * space release is recorded as a full format.
+	 */
+	if (fdata->start_unit == 0)
+		private->ese_format_quick = 0;
+
+	return rc;
 }
 
 static bool test_and_set_format_track(sector_t start, sector_t end,
@@ -4085,6 +4149,7 @@ dasd_eckd_dso_ras(struct dasd_device *device, struct dasd_block *block,
 
 static int dasd_eckd_release_space_full(struct dasd_device *device)
 {
+	struct dasd_eckd_private *private;
 	struct dasd_ccw_req *cqr;
 	int rc;
 
@@ -4096,10 +4161,16 @@ static int dasd_eckd_release_space_full(struct dasd_device *device)
 
 	if (!rc) {
 		/*
-		 * Releasing all space (RAS) wipes every track and the device is fully
-		 * sparse again, so restart the heuristic from ft1.
+		 * Releasing all space (RAS) wipes every track and the device is
+		 * fully sparse again, so restart the heuristic from ft1.
 		 */
 		dasd_ft_bias_apply(device);
+		/*
+		 * A full release is what makes a subsequent format a quick
+		 * (thin) one; remember it so the format label records that.
+		 */
+		private = device->private;
+		private->ese_format_quick = 1;
 	}
 
 	dasd_sfree_request(cqr, cqr->memdev);
diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index 0fdb92fdddc8..92fd8ac92b79 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -159,6 +159,39 @@ struct eckd_r0 {
 #define DASD_EAV_CYL_HI_SHIFT	16	/* cylinder bits beyond the 16-bit cyl field */
 #define DASD_EAV_HEAD_HI_SHIFT	4	/* head occupies the low-order 4 bits of head */
 
+/*
+ * On-disk DASD format label.
+ *
+ * Written into track 0, head 0, record 4 (R4 - the first non-special CDL
+ * record) as part of the same channel program that formats track 0, so it is
+ * stored atomically with the track: either both the track format and the label
+ * make it to disk or neither does. Its presence with a valid magic therefore
+ * marks a completed format and can be used for format detection.
+ *
+ * The structure is exactly the smallest supported block size (512 bytes) so it
+ * always fits into a single record.
+ * For larger block sizes the rest of the record is zero padded.
+ * The magic together with the version is used to recognise a valid label.
+ */
+#define DASD_ESE_LABEL_MAGIC	0xC4C1E2C4C6D4E3F1ULL	/* EBCDIC "DASDFMT1" */
+#define DASD_ESE_LABEL_VERSION	1
+
+/* dasd_format_label.flags */
+#define DASD_ESE_LABEL_F_ESE	0x00000001	/* volume is extent space efficient */
+#define DASD_ESE_LABEL_F_QUICK	0x00000002	/* quick (space released) format */
+#define DASD_ESE_LABEL_F_FULL	0x00000004	/* full format */
+
+struct dasd_format_label {
+	__u64 magic;			/* DASD_ESE_LABEL_MAGIC */
+	__u32 version;			/* DASD_ESE_LABEL_VERSION */
+	__u32 flags;			/* DASD_ESE_LABEL_F_* */
+	__u32 blksize;			/* block size the volume was formatted with */
+	__u32 reserved0;
+	__u64 format_tod;		/* TOD clock at format time */
+	__u8  kernel_version[64];	/* NUL terminated kernel release (uname -r) */
+	__u8  reserved[416];		/* pad the struct to 512 bytes */
+} __packed;
+
 struct ch_t {
 	__u16 cyl;
 	__u16 head;
@@ -709,6 +742,11 @@ struct dasd_eckd_private {
 
 	u32 fcx_max_data;
 	char suc_reason;
+	/*
+	 * Set when the whole volume's space was released (full RAS); consumed by
+	 * the next format to mark the on-disk label as a quick (vs full) format.
+	 */
+	int ese_format_quick;
 };
 
 
-- 
2.53.0

Re: [PATCH 15/19] s390/dasd: Stamp a format label into newly formatted volumes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Data race on `ese_format_quick` allows concurrent space release to incorrectly taint the format label of a volume.

Pre-existing issues:
- [High] Heap buffer overflow in `dasd_eckd_build_format` when formatting multiple tracks with intensity 0x01, 0x04, 0x09, or 0x0c.
--

--- Patch [15]: [PATCH 15/19] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 6ce217cc3945c138bfb4345ea55cd9cbaf16ceb9
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Stamp a format label into newly formatted volumes

This commit writes a 512-byte on-disk label during CDL volume formatting to
record whether the volume is ESE and if it was a quick or full format.
The quick format state is derived from a preceding full space release.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 62c03c4787c83..25a9055b97a2c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2723,6 +2724,28 @@ dasd_eckd_build_check(struct dasd_device *base, struct format_data_t *fdata,
>  	return cqr;
>  }
>  
> +/* Fill the format label into a R4 record buffer, zero-padded to blksize. */
> +static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
> +					unsigned int blksize)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +	struct dasd_format_label *label = data;
> +
> +	memset(label, 0, blksize);
> +	label->magic = DASD_ESE_LABEL_MAGIC;
> +	label->version = DASD_ESE_LABEL_VERSION;
> +	if (dasd_eckd_is_ese(device))
> +		label->flags |= DASD_ESE_LABEL_F_ESE;
> +	if (private->ese_format_quick)

[Severity: Medium]
Is the ese_format_quick flag safe from concurrent updates?

In dasd_eckd_fill_format_label(), private->ese_format_quick is read locklessly
while generating the CCW requests in userspace context.

If a concurrent BIODASDRAS (space release) ioctl completes and sets this flag 
to 1 via dasd_eckd_release_space_full():

dasd_eckd_release_space_full()
    private->ese_format_quick = 1;

Could the concurrent format process fetch a stale or intermediate value and
incorrectly encode DASD_ESE_LABEL_F_QUICK into the volume's on-disk label?

> +		label->flags |= DASD_ESE_LABEL_F_QUICK;
> +	else
> +		label->flags |= DASD_ESE_LABEL_F_FULL;
> +	label->blksize = blksize;
> +	label->format_tod = get_tod_clock();
> +	strscpy(label->kernel_version, init_utsname()->release,
> +		sizeof(label->kernel_version));
> +}

[ ... ]

> @@ -2774,6 +2798,15 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  
>  	use_prefix = base_priv->features.feature[8] & 0x01;
>  
> +	/*
> +	 * Stamp the format label into R4 of the very first track. Only for CDL
> +	 * (R4 is the first non-special record there), only when this request
> +	 * covers track 0, only for the record-writing format intensities (not
> +	 * track invalidation), and only if the track actually has an R4.
> +	 */
> +	write_label = (intensity & 0x08) && !((intensity & ~0x08) & 0x04) &&
> +		      fdata->start_unit == 0 && rpt > 3;
> +
>  	switch (intensity) {
>  	case 0x00:	/* Normal format */
>  	case 0x08:	/* Normal format, use cdl. */
> @@ -2820,6 +2853,10 @@ dasd_eckd_build_format(struct dasd_device *base, struct dasd_device *startdev,
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: High]
This is a pre-existing issue, but does this allocation lack a multiplication 
factor for nr_tracks?

Looking at dasd_eckd_build_format(), for track invalidation 
(intensity 0x04 or 0x0c), the memory calculation sets cplength statically
to 3 and datasize only provisions for a single eckd_count payload.

Later in the function, the code unconditionally iterates over nr_tracks:

for (j = 0; j < nr_tracks; j++) {
    ...
    if ((intensity & ~0x08) & 0x04) {  /* erase track */
        ect = (struct eckd_count *) data;
        data += sizeof(struct eckd_count);
        ...
        ccw++;
    }
}

If nr_tracks is greater than 1, it appears this loop will write past the 
allocated bounds of the memory chunk since the allocation size was not scaled
by nr_tracks.

Can this cause a heap buffer overflow during a privileged BIODASDFMT ioctl?

>  
> +	/* room for the label data that R4 carries in addition to its count */
> +	if (write_label)
> +		datasize += fdata->blksize;
> +
>  	fcp = dasd_fmalloc_request(DASD_ECKD_MAGIC, cplength, datasize, startdev);
>  	if (IS_ERR(fcp))
>  		return fcp;

[ ... ]

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

[PATCH 16/19] s390/dasd: Detect ESE volumes from the on-disk format label

[Stefan Haberland]

Read the format label from track 0 record 4 at device bring-up and cache
it. When a valid label is present, is_ese() is derived from it instead of
the hardware volume field.
A volume copied off ESE storage onto other hardware is thus still handled
as thin.
Without a label (older format) is_ese() falls back to the hardware field as
before.
The cache is refreshed after a format so is_ese() stays coherent without an
offline/online cycle.

The label F_ESE bit is stamped from the hardware capability rather than
is_ese(), and space release (quick format) is gated on the hardware
capability, so a copied label cannot enable it on non-ESE hardware.

The ese sysfs attribute, and with this lsdasd, shows the hardware
capability and not the internal handling. This is in line with the view
from storage server interface. To reflect the specific internal handling an
additional attribute on_demand_formatting is added to show that a device is
handled like an ESE device internally based on the disk label.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c |   6 +-
 drivers/s390/block/dasd_eckd.c   | 174 ++++++++++++++++++++++++++++---
 drivers/s390/block/dasd_eckd.h   |   7 ++
 drivers/s390/block/dasd_int.h    |   3 +
 4 files changed, 175 insertions(+), 15 deletions(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index f6aab94b7be6..d07d384a004f 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -2482,9 +2482,10 @@ static ssize_t dasd_##_name##_show(struct device *dev,			\
 									\
 	return sysfs_emit(buf, "%d\n", val);			\
 }									\
-static DEVICE_ATTR(_name, 0444, dasd_##_name##_show, NULL);		\
+static DEVICE_ATTR(_name, 0444, dasd_##_name##_show, NULL);
 
-DASD_DEFINE_ATTR(ese, device->discipline->is_ese);
+DASD_DEFINE_ATTR(ese, device->discipline->ese_capable);
+DASD_DEFINE_ATTR(on_demand_formatting, device->discipline->on_demand_format);
 DASD_DEFINE_ATTR(extent_size, device->discipline->ext_size);
 DASD_DEFINE_ATTR(pool_id, device->discipline->ext_pool_id);
 DASD_DEFINE_ATTR(space_configured, device->discipline->space_configured);
@@ -2522,6 +2523,7 @@ static struct attribute * dasd_attrs[] = {
 	&dev_attr_path_reset.attr,
 	&dev_attr_hpf.attr,
 	&dev_attr_ese.attr,
+	&dev_attr_on_demand_formatting.attr,
 	&dev_attr_fc_security.attr,
 	&dev_attr_copy_pair.attr,
 	&dev_attr_copy_role.attr,
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 25a9055b97a2..d5854ed0076e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1677,7 +1677,8 @@ static int dasd_eckd_read_vol_info(struct dasd_device *device)
 	return rc;
 }
 
-static int dasd_eckd_is_ese(struct dasd_device *device)
+/* Hardware/volume ESE capability, from the Volume Storage Query. */
+static int dasd_eckd_ese_capable(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 
@@ -1687,6 +1688,53 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
 	return private->vsq.vol_info.ese;
 }
 
+/*
+ * Whether the volume is to be handled as ESE (thin). This reflects the state
+ * of the data, not the hardware: a volume copied off ESE storage onto other
+ * hardware still needs ESE handling. The on-disk format label is authoritative
+ * when present; without it (e.g. a volume formatted by an older driver) fall
+ * back to the hardware ESE field.
+ *
+ * Only the F_ESE flag gates this. An ESE volume is thin regardless of whether
+ * it was quick- or full-formatted (tracks are allocated on write, and discard
+ * re-thins a full one).
+ */
+static int dasd_eckd_is_ese(struct dasd_device *device)
+{
+	struct dasd_eckd_private *private = device->private;
+
+	/* sysfs may read this during set_online before private is allocated */
+	if (!private)
+		return 0;
+
+	if (private->ese_label_valid)
+		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);
+
+	return dasd_eckd_ese_capable(device);
+}
+
+/*
+ * Whether the volume is formatted on demand (thin), as opposed to fully
+ * formatted. This is the format mode, not the hardware ESE capability. When a
+ * label is present it is authoritative (F_QUICK). Without a label the mode is
+ * unknown, but an ESE volume is still handled on demand (NRF triggers the
+ * format), so fall back to the ESE state to stay consistent with the driver's
+ * behavior on older, label-less volumes.
+ */
+static int dasd_eckd_on_demand_format(struct dasd_device *device)
+{
+	struct dasd_eckd_private *private = device->private;
+
+	/* sysfs may read this during set_online before private is allocated */
+	if (!private)
+		return 0;
+
+	if (private->ese_label_valid)
+		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_QUICK);
+
+	return dasd_eckd_is_ese(device);
+}
+
 static int dasd_eckd_ext_pool_id(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
@@ -2106,6 +2154,69 @@ static bool dasd_eckd_pprc_enabled(struct dasd_device *device)
 	return private->rdc_data.facilities.PPRC_enabled;
 }
 
+/*
+ * Read the on-disk format label from track 0, record 4. On a formatted volume
+ * R4 holds the label as its record data; on an unformatted (fresh ESE) or
+ * label-less volume the read returns No Record Found, which is expected and
+ * leaves the cache invalid so is_ese() falls back to the hardware field.
+ */
+static void dasd_eckd_read_format_label(struct dasd_device *device)
+{
+	struct dasd_eckd_private *private = device->private;
+	struct dasd_format_label *label;
+	struct DE_eckd_data *dedata;
+	struct LO_eckd_data *lodata;
+	struct dasd_ccw_req *cqr;
+	struct ccw1 *ccw;
+
+	private->ese_label_valid = false;
+
+	/* The label lives on the base volume; aliases have none of their own. */
+	if (private->uid.type == UA_BASE_PAV_ALIAS ||
+	    private->uid.type == UA_HYPER_PAV_ALIAS)
+		return;
+
+	cqr = dasd_smalloc_request(DASD_ECKD_MAGIC, 3 /* DE + LO + READ */,
+				   sizeof(*dedata) + sizeof(*lodata) +
+				   sizeof(*label), device, NULL);
+	if (IS_ERR(cqr))
+		return;
+
+	dedata = cqr->data;
+	lodata = (struct LO_eckd_data *)(dedata + 1);
+	label = (struct dasd_format_label *)(lodata + 1);
+
+	ccw = cqr->cpaddr;
+	define_extent(ccw++, dedata, 0, 0, DASD_ECKD_CCW_READ, device, 0);
+	ccw[-1].flags |= CCW_FLAG_CC;
+	locate_record(ccw++, lodata, 0, 4, 1, DASD_ECKD_CCW_READ, device,
+		      sizeof(*label));
+	ccw[-1].flags |= CCW_FLAG_CC;
+	ccw->cmd_code = DASD_ECKD_CCW_READ;
+	ccw->count = sizeof(*label);
+	ccw->flags = CCW_FLAG_SLI;
+	ccw->cda = virt_to_dma32(label);
+
+	cqr->startdev = device;
+	cqr->memdev = device;
+	cqr->block = NULL;
+	cqr->retries = 256;
+	cqr->expires = 10 * HZ;
+	cqr->buildclk = get_tod_clock();
+	cqr->status = DASD_CQR_FILLED;
+	/* R4 may be absent (unformatted) or larger than the label. */
+	set_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
+	set_bit(DASD_CQR_SUPPRESS_IL, &cqr->flags);
+
+	if (!dasd_sleep_on(cqr) &&
+	    label->magic == DASD_ESE_LABEL_MAGIC &&
+	    label->version == DASD_ESE_LABEL_VERSION) {
+		private->ese_label = *label;
+		private->ese_label_valid = true;
+	}
+	dasd_sfree_request(cqr, device);
+}
+
 /*
  * Check device characteristics.
  * If the device is accessible using ECKD discipline, the device is enabled.
@@ -2216,9 +2327,12 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 	/* Read Volume Information */
 	dasd_eckd_read_vol_info(device);
 
+	/* Read the on-disk format label for ESE detection */
+	dasd_eckd_read_format_label(device);
+
 	/*
-	 * is_ese() now reflects the hardware ESE state, so derive the default
-	 * fulltrack write bias from the module parameter.
+	 * is_ese() now reflects the real ESE state (vsq + on-disk label), so
+	 * the adaptive heuristic can be derived correctly for this device.
 	 */
 	device->ft_bias = min_t(unsigned int, full_track_bias, DASD_FT_BIAS_MAX);
 	dasd_ft_bias_apply(device);
@@ -2734,7 +2848,12 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
 	memset(label, 0, blksize);
 	label->magic = DASD_ESE_LABEL_MAGIC;
 	label->version = DASD_ESE_LABEL_VERSION;
-	if (dasd_eckd_is_ese(device))
+	/*
+	 * F_ESE records the hardware capability at format time, not is_ese():
+	 * is_ese() is derived from the label, so using it here would let the
+	 * flag flip on repeated quick/full reformats.
+	 */
+	if (dasd_eckd_ese_capable(device))
 		label->flags |= DASD_ESE_LABEL_F_ESE;
 	if (private->ese_format_quick)
 		label->flags |= DASD_ESE_LABEL_F_QUICK;
@@ -2744,6 +2863,13 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
 	label->format_tod = get_tod_clock();
 	strscpy(label->kernel_version, init_utsname()->release,
 		sizeof(label->kernel_version));
+
+	/*
+	 * Populate the cache directly from the bytes just computed instead of
+	 * synchronously reading them back from disk after the write lands.
+	 */
+	private->ese_label = *label;
+	private->ese_label_valid = true;
 }
 
 static struct dasd_ccw_req *
@@ -3228,23 +3354,35 @@ static int dasd_eckd_format_device(struct dasd_device *base,
 	struct dasd_eckd_private *private = base->private;
 	int rc;
 
-	/*
-	 * A full format (start_unit == 0) returns the device to a fully sparse
-	 * state, so restart the heuristic from ft1 without an offline cycle.
-	 */
-	if (fdata->start_unit == 0)
-		dasd_ft_bias_apply(base);
-
 	rc = dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
 					   0, NULL);
+	if (fdata->start_unit != 0)
+		return rc;
+
+	if (rc) {
+		/*
+		 * The format failed, so the label cached speculatively during
+		 * CCW build may not match the disk; drop it so is_ese() falls
+		 * back to the hardware field until the next successful format
+		 * or bring-up.
+		 */
+		private->ese_label_valid = false;
+		return rc;
+	}
 
 	/*
 	 * The quick-format indicator was consumed by the label stamped into
 	 * track 0; clear it so a later format that is not preceded by a full
 	 * space release is recorded as a full format.
 	 */
-	if (fdata->start_unit == 0)
-		private->ese_format_quick = 0;
+	private->ese_format_quick = 0;
+
+	/*
+	 * A full format returns the device to a fully sparse state and has just
+	 * committed a fresh label; restart the heuristic from ft1 on the now
+	 * current is_ese state, without an offline cycle.
+	 */
+	dasd_ft_bias_apply(base);
 
 	return rc;
 }
@@ -4249,6 +4387,14 @@ static int dasd_eckd_release_space_trks(struct dasd_device *device,
 static int dasd_eckd_release_space(struct dasd_device *device,
 				   struct format_data_t *rdata)
 {
+	/*
+	 * Space release (and thus a quick format) requires real ESE hardware.
+	 * is_ese() may be true from a copied label on non-ESE hardware, so gate
+	 * on the hardware capability, not on is_ese().
+	 */
+	if (!dasd_eckd_ese_capable(device))
+		return -EOPNOTSUPP;
+
 	if (rdata->intensity & DASD_FMT_INT_ESE_FULL)
 		return dasd_eckd_release_space_full(device);
 	else if (rdata->intensity == 0)
@@ -7622,6 +7768,8 @@ static struct dasd_discipline dasd_eckd_discipline = {
 	.hpf_enabled = dasd_eckd_hpf_enabled,
 	.reset_path = dasd_eckd_reset_path,
 	.is_ese = dasd_eckd_is_ese,
+	.ese_capable = dasd_eckd_ese_capable,
+	.on_demand_format = dasd_eckd_on_demand_format,
 	.space_allocated = dasd_eckd_space_allocated,
 	.space_configured = dasd_eckd_space_configured,
 	.logical_capacity = dasd_eckd_logical_capacity,
diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index 92fd8ac92b79..30745f62402b 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -747,6 +747,13 @@ struct dasd_eckd_private {
 	 * the next format to mark the on-disk label as a quick (vs full) format.
 	 */
 	int ese_format_quick;
+	/*
+	 * Cached on-disk format label (R4), read at online and refreshed on
+	 * format. When valid, is_ese() is derived from it; otherwise it falls
+	 * back to the hardware ESE field (vsq.vol_info.ese).
+	 */
+	struct dasd_format_label ese_label;
+	bool ese_label_valid;
 };
 
 
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 59cce4e7dbc1..8c73850f7947 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -401,6 +401,9 @@ struct dasd_discipline {
 	 * Extent Space Efficient (ESE) relevant functions
 	 */
 	int (*is_ese)(struct dasd_device *);
+	int (*ese_capable)(struct dasd_device *);
+	/* Whether the volume is formatted on demand (thin), from the label */
+	int (*on_demand_format)(struct dasd_device *);
 	/* Capacity */
 	int (*space_allocated)(struct dasd_device *);
 	int (*space_configured)(struct dasd_device *);
-- 
2.53.0

Re: [PATCH 16/19] s390/dasd: Detect ESE volumes from the on-disk format label

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Stale ESE label cache after a full format without CDL or with large block sizes.
- [Medium] Missing memory barrier when publishing `ese_label` and `ese_label_valid` leads to data race.

Pre-existing issues:
- [Critical] Heap buffer overflow in `dasd_eckd_build_format` for multi-track formatting with specific intensities.
- [Critical] Use-After-Free of `device->private` due to concurrent sysfs read and `set_online` failure.
--

--- Patch [16]: [PATCH 16/19] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit 8d23af7219b60eb10fdf0ec87204766573d15885
Author: Stefan Haberland <sth@linux.ibm.com>
Subject: s390/dasd: Detect ESE volumes from the on-disk format label

This commit introduces the ability to read the format label from track 0
record 4 during device bring-up to detect Extent Space Efficient (ESE) volumes.
It caches this label and uses it to correctly handle thin volumes even when
copied across different hardware, while also updating sysfs attributes to
expose on-demand formatting status.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 25a9055b97a2c..d5854ed0076e3 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -1687,6 +1688,53 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
>  	return private->vsq.vol_info.ese;
>  }
>  
> +/*
> + * Whether the volume is to be handled as ESE (thin). This reflects the state
> + * of the data, not the hardware: a volume copied off ESE storage onto other
> + * hardware still needs ESE handling. The on-disk format label is authoritative
> + * when present; without it (e.g. a volume formatted by an older driver) fall
> + * back to the hardware ESE field.
> + *
> + * Only the F_ESE flag gates this. An ESE volume is thin regardless of whether
> + * it was quick- or full-formatted (tracks are allocated on write, and discard
> + * re-thins a full one).
> + */
> +static int dasd_eckd_is_ese(struct dasd_device *device)
> +{
> +	struct dasd_eckd_private *private = device->private;
> +
> +	/* sysfs may read this during set_online before private is allocated */
> +	if (!private)
> +		return 0;
> +
> +	if (private->ese_label_valid)
> +		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);

[Severity: Critical]
This is a pre-existing issue, but can this lockless access to device->private
in dasd_eckd_is_ese() result in a use-after-free?

During device initialization in dasd_eckd_check_characteristics(), if an error
occurs (like a failure in reading configuration), the error path out_err1
calls kfree(device->private) and sets it to NULL.

However, if a sysfs read for attributes like "ese" or "on_demand_formatting"
calls dasd_eckd_is_ese() concurrently, it might read the private pointer just
before it's freed:

	struct dasd_eckd_private *private = device->private;

And then dereference it after it is freed when accessing
private->ese_label.flags here. Should there be synchronization against sysfs
readers during the error paths of device initialization?

[Severity: Medium]
Could this read stale or uninitialized data from private->ese_label?

The writer sets private->ese_label_valid = true in
dasd_eckd_fill_format_label() immediately after populating private->ese_label,
but does so without a write memory barrier. 

Because this reader checks private->ese_label_valid and accesses
private->ese_label.flags without a read memory barrier (like
smp_load_acquire()), could the CPU or compiler reorder operations such that
this function sees the valid flag as true but reads uninitialized label data?

> +
> +	return dasd_eckd_ese_capable(device);
> +}
[ ... ]
> @@ -2734,7 +2848,12 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
>  	memset(label, 0, blksize);
>  	label->magic = DASD_ESE_LABEL_MAGIC;
>  	label->version = DASD_ESE_LABEL_VERSION;
> -	if (dasd_eckd_is_ese(device))
> +	/*
> +	 * F_ESE records the hardware capability at format time, not is_ese():
> +	 * is_ese() is derived from the label, so using it here would let the
> +	 * flag flip on repeated quick/full reformats.
> +	 */
> +	if (dasd_eckd_ese_capable(device))
>  		label->flags |= DASD_ESE_LABEL_F_ESE;
>  	if (private->ese_format_quick)
>  		label->flags |= DASD_ESE_LABEL_F_QUICK;
> @@ -2744,6 +2863,13 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
>  	label->format_tod = get_tod_clock();
>  	strscpy(label->kernel_version, init_utsname()->release,
>  		sizeof(label->kernel_version));
> +
> +	/*
> +	 * Populate the cache directly from the bytes just computed instead of
> +	 * synchronously reading them back from disk after the write lands.
> +	 */
> +	private->ese_label = *label;
> +	private->ese_label_valid = true;

[Severity: Medium]
Could this update in dasd_eckd_fill_format_label() cause a data race with
concurrent sysfs readers?

This writes to private->ese_label and then sets private->ese_label_valid to
true without a write memory barrier (such as smp_store_release()). 

Concurrently, dasd_eckd_is_ese() checks private->ese_label_valid and accesses
private->ese_label.flags without a read memory barrier (such as
smp_load_acquire()). Could the CPU or compiler reorder operations such that
readers observe the valid flag as true but read stale or uninitialized label
data?

>  }
[ ... ]
> @@ -3228,23 +3354,35 @@ static int dasd_eckd_format_device(struct dasd_device *base,
>  	struct dasd_eckd_private *private = base->private;
>  	int rc;
>  
> -	/*
> -	 * A full format (start_unit == 0) returns the device to a fully sparse
> -	 * state, so restart the heuristic from ft1 without an offline cycle.
> -	 */
> -	if (fdata->start_unit == 0)
> -		dasd_ft_bias_apply(base);
> -
>  	rc = dasd_eckd_format_process_data(base, fdata, enable_pav, 0, NULL,
>  					   0, NULL);
> +	if (fdata->start_unit != 0)
> +		return rc;
> +
> +	if (rc) {
> +		/*
> +		 * The format failed, so the label cached speculatively during
> +		 * CCW build may not match the disk; drop it so is_ese() falls
> +		 * back to the hardware field until the next successful format
> +		 * or bring-up.
> +		 */
> +		private->ese_label_valid = false;
> +		return rc;
> +	}
>  
>  	/*
>  	 * The quick-format indicator was consumed by the label stamped into
>  	 * track 0; clear it so a later format that is not preceded by a full
>  	 * space release is recorded as a full format.
>  	 */
> -	if (fdata->start_unit == 0)
> -		private->ese_format_quick = 0;
> +	private->ese_format_quick = 0;
> +
> +	/*
> +	 * A full format returns the device to a fully sparse state and has just
> +	 * committed a fresh label; restart the heuristic from ft1 on the now
> +	 * current is_ese state, without an offline cycle.
> +	 */
> +	dasd_ft_bias_apply(base);

[Severity: High]
Could the cached ESE label become stale here in dasd_eckd_format_device() if
the format does not write a label?

If a full format successfully executes but skips writing the label (for example,
when intensity is 0x00 and write_label evaluates to false in
dasd_eckd_build_format()), the on-disk label is erased. 

Because this success path does not clear private->ese_label_valid, the driver
might incorrectly continue to use a stale in-memory label that no longer exists
on the disk. This breaks the on-demand formatting capability detection. Should
the success path also conditionally invalidate the cache if no label was
written?

>  
>  	return rc;
>  }

[Severity: Critical]
This is a pre-existing issue, but does dasd_eckd_build_format() in
drivers/s390/block/dasd_eckd.c calculate the memory sizes incorrectly for
multi-track requests, leading to a heap buffer overflow?

When handling ioctl(DASDFMT) with formatting intensities 0x01, 0x09, 0x04, or
0x0c, the calculation for the required CCWs (cplength) and buffer size
(datasize) seems to fall short when nr_tracks > 1. 

For instance, with intensity 0x01:

	case 0x01:	/* Write record zero and format track. */
	case 0x09:	/* Write record zero and format track, use cdl. */
		cplength = 2 + rpt * nr_tracks;

But the formatting loop later consumes more CCWs:

		if (intensity & 0x01) {	/* write record zero */
			ect = (struct eckd_count *) data;
			data += sizeof(struct eckd_count);
... ccw++;

This consumes (1 + rpt) * nr_tracks CCWs, which exceeds the allocated amount
and overflows fcp->cpaddr. Similarly, datasize only allocates enough space for
one struct eckd_count track header instead of nr_tracks, causing fcp->data to
overflow. Since this formatting ioctl can be issued by a privileged user, could
this deterministic overflow lead to memory corruption?

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

[PATCH 17/19] s390/dasd: Report ESE capability and format mode at device online

[Stefan Haberland]

Extend the device information line logged when a volume comes online with
the ESE hardware capability and the on-disk format mode. The format mode
(full or on demand) is derived from the on-disk format label alone, so a
volume that is not backed by ESE hardware but was still formatted on
demand is reported correctly.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index d5854ed0076e..3c4fcfb1558d 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2523,6 +2523,7 @@ static int dasd_eckd_end_analysis(struct dasd_block *block)
 	struct dasd_device *device = block->base;
 	struct dasd_eckd_private *private = device->private;
 	struct eckd_count *count_area;
+	const char *ese_str, *fmt_str;
 	unsigned int sb, blk_per_trk;
 	int status, i;
 	struct dasd_ccw_req *init_cqr;
@@ -2609,15 +2610,29 @@ static int dasd_eckd_end_analysis(struct dasd_block *block)
 			  private->rdc_data.trk_per_cyl *
 			  blk_per_trk);
 
+	/*
+	 * Report the ESE hardware capability and the format mode. The mode
+	 * comes from dasd_eckd_on_demand_format() (the on-disk label, or the
+	 * ESE state when no label is present), matching the on_demand_formatting
+	 * sysfs attribute.
+	 */
+	ese_str = dasd_eckd_ese_capable(device) ? ", ESE" : "";
+	fmt_str = "";
+	if (dasd_eckd_on_demand_format(device))
+		fmt_str = ", on-demand format";
+	else if (dasd_eckd_ese_capable(device))
+		fmt_str = ", full format";
+
 	dev_info(&device->cdev->dev,
-		 "DASD with %u KB/block, %lu KB total size, %u KB/track, "
-		 "%s\n", (block->bp_block >> 10),
+		 "DASD with %u KB/block, %lu KB total size, %u KB/track, %s%s%s\n",
+		 (block->bp_block >> 10),
 		 (((unsigned long) private->real_cyl *
 		   private->rdc_data.trk_per_cyl *
 		   blk_per_trk * (block->bp_block >> 9)) >> 1),
 		 ((blk_per_trk * block->bp_block) >> 10),
 		 private->uses_cdl ?
-		 "compatible disk layout" : "linux disk layout");
+		 "compatible disk layout" : "linux disk layout",
+		 ese_str, fmt_str);
 
 	return 0;
 }
-- 
2.53.0

Re: [PATCH 17/19] s390/dasd: Report ESE capability and format mode at device online

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing memory barrier when publishing the on-disk format label, allowing lockless sysfs readers to observe uninitialized data.
--

--- Patch [17]: [PATCH 17/19] s390/dasd: Report ESE capability and format mode at device online ---
commit 248d6eb45e332487f9255bf060a4d057f9939181
Author: Stefan Haberland <sth@linux.ibm.com>

s390/dasd: Report ESE capability and format mode at device online

This patch extends the device information line logged when a volume comes
online with the ESE hardware capability and the on-disk format mode. The
format mode is derived from the on-disk format label.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index d5854ed0076e3..3c4fcfb1558de 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2609,15 +2610,29 @@ static int dasd_eckd_end_analysis(struct dasd_block *block)
[ ... ]
> +	/*
> +	 * Report the ESE hardware capability and the format mode. The mode
> +	 * comes from dasd_eckd_on_demand_format() (the on-disk label, or the
> +	 * ESE state when no label is present), matching the on_demand_formatting
> +	 * sysfs attribute.
> +	 */
> +	ese_str = dasd_eckd_ese_capable(device) ? ", ESE" : "";
> +	fmt_str = "";
> +	if (dasd_eckd_on_demand_format(device))

[Severity: Medium]
Could this lockless read of ese_label_valid in dasd_eckd_on_demand_format()
observe uninitialized or torn data from ese_label due to compiler reordering?

Looking at where the label is populated, such as in
dasd_eckd_read_format_label():

		private->ese_label = *label;
		private->ese_label_valid = true;

and in dasd_eckd_fill_format_label():

	private->ese_label = *label;
	private->ese_label_valid = true;

The valid flag is set to true immediately after the struct assignment without a
write memory barrier. In dasd_eckd_on_demand_format(), there is also no
acquire barrier:

	if (private->ese_label_valid)
		return !!(private->ese_label.flags & DASD_ESE_LABEL_F_QUICK);

Since dasd_eckd_on_demand_format() is accessible via sysfs attributes like
on_demand_formatting concurrently while the device is coming online or
formatting, could this missing barrier cause sysfs to transiently return
incorrect values (0 instead of 1)?

> +		fmt_str = ", on-demand format";
> +	else if (dasd_eckd_ese_capable(device))
> +		fmt_str = ", full format";
>  
>  	dev_info(&device->cdev->dev,
> -		 "DASD with %u KB/block, %lu KB total size, %u KB/track, "
> -		 "%s\n", (block->bp_block >> 10),
> +		 "DASD with %u KB/block, %lu KB total size, %u KB/track, %s%s%s\n",
> +		 (block->bp_block >> 10),
>  		 (((unsigned long) private->real_cyl *
>  		   private->rdc_data.trk_per_cyl *
>  		   blk_per_trk * (block->bp_block >> 9)) >> 1),
>  		 ((blk_per_trk * block->bp_block) >> 10),
>  		 private->uses_cdl ?
> -		 "compatible disk layout" : "linux disk layout");
> +		 "compatible disk layout" : "linux disk layout",
> +		 ese_str, fmt_str);
>  
>  	return 0;
>  }

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

[PATCH 18/19] s390/dasd: Re-enable discard support for ESE volumes

[Stefan Haberland]

Re-enable block-layer discard for ESE ECKD volumes, releasing thin space
via release allocated space (RAS).
This is based on
commit 7e64db1597fe ("s390/dasd: Add discard support for ESE volumes")
but adapted to the current code and fixed.

REQ_OP_DISCARD is routed to a RAS release over the request's track range,
and discard requests run on the base device only. Discard limits use extent
granularity via the disc_limits discipline hook so the block layer only
issues extent-aligned discards.

Discard is gated on the DASD_FEATURE_DISCARD device feature rather than a
per-discipline flag: the driver sets the feature when the volume is on ESE
hardware (i.e. RAS is available), and the block-layer setup enables discard
limits for a device that has it.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c      |  34 +++++--
 drivers/s390/block/dasd_eckd.c | 173 ++++++++++++++++++++++++++-------
 drivers/s390/block/dasd_int.h  |   2 +
 3 files changed, 166 insertions(+), 43 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 18f5f097d2b3..5979758311c8 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -354,17 +354,19 @@ static int dasd_state_basic_to_ready(struct dasd_device *device)
 	 */
 	lim.dma_alignment = lim.logical_block_size - 1;
 
-	if (device->discipline->has_discard) {
+	if (device->features & DASD_FEATURE_DISCARD) {
 		unsigned int max_bytes;
 
-		lim.discard_granularity = block->bp_block;
-
-		/* Calculate max_discard_sectors and make it PAGE aligned */
-		max_bytes = USHRT_MAX * block->bp_block;
-		max_bytes = ALIGN_DOWN(max_bytes, PAGE_SIZE);
-
-		lim.max_hw_discard_sectors = max_bytes / block->bp_block;
-		lim.max_write_zeroes_sectors = lim.max_hw_discard_sectors;
+		if (device->discipline->disc_limits) {
+			device->discipline->disc_limits(block, &lim);
+		} else {
+			lim.discard_granularity = block->bp_block;
+			/* Calculate max_discard_sectors and make it PAGE aligned */
+			max_bytes = USHRT_MAX * block->bp_block;
+			max_bytes = ALIGN_DOWN(max_bytes, PAGE_SIZE);
+			lim.max_hw_discard_sectors = max_bytes / block->bp_block;
+			lim.max_write_zeroes_sectors = lim.max_hw_discard_sectors;
+		}
 	}
 	rc = queue_limits_commit_update(block->gdp->queue, &lim);
 	if (rc)
@@ -3136,6 +3138,7 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 	struct dasd_device *basedev;
 	struct dasd_ccw_req *cqr;
 	blk_status_t rc = BLK_STS_OK;
+	bool complete_noop = false;
 
 	basedev = block->base;
 	spin_lock_irq(&dq->lock);
@@ -3184,6 +3187,17 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 			rc = BLK_STS_RESOURCE;
 		} else if (PTR_ERR(cqr) == -EINVAL) {
 			rc = BLK_STS_INVAL;
+		} else if (PTR_ERR(cqr) == -EOPNOTSUPP) {
+			/*
+			 * A discard that covers no whole extent releases
+			 * nothing. Discard is advisory, so complete it as a
+			 * benign no-op: the device does support discard, this
+			 * range just does not align to the large ESE extent
+			 * granularity.
+			 * Completed after the lock is dropped.
+			 */
+			rc = BLK_STS_OK;
+			complete_noop = true;
 		} else {
 			DBF_DEV_EVENT(DBF_ERR, basedev,
 				      "CCW creation failed (rc=%ld) on request %p",
@@ -3217,6 +3231,8 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
 
 out:
 	spin_unlock_irq(&dq->lock);
+	if (complete_noop)
+		blk_mq_end_request(req, BLK_STS_OK);
 	return rc;
 }
 
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 3c4fcfb1558d..5c1f328b7c8d 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2327,6 +2327,18 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 	/* Read Volume Information */
 	dasd_eckd_read_vol_info(device);
 
+	/*
+	 * Advertise discard through the device feature so the block layer sets
+	 * up discard limits. Discard releases allocated space, so require a thin
+	 * (ESE) volume whose storage reports support for the space-release
+	 * function. Raw-track access bypasses the normal block CCW path (discard
+	 * would reach the raw builder, which has no record data), so exclude it.
+	 */
+	if (dasd_eckd_ese_capable(device) &&
+	    (private->features.feature[56] & 0x01) &&
+	    !(device->features & DASD_FEATURE_USERAW))
+		device->features |= DASD_FEATURE_DISCARD;
+
 	/* Read the on-disk format label for ESE detection */
 	dasd_eckd_read_format_label(device);
 
@@ -4136,37 +4148,13 @@ static int dasd_eckd_ras_sanity_checks(struct dasd_device *device,
 }
 
 /*
- * Helper function to count the amount of involved extents within a given range
- * with extent alignment in mind.
+ * Number of extents the track range [from, to] spans. Extent n covers tracks
+ * [n * trks_per_ext, (n + 1) * trks_per_ext - 1], so the range touches the
+ * extents from (from / trks_per_ext) to (to / trks_per_ext) inclusive.
  */
 static int count_exts(unsigned int from, unsigned int to, int trks_per_ext)
 {
-	int cur_pos = 0;
-	int count = 0;
-	int tmp;
-
-	if (from == to)
-		return 1;
-
-	/* Count first partial extent */
-	if (from % trks_per_ext != 0) {
-		tmp = from + trks_per_ext - (from % trks_per_ext) - 1;
-		if (tmp > to)
-			tmp = to;
-		cur_pos = tmp - from + 1;
-		count++;
-	}
-	/* Count full extents */
-	if (to - (from + cur_pos) + 1 >= trks_per_ext) {
-		tmp = to - ((to - trks_per_ext + 1) % trks_per_ext);
-		count += (tmp - (from + cur_pos) + 1) / trks_per_ext;
-		cur_pos = tmp;
-	}
-	/* Count last partial extent */
-	if (cur_pos < to)
-		count++;
-
-	return count;
+	return to / trks_per_ext - from / trks_per_ext + 1;
 }
 
 static int dasd_in_copy_relation(struct dasd_device *device)
@@ -4217,9 +4205,17 @@ dasd_eckd_dso_ras(struct dasd_device *device, struct dasd_block *block,
 	if (dasd_eckd_ras_sanity_checks(device, first_trk, last_trk))
 		return ERR_PTR(-EINVAL);
 
-	copy_relation = dasd_in_copy_relation(device);
-	if (copy_relation < 0)
-		return ERR_PTR(copy_relation);
+	/*
+	 * The block-layer discard path (req != NULL) runs in atomic context, so
+	 * it must not issue the sleeping copy-relation (PPRC) query. It also
+	 * leaves guarantee_init off - discard does not promise zeroing anyway.
+	 */
+	copy_relation = 0;
+	if (!req) {
+		copy_relation = dasd_in_copy_relation(device);
+		if (copy_relation < 0)
+			return ERR_PTR(copy_relation);
+	}
 
 	rq = req ? blk_mq_rq_to_pdu(req) : NULL;
 
@@ -4251,7 +4247,7 @@ dasd_eckd_dso_ras(struct dasd_device *device, struct dasd_block *block,
 	 * not fully specified, but is only supported with a certain feature
 	 * subset and for devices not in a copy relation.
 	 */
-	if (features->feature[56] & 0x01 && !copy_relation)
+	if (!req && features->feature[56] & 0x01 && !copy_relation)
 		ras_data->op_flags.guarantee_init = 1;
 
 	ras_data->lss = private->conf.ned->ID;
@@ -4347,6 +4343,9 @@ static int dasd_eckd_release_space_trks(struct dasd_device *device,
 
 	INIT_LIST_HEAD(&ras_queue);
 
+	if (dasd_eckd_ext_size(device) == 0)
+		return -EINVAL;
+
 	device_exts = private->real_cyl / dasd_eckd_ext_size(device);
 	trks_per_ext = dasd_eckd_ext_size(device) * private->rdc_data.trk_per_cyl;
 
@@ -5484,6 +5483,58 @@ dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
 	return ERR_PTR(ret);
 }
 
+static struct dasd_ccw_req *
+dasd_eckd_build_cp_discard(struct dasd_device *device, struct dasd_block *block,
+			   struct request *req, sector_t first_trk,
+			   sector_t last_trk, unsigned int first_offs,
+			   unsigned int last_offs, unsigned int blk_per_trk)
+{
+	struct dasd_eckd_private *private = device->private;
+	sector_t first_ext_trk, last_ext_end, last_ext_trk;
+	unsigned int trks_per_ext;
+
+	trks_per_ext = dasd_eckd_ext_size(device) * private->rdc_data.trk_per_cyl;
+	if (!trks_per_ext)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	/*
+	 * A discard range is rarely track-aligned: fstrim is FS-block granular
+	 * and discard_granularity is only a hint. If it starts or ends mid-track,
+	 * that boundary track still holds live records outside the range, so drop
+	 * it from the whole-track span first. Otherwise a partial boundary track
+	 * that happens to sit on an extent boundary would be released together
+	 * with its live records resulting in silent data loss
+	 */
+	if (first_offs)				/* partial first track */
+		first_trk++;
+	if (last_offs != blk_per_trk - 1) {	/* partial last track */
+		if (!last_trk)
+			return ERR_PTR(-EOPNOTSUPP);
+		last_trk--;
+	}
+	if (first_trk > last_trk)
+		return ERR_PTR(-EOPNOTSUPP);	/* no whole track fully covered */
+
+	/*
+	 * RAS releases whole extents. Only release extents that lie entirely
+	 * within the (now whole-track) discard range by rounding inward to extent
+	 * boundaries - an extent shared with a live allocation must never be
+	 * released. If no whole extent is covered there is nothing to release
+	 * safely (e.g. a sub-extent discard, unavoidable with large extents), so
+	 * reject the request rather than release too much.
+	 */
+	first_ext_trk = roundup(first_trk, trks_per_ext);
+	/* one past the last whole extent inside the range (exclusive) */
+	last_ext_end = rounddown(last_trk + 1, trks_per_ext);
+	if (first_ext_trk >= last_ext_end)
+		return ERR_PTR(-EOPNOTSUPP);
+	/* inclusive last track; the guard above keeps this from underflowing */
+	last_ext_trk = last_ext_end - 1;
+
+	return dasd_eckd_dso_ras(device, block, req, first_ext_trk,
+				 last_ext_trk, 1);
+}
+
 static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 					       struct dasd_block *block,
 					       struct request *req)
@@ -5522,6 +5573,12 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
 	last_offs = sector_div(last_trk, blk_per_trk);
 	cdlspecial = (private->uses_cdl && first_rec < 2*blk_per_trk);
 
+	if (req_op(req) == REQ_OP_DISCARD)
+		return dasd_eckd_build_cp_discard(startdev, block, req,
+						  first_trk, last_trk,
+						  first_offs, last_offs,
+						  blk_per_trk);
+
 	fcx_multitrack = private->features.feature[40] & 0x20;
 	data_size = blk_rq_bytes(req);
 	if (data_size % blksize || data_size == 0)
@@ -5835,11 +5892,13 @@ static struct dasd_ccw_req *dasd_eckd_build_alias_cp(struct dasd_device *base,
 						     struct request *req)
 {
 	struct dasd_eckd_private *private;
-	struct dasd_device *startdev;
+	struct dasd_device *startdev = NULL;
 	unsigned long flags;
 	struct dasd_ccw_req *cqr;
 
-	startdev = dasd_alias_get_start_dev(base);
+	/* Discard requests (space release) can only run on the base device. */
+	if (req_op(req) != REQ_OP_DISCARD)
+		startdev = dasd_alias_get_start_dev(base);
 	if (!startdev)
 		startdev = base;
 	private = startdev->private;
@@ -7727,6 +7786,51 @@ static unsigned int dasd_eckd_max_sectors(struct dasd_block *block)
 	return DASD_ECKD_MAX_BLOCKS << block->s2b_shift;
 }
 
+/*
+ * Discard on ECKD releases space through RAS, which works on whole extents.
+ * Advertise extent granularity so the block layer only sends extent-aligned
+ * discards (avoiding partially specified extents), and only for volumes on ESE
+ * hardware. Non-ESE devices are left without discard limits.
+ */
+static void dasd_eckd_disc_limits(struct dasd_block *block,
+				  struct queue_limits *lim)
+{
+	struct dasd_device *device = block->base;
+	struct dasd_eckd_private *private = device->private;
+	unsigned int logical_block_size = block->bp_block;
+	unsigned int max_discard_sectors, max_bytes, ext_bytes;
+	int recs_per_trk, trks_per_cyl, ext_limit, ext_size;
+
+	if (!dasd_eckd_ese_capable(device) || dasd_eckd_ext_size(device) == 0)
+		return;
+
+	trks_per_cyl = private->rdc_data.trk_per_cyl;
+	recs_per_trk = recs_per_track(&private->rdc_data, 0, logical_block_size);
+
+	ext_size = dasd_eckd_ext_size(device);
+	ext_limit = min(private->real_cyl / ext_size, DASD_ECKD_RAS_EXTS_MAX);
+	ext_bytes = ext_size * trks_per_cyl * recs_per_trk * logical_block_size;
+	if (!ext_bytes)		/* malformed RDC data - leave discard unset */
+		return;
+	max_bytes = UINT_MAX - (UINT_MAX % ext_bytes);
+	if (max_bytes / ext_bytes > ext_limit)
+		max_bytes = ext_bytes * ext_limit;
+
+	max_discard_sectors = max_bytes / 512;
+
+	lim->max_hw_discard_sectors = max_discard_sectors;
+	/*
+	 * ext_bytes is the hardware extent size and is not a power of two, so
+	 * the block layer's power-of-two round_up()/round_down() alignment
+	 * helpers compute it only approximately. That is a hint, not a
+	 * correctness requirement: RAS safety is enforced in the CCW builder,
+	 * which rounds the range inward to whole extents and rejects a request
+	 * that covers no whole extent, so a misaligned range is never
+	 * over-released. At worst a few sub-extent discards are declined.
+	 */
+	lim->discard_granularity = ext_bytes;
+}
+
 static struct ccw_driver dasd_eckd_driver = {
 	.driver = {
 		.name	= "dasd-eckd",
@@ -7749,6 +7853,7 @@ static struct dasd_discipline dasd_eckd_discipline = {
 	.owner = THIS_MODULE,
 	.name = "ECKD",
 	.ebcname = "ECKD",
+	.disc_limits = dasd_eckd_disc_limits,
 	.check_device = dasd_eckd_check_characteristics,
 	.uncheck_device = dasd_eckd_uncheck_device,
 	.do_analysis = dasd_eckd_do_analysis,
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 8c73850f7947..ef4930432c09 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -404,6 +404,8 @@ struct dasd_discipline {
 	int (*ese_capable)(struct dasd_device *);
 	/* Whether the volume is formatted on demand (thin), from the label */
 	int (*on_demand_format)(struct dasd_device *);
+	/* Fill discard queue limits */
+	void (*disc_limits)(struct dasd_block *, struct queue_limits *);
 	/* Capacity */
 	int (*space_allocated)(struct dasd_device *);
 	int (*space_configured)(struct dasd_device *);
-- 
2.53.0

Re: [PATCH 18/19] s390/dasd: Re-enable discard support for ESE volumes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Setting discard_granularity to a non-power-of-two value breaks block layer arithmetic and causes discards to fail silently.
--

--- Patch [18]: [PATCH 18/19] s390/dasd: Re-enable discard support for ESE volumes ---
commit 89798f460c12b21c7a8a68cccddbe7f634389280
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Re-enable discard support for ESE volumes

This commit re-enables block-layer discard for ESE ECKD volumes. It routes
discard operations to a release allocated space (RAS) function, using extent
granularity for discard limits and gating discard support on the
DASD_FEATURE_DISCARD device feature.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 18f5f097d2b3d..5979758311c80 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3184,6 +3187,17 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
>  			rc = BLK_STS_RESOURCE;
>  		} else if (PTR_ERR(cqr) == -EINVAL) {
>  			rc = BLK_STS_INVAL;
> +		} else if (PTR_ERR(cqr) == -EOPNOTSUPP) {
> +			/*
> +			 * A discard that covers no whole extent releases
> +			 * nothing. Discard is advisory, so complete it as a
> +			 * benign no-op: the device does support discard, this
> +			 * range just does not align to the large ESE extent
> +			 * granularity.
> +			 * Completed after the lock is dropped.
> +			 */
> +			rc = BLK_STS_OK;
> +			complete_noop = true;

[Severity: High]
Will the misaligned discard requests produced by the alignment math be
silently dropped here?

Because the block layer might split the requests into smaller pieces that do
not cover whole extents, they are caught here and completed with BLK_STS_OK.
This gives the illusion of success to the caller, but substantial portions of
the disk might never actually be freed.

[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 3c4fcfb1558de..5c1f328b7c8d7 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -7727,6 +7786,51 @@ static unsigned int dasd_eckd_max_sectors(struct dasd_block *block)
>  	return DASD_ECKD_MAX_BLOCKS << block->s2b_shift;
>  }
>  
> +/*
> + * Discard on ECKD releases space through RAS, which works on whole extents.
> + * Advertise extent granularity so the block layer only sends extent-aligned
> + * discards (avoiding partially specified extents), and only for volumes on ESE
> + * hardware. Non-ESE devices are left without discard limits.
> + */
> +static void dasd_eckd_disc_limits(struct dasd_block *block,
> +				  struct queue_limits *lim)
> +{
[ ... ]
> +	/*
> +	 * ext_bytes is the hardware extent size and is not a power of two, so
> +	 * the block layer's power-of-two round_up()/round_down() alignment
> +	 * helpers compute it only approximately. That is a hint, not a
> +	 * correctness requirement: RAS safety is enforced in the CCW builder,
> +	 * which rounds the range inward to whole extents and rejects a request
> +	 * that covers no whole extent, so a misaligned range is never
> +	 * over-released. At worst a few sub-extent discards are declined.
> +	 */
> +	lim->discard_granularity = ext_bytes;
> +}

[Severity: High]
Does setting discard_granularity to a non-power-of-two value break the block
layer alignment arithmetic?

The block layer uses round_up() and round_down() in include/linux/math.h to
handle granularity boundaries, which operate using a bitwise mask (y - 1).
Because the hardware extent size (ext_bytes) is not a power of two, this
bitmask is not a contiguous string of 1s.

This results in pseudo-random alignment boundaries, where the block layer
subsequently emits alternating 1MB and randomly-sized discard requests. Since
1MB is smaller than a single ECKD extent (which is typically around 15MB),
these discards will not cover whole extents and will be rejected by
dasd_eckd_build_cp_discard() with -EOPNOTSUPP.

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

[PATCH 19/19] s390/dasd: Read cached unit address and LSS in the CCW build path

[Stefan Haberland]

The CCW build path (prefix_LRE, the full-track prefix and dso_ras) read the
base address and LSS straight from conf.ned. That buffer is freed and
reallocated by the reload worker (do_reload_device - dasd_eckd_read_conf -
dasd_eckd_clear_conf_data), so a configuration change concurrent with I/O
can free conf.ned while a request is being built.
Use-after-free reported by KASAN in prefix_LRE.

Read the cached copies instead.
The unit address is already kept in uid.real_unit_addr, and the LSS is now
cached in ned_lss. Both are refreshed under the ccwdev lock in
dasd_eckd_generate_uid whenever the configuration is (re)read.
Also fix for prepare for read subsystem data (prssd) users.

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 26 ++++++++++++++++----------
 drivers/s390/block/dasd_eckd.h |  8 ++++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 5c1f328b7c8d..fd23ac25a7da 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -589,8 +589,9 @@ static int prefix_LRE(struct ccw1 *ccw, struct PFX_eckd_data *pfxdata,
 		return -EINVAL;
 	}
 	pfxdata->format = format;
-	pfxdata->base_address = basepriv->conf.ned->unit_addr;
-	pfxdata->base_lss = basepriv->conf.ned->ID;
+	/* cached copies - conf.ned may be freed under us by the reload worker */
+	pfxdata->base_address = READ_ONCE(basepriv->ned_ua);
+	pfxdata->base_lss = READ_ONCE(basepriv->ned_lss);
 	pfxdata->validity.define_extent = 1;
 
 	/* private uid is kept up to date, conf_data may be outdated */
@@ -807,6 +808,9 @@ static int dasd_eckd_generate_uid(struct dasd_device *device)
 		return -ENODEV;
 	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
 	create_uid(&private->conf, &private->uid);
+	/* cache LSS and unit address for the lockless CCW-build path */
+	WRITE_ONCE(private->ned_lss, private->conf.ned->ID);
+	WRITE_ONCE(private->ned_ua, private->conf.ned->unit_addr);
 	spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
 	return 0;
 }
@@ -1632,8 +1636,8 @@ static int dasd_eckd_read_vol_info(struct dasd_device *device)
 	prssdp = cqr->data;
 	prssdp->order = PSF_ORDER_PRSSD;
 	prssdp->suborder = PSF_SUBORDER_VSQ;	/* Volume Storage Query */
-	prssdp->lss = private->conf.ned->ID;
-	prssdp->volume = private->conf.ned->unit_addr;
+	prssdp->lss = READ_ONCE(private->ned_lss);
+	prssdp->volume = READ_ONCE(private->ned_ua);
 
 	ccw = cqr->cpaddr;
 	ccw->cmd_code = DASD_ECKD_CCW_PSF;
@@ -4250,8 +4254,9 @@ dasd_eckd_dso_ras(struct dasd_device *device, struct dasd_block *block,
 	if (!req && features->feature[56] & 0x01 && !copy_relation)
 		ras_data->op_flags.guarantee_init = 1;
 
-	ras_data->lss = private->conf.ned->ID;
-	ras_data->dev_addr = private->conf.ned->unit_addr;
+	/* cached copies - conf.ned may be freed under us by the reload worker */
+	ras_data->lss = READ_ONCE(private->ned_lss);
+	ras_data->dev_addr = READ_ONCE(private->ned_ua);
 	ras_data->nr_exts = nr_exts;
 
 	if (by_extent) {
@@ -4822,8 +4827,9 @@ static int prepare_itcw(struct itcw *itcw,
 	lredata = &pfxdata->locate_record;
 
 	pfxdata->format = 1; /* PFX with LRE */
-	pfxdata->base_address = basepriv->conf.ned->unit_addr;
-	pfxdata->base_lss = basepriv->conf.ned->ID;
+	/* cached copies - conf.ned may be freed under us by the reload worker */
+	pfxdata->base_address = READ_ONCE(basepriv->ned_ua);
+	pfxdata->base_lss = READ_ONCE(basepriv->ned_lss);
 	pfxdata->validity.define_extent = 1;
 
 	/* private uid is kept up to date, conf_data may be outdated */
@@ -6905,8 +6911,8 @@ static int dasd_eckd_query_host_access(struct dasd_device *device,
 	prssdp->order = PSF_ORDER_PRSSD;
 	prssdp->suborder = PSF_SUBORDER_QHA;	/* query host access */
 	/* LSS and Volume that will be queried */
-	prssdp->lss = private->conf.ned->ID;
-	prssdp->volume = private->conf.ned->unit_addr;
+	prssdp->lss = READ_ONCE(private->ned_lss);
+	prssdp->volume = READ_ONCE(private->ned_ua);
 	/* all other bytes of prssdp must be zero */
 
 	ccw = cqr->cpaddr;
diff --git a/drivers/s390/block/dasd_eckd.h b/drivers/s390/block/dasd_eckd.h
index 30745f62402b..8e6f09e9ca7e 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -736,6 +736,14 @@ struct dasd_eckd_private {
 
 	/* alias management */
 	struct dasd_uid uid;
+	/*
+	 * Cached copies of conf.ned->ID (the LSS) and conf.ned->unit_addr,
+	 * refreshed under ccwdev_lock. Kept outside uid because create_uid()
+	 * memsets uid before repopulating it, which would expose a transient
+	 * zero to the lockless CCW-build readers.
+	 */
+	__u8 ned_lss;
+	__u8 ned_ua;
 	struct alias_pav_group *pavgroup;
 	struct alias_lcu *lcu;
 	int count;
-- 
2.53.0

Re: [PATCH 19/19] s390/dasd: Read cached unit address and LSS in the CCW build path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Lockless readers can observe a mismatched, torn state of `ned_lss` and `ned_ua` due to sequential 8-bit reads and writes.

Pre-existing issues:
- [Critical] Concurrent execution of path event handler and device reload leads to unlocked `memcpy` into freed `private->conf.data`.
- [High] sysfs `path_reset` attribute accesses `private->conf.gneq` without locks, racing with configuration data reallocation.
- [High] Concurrent execution of `dasd_eckd_generate_uid` and `dasd_eckd_read_conf` results in a Use-After-Free read of the dangling `private->conf.ned` pointer.
--

--- Patch [19]: [PATCH 19/19] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit 27a80ff5ec151de5f38d959cec701e81aa17aed0
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Read cached unit address and LSS in the CCW build path

The CCW build path read the base address and LSS straight from conf.ned,
which could be freed by the reload worker. This patch caches unit address
and LSS in the private structure and reads the cached copies to prevent
use-after-free.

>     The CCW build path (prefix_LRE, the full-track prefix and dso_ras) read the
>     base address and LSS straight from conf.ned. That buffer is freed and
>     reallocated by the reload worker (do_reload_device - dasd_eckd_read_conf -
>     dasd_eckd_clear_conf_data), so a configuration change concurrent with I/O
>     can free conf.ned while a request is being built.

[Severity: Critical]
This is a pre-existing issue, but does concurrent execution of the path event
handler and device reload lead to an unlocked memcpy into freed memory?

The path event worker do_pe_handler_work() calls rebuild_device_uid(), which
copies configuration data using:

memcpy(private->conf.data, data->rcd_buffer, DASD_ECKD_RCD_DATA_SIZE);

Concurrently, do_reload_device() calls dasd_eckd_clear_conf_data(), which
explicitly sets private->conf.data = NULL and frees the buffer.

Neither path acquires ccwdev_lock or state_mutex. Can this cause the memcpy
to write into freed heap memory or dereference a NULL destination pointer?

[Severity: High]
This is another pre-existing issue, but does the sysfs path_reset attribute
access private->conf.gneq without locks, racing with configuration data
reallocation?

The sysfs store function dasd_path_reset_store() calls dasd_eckd_reset_path(),
which calls get_fcx_max_data(). This function reads
private->conf.gneq->reserved2[7] without acquiring any locks.

Since do_reload_device() can concurrently execute dasd_eckd_clear_conf_data()
and free the memory block that conf.gneq points into, while failing to set
conf.gneq to NULL, can get_fcx_max_data() dereference a dangling pointer and
cause a use-after-free read?

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 5c1f328b7c8d7..fd23ac25a7da9 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -589,8 +589,9 @@ static int prefix_LRE(struct ccw1 *ccw, struct PFX_eckd_data *pfxdata,
>  		return -EINVAL;
>  	}
>  	pfxdata->format = format;
> -	pfxdata->base_address = basepriv->conf.ned->unit_addr;
> -	pfxdata->base_lss = basepriv->conf.ned->ID;
> +	/* cached copies - conf.ned may be freed under us by the reload worker */
> +	pfxdata->base_address = READ_ONCE(basepriv->ned_ua);
> +	pfxdata->base_lss = READ_ONCE(basepriv->ned_lss);

[Severity: High]
Can lockless readers observe a mismatched, torn state of ned_lss and ned_ua due
to sequential 8-bit reads?

Since ned_lss and ned_ua are separate 8-bit variables, they are read
sequentially here.

If a concurrent configuration change updates both variables, could a lockless
reader be preempted between the two reads and observe a torn state, such as
the old LSS but the new unit address?

[ ... ]

> @@ -807,6 +808,9 @@ static int dasd_eckd_generate_uid(struct dasd_device *device)
>  		return -ENODEV;
>  	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
>  	create_uid(&private->conf, &private->uid);
> +	/* cache LSS and unit address for the lockless CCW-build path */
> +	WRITE_ONCE(private->ned_lss, private->conf.ned->ID);
> +	WRITE_ONCE(private->ned_ua, private->conf.ned->unit_addr);

[Severity: High]
This is a pre-existing issue, but does concurrent execution of
dasd_eckd_generate_uid() and dasd_eckd_read_conf() result in a use-after-free
read of the dangling private->conf.ned pointer here?

Since dasd_eckd_clear_conf_data() frees the configuration data buffer but
fails to nullify the derived private->conf.ned and private->conf.gneq pointers,
the lockless check for !private->conf.ned at the start of this function passes
because the pointer is dangling rather than NULL.

Does this cause the code to acquire ccwdev_lock and access freed memory inside
create_uid() and these newly added WRITE_ONCE() statements?

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

Re: [PATCH 00/19] s390/dasd: ESE Performance improvements

[Jens Axboe]

On Wed, 05 Aug 2026 13:15:53 +0200, Stefan Haberland wrote:
> please apply the patch series for the upcoming merge window.
> It applies onto your for-next branch.
> 
> The series significantly improves the performance and overall usability of
> Extent Space Efficient (ESE, aka thin provisioned) DASD devices.
> 
> On a freshly provisioned ESE volume I get the following throughput
> improvements, depending on workload and setup:
> 
> [...]

Applied, thanks!

[01/19] s390/dasd: Do not complete a failed ESE read as successful
        commit: cddb447c62466f3076938ce120028d7b591f9f37
[02/19] s390/dasd: Propagate partial completion length across ERP recovery
        commit: 6fb5ba2e7e43173a3761e46f091070a8185efa14
[03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data
        commit: 2a1780f9fc2493bd34c418a0be6fc58943afcecf
[04/19] s390/dasd: Snapshot intrc before freeing the request block
        commit: dc3e3f7306cb74066f231251602b1be5aaec7bd8
[05/19] s390/dasd: Optimize max blocks per request for track alignment
        commit: e647f351da2c6601a3e280b5c3477421e981ca73
[06/19] s390/dasd: Use GFP_KERNEL in dasd_alloc_device()
        commit: feea12d1cd8110aac009df32d3c6cd1daf117f95
[07/19] s390/dasd: Add defines for the Extended Address Volume track address
        commit: 1edac73fa2e9e8cadf19d59a49d64c681e4e45a3
[08/19] s390/dasd: Add infrastructure for ESE full-track write
        commit: 9cebfced13249fb02061dbee8a29cf91ba364519
[09/19] s390/dasd: Add range-based format-track collision detection
        commit: 05697849292011dc828b3651c159cfe694d1b5ef
[10/19] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
        commit: 44f9bf47987ec25a90d46d171b8df847d773f033
[11/19] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
        commit: 123ec1e9cb4200308ad1535c84a4cf2f81904517
[12/19] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
        commit: 791d257a21ba5241ce723a6c17f395aa0d419982
[13/19] s390/dasd: Add full_track_bias to control fulltrack write mode
        commit: 42849375e9280f2ccf492ced782e9591910d6b47
[14/19] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
        commit: 4e304b2e56f09bce314ffeb4d4033ef7b8776c30
[15/19] s390/dasd: Stamp a format label into newly formatted volumes
        commit: 7d206efdc2f29240cfd815ffebfa958e2295a63e
[16/19] s390/dasd: Detect ESE volumes from the on-disk format label
        commit: 268e40548da2758851b6962a0a4ed2ab241ddbe9
[17/19] s390/dasd: Report ESE capability and format mode at device online
        commit: 6c1be943fdb6f8557596f5164ea48ae65cd0c1df
[18/19] s390/dasd: Re-enable discard support for ESE volumes
        commit: 04ea1579bc7707366de1d642115ad3b65c6171e1
[19/19] s390/dasd: Read cached unit address and LSS in the CCW build path
        commit: a600051da14b4cacc7b00685c967f40b0425ef5b

Best regards,
-- 
Jens Axboe