* [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 15:50 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data Stefan Haberland
` (15 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery
2026-07-31 15:38 ` [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery Stefan Haberland
@ 2026-07-31 15:50 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 15:50 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Heiko Carstens, linux-s390, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The return value of `ese_read()` is completely ignored, leading to silent data corruption on read failures.
--
--- Patch [1]: [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery ---
commit 804bfccaf59e282c990c294a7564d63b4f3b75a4
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Propagate partial completion length across ERP recovery
This patch updates dasd_default_erp_postaction() to copy the proc_bytes
field from the finished ERP request back to the original request. This
prevents partially completed requests from losing their completion length
during ERP recovery, avoiding silent data corruption for thin provisioned
devices.
> 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;
While reviewing this fix for thin provisioned devices, I noticed the following
in dasd_int_handler():
drivers/s390/block/dasd.c:dasd_int_handler() {
...
if (rq_data_dir(req) == READ) {
device->discipline->ese_read(cqr, irb);
cqr->status = DASD_CQR_SUCCESS;
cqr->stopclk = now;
dasd_device_clear_timer(device);
dasd_schedule_device_bh(device);
return;
}
...
}
[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but does this code ignore the return value of ese_read()?
If ese_read() encounters an error and fails to zero out the block buffer, the
request still unconditionally completes with DASD_CQR_SUCCESS.
Could this result in the block layer being handed uninitialized memory or
previous page contents instead of zeroed data?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
2026-07-31 15:38 ` [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 15:59 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 03/17] s390/dasd: Optimize max blocks per request for track alignment Stefan Haberland
` (14 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_eckd.c | 38 +++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 073795ea7cd0..5593013c9db2 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1491,6 +1491,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);
@@ -1646,6 +1648,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;
}
@@ -1653,6 +1658,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;
}
@@ -1666,6 +1674,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;
@@ -1680,6 +1691,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;
@@ -1689,6 +1703,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;
}
@@ -1831,7 +1848,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;
@@ -1847,6 +1868,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;
}
@@ -1854,6 +1878,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;
}
@@ -1864,6 +1891,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;
}
@@ -5935,6 +5965,9 @@ 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)
return -EOPNOTSUPP;
@@ -6801,6 +6834,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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data
2026-07-31 15:38 ` [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data Stefan Haberland
@ 2026-07-31 15:59 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 15:59 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, linux-s390, Vasily Gorbik, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The patch fails to fully resolve the initialization race for the `host_access_count` sysfs attribute, resulting in a NULL pointer dereference. While it adds a `!private` check to `dasd_eckd_query_host_access()`, it overlooks that `private->lcu` remains NULL until populated later by `dasd_alias_make_device_known_to_lcu()`. An unprivileged sysfs read during this window will bypass the `!private` check and unconditionally dereference `private->lcu->pav`, causing a kernel panic.
- [Critical] The `if (!private)` checks added by the patch fail to protect against concurrent freeing of `device->private` during device initialization errors, leading to a Use-After-Free and memory corruption. If `dasd_eckd_check_characteristics` fails, its error path frees `device->private`. A concurrent unprivileged sysfs read can fetch the non-NULL pointer just before it is freed, bypass the check, and subsequently dereference or write to the freed memory.
- [High] Missing NULL pointer checks in `dasd_copy_pair_store` and `dasd_eckd_pprc_enabled` allow a kernel panic when configuring copy pairs via sysfs during device initialization. The sysfs store handler `dasd_copy_pair_store` does not verify if `device->discipline` is allocated before dereferencing `device->discipline->pprc_enabled`. Furthermore, if `device->discipline` is assigned but `device->private` is not yet allocated, `dasd_eckd_pprc_enabled` unconditionally dereferences `device->private`, resulting in a kernel panic.
- [High] Missing NULL pointer check for `device->private` in `dasd_eckd_read_fc_security()`, leading to a kernel panic triggered by asynchronous path events during initialization. If a hardware path event occurs after `device->discipline` is assigned but before `device->private` is allocated, `do_pe_handler_work()` executes and calls `dasd_eckd_read_fc_security()`, which unconditionally dereferences `private`.
--
--- Patch [2]: [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit d022f6e90af16afe7ae1766d9ba490832f14cca6
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 74fe73b5738ab..a820d68122d93 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -1666,6 +1674,9 @@ static int dasd_eckd_space_configured(struct dasd_device *device)
> struct dasd_eckd_private *private = device->private;
> int rc;
>
> + if (!private)
> + return 0;
> +
[Severity: Critical]
This is a pre-existing issue, but does this check protect against concurrent
freeing of device->private during device initialization errors?
If dasd_eckd_check_characteristics() fails, its error path frees
device->private without synchronization against concurrent lockless sysfs
readers:
kfree(device->private);
device->private = NULL;
Could a concurrent unprivileged sysfs read fetch the non-NULL pointer, pass
this check, and then perform a Use-After-Free when dasd_eckd_read_vol_info()
copies data into the freed private->vsq buffer?
> rc = dasd_eckd_read_vol_info(device);
[ ... ]
> @@ -5935,6 +5965,9 @@ 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)
[Severity: Critical]
This is a pre-existing issue, but could this still lead to a NULL pointer
dereference?
While this check ensures private is not NULL, private->lcu can remain NULL
until it is populated later by dasd_alias_make_device_known_to_lcu().
If an unprivileged sysfs read triggers dasd_eckd_host_access_count() which
calls dasd_eckd_query_host_access() during this window, wouldn't it bypass
the !private check and unconditionally dereference private->lcu->pav?
> return -EOPNOTSUPP;
[Severity: High]
This is a pre-existing issue, but I noticed a couple of other places that
might need similar NULL pointer checks.
In dasd_copy_pair_store() in drivers/s390/block/dasd_devmap.c, could
device->discipline be dereferenced while it is NULL when a user writes to
the copy_pair sysfs attribute during device initialization?
if (device->discipline->pprc_enabled) {
Additionally, if device->discipline is set but device->private is not yet
allocated, could dasd_eckd_pprc_enabled() in drivers/s390/block/dasd_eckd.c
unconditionally dereference device->private?
return private->rdc_data.facilities.PPRC_enabled;
[Severity: High]
This is also a pre-existing issue, but is a NULL check for device->private
needed in dasd_eckd_read_fc_security() as well?
If a hardware path event occurs after device->discipline is assigned but
before device->private is allocated, do_pe_handler_work() executes and calls
dasd_eckd_read_fc_security().
Could this unconditionally dereference private?
rc = chsc_scud(private->uid.ssid, (u64 *)esm, &esm_valid);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 03/17] s390/dasd: Optimize max blocks per request for track alignment
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
2026-07-31 15:38 ` [PATCH v5 01/17] s390/dasd: Propagate partial completion length across ERP recovery Stefan Haberland
2026-07-31 15:38 ` [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 15:46 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device() Stefan Haberland
` (13 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device()
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (2 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 03/17] s390/dasd: Optimize max blocks per request for track alignment Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 15:52 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 05/17] s390/dasd: Add defines for the Extended Address Volume track address Stefan Haberland
` (12 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 dc1c6d5ab3be..d696fab0807a 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device()
2026-07-31 15:38 ` [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device() Stefan Haberland
@ 2026-07-31 15:52 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 15:52 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, linux-s390, Alexander Gordeev, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing timer synchronization in teardown allows the device timeout handler to run concurrently with or after the device is freed.
--
--- Patch [4]: [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device() ---
commit 476054dc9b8482264c4038c62c8c82f3d75106b4
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Use GFP_KERNEL in dasd_alloc_device()
This commit changes the allocation flags in dasd_alloc_device() from
GFP_ATOMIC to GFP_KERNEL. Since the function runs in process context,
using GFP_KERNEL is more reliable for large DMA allocations.
> 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
> @@ -86,25 +86,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);
[Severity: High]
This is a pre-existing issue, but does the device teardown path lack timer
synchronization for this allocated device?
During teardown, dasd_device_clear_timer() calls non-synchronous
timer_delete():
void dasd_device_clear_timer(struct dasd_device *device)
{
timer_delete(&device->timer);
}
Because this does not wait for a concurrently executing timer handler on
another CPU, and the timer does not acquire a reference on the device when
armed, could dasd_delete_device() complete its wait for the reference count
and free the device while dasd_device_timeout() is still running?
If dasd_device_timeout() executes on another CPU and accesses the device:
spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
Could this result in a use-after-free of the device and its associated
structures?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=4
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 05/17] s390/dasd: Add defines for the Extended Address Volume track address
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (3 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 04/17] s390/dasd: Use GFP_KERNEL in dasd_alloc_device() Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 15:48 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
` (11 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 5593013c9db2..0734a82a7ba1 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -199,8 +199,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
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (4 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 05/17] s390/dasd: Add defines for the Extended Address Volume track address Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:16 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection Stefan Haberland
` (10 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 | 93 ++++++++++++++++++++++++++++------
drivers/s390/block/dasd_eckd.c | 11 ++++
drivers/s390/block/dasd_eckd.h | 5 ++
drivers/s390/block/dasd_int.h | 19 +++++++
4 files changed, 113 insertions(+), 15 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index d696fab0807a..b8ecdb211139 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);
@@ -1883,6 +1923,10 @@ 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;
+ cqr->callback = NULL;
+ break;
default:
dev_err(&device->cdev->dev,
"Unexpected CQR status %02x", cqr->status);
@@ -2210,6 +2254,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);
@@ -2771,7 +2816,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) {
@@ -2882,6 +2928,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);
@@ -2941,6 +2995,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 0734a82a7ba1..e1a2c29d8a8b 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -204,6 +204,17 @@ static void set_ch_t(struct ch_t *geo, __u32 cyl, __u8 head)
geo->head |= head;
}
+static 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 99c253421653..0ca039126ae9 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write
2026-07-31 15:38 ` [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
@ 2026-07-31 16:16 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:16 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Alexander Gordeev, Vasily Gorbik, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Aborted origin CQRs (DASD_CQR_ABORTED) are erroneously completed to the block layer with an I/O error or requeued, causing double-completion or use-after-free.
--
--- Patch [6]: [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write ---
commit 6ca5a3ba40b43e7a20a1f06beb37ce4d8e6866d8
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add infrastructure for ESE full-track write
This commit adds driver internals to build WRITE_FULL_TRACK FCX channel
programs for unformatted tracks on ESE devices. It introduces new CQR states
(DASD_CQR_ABORT and DASD_CQR_ABORTED) intended to retire the origin CQR of a
replaced write without completing it to the block layer. It also expands
the CCW and ESE format pools to keep maximum-size requests in flight.
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3658a714abd62..d6ccf0186ada4 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -2759,7 +2804,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]
Will this cause aborted origin CQRs to be erroneously completed to the block
layer?
By allowing DASD_CQR_ABORTED to pass through here, the request is moved to
final_queue at the end of __dasd_process_block_ccw_queue().
Later in dasd_block_tasklet(), all requests in final_queue are unconditionally
passed to __dasd_cleanup_cqr():
dasd_block_tasklet() {
...
list_for_each_safe(l, n, &final_queue) {
cqr = list_entry(l, struct dasd_ccw_req, blocklist);
...
__dasd_cleanup_cqr(cqr);
...
}
}
Since __dasd_cleanup_cqr() lacks a specific check for DASD_CQR_ABORTED, it
will set the error to BLK_STS_IOERR and complete the request:
__dasd_cleanup_cqr() {
...
status = cqr->block->base->discipline->free_cp(cqr, req);
...
else if (status == 0) {
switch (cqr->intrc) {
...
default:
error = BLK_STS_IOERR;
break;
}
}
if (error)
blk_mq_end_request(req, error);
}
If the replacement format-CQR also attempts to complete or requeue this
same block request, could this lead to a double-completion or a
use-after-free in the block layer?
As a related question, does dasd_generic_requeue_all_requests() also need
to check if the CQR was aborted? It currently appears to requeue the block
request without checking for the aborted status:
dasd_generic_requeue_all_requests() {
...
_dasd_requeue_request(cqr);
list_del_init(&cqr->blocklist);
...
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=6
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (5 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 06/17] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:11 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK Stefan Haberland
` (9 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 | 60 ++++++++++++++++++++++------------
drivers/s390/block/dasd_int.h | 19 +++++++++--
3 files changed, 79 insertions(+), 29 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index b8ecdb211139..58867eb397e6 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);
@@ -2865,6 +2858,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)) {
+ 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 e1a2c29d8a8b..6577958fdc2d 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3150,32 +3150,43 @@ 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 && cqr->collision) {
+ 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 && 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);
@@ -3183,13 +3194,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);
}
@@ -3211,8 +3222,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;
@@ -3231,7 +3242,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);
@@ -3248,9 +3258,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);
@@ -3262,17 +3272,27 @@ 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) {
+ 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 0ca039126ae9..a7010f24eb41 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection
2026-07-31 15:38 ` [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection Stefan Haberland
@ 2026-07-31 16:11 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:11 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, Vasily Gorbik, linux-s390, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Removal of the `trkcount` snapshot check before its replacement is fully active leaves a window for silent data corruption, breaking bisectability.
--
--- Patch [7]: [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection ---
commit 758cbeac7b7b1d21b34dd4da47443455ac60838a
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add range-based format-track collision detection
This commit replaces the single per-device format_entry slot with an
array of 16 slots to allow multiple in-flight format requests. It also
rewrites the collision detection to use a range-based overlap check
instead of the previous trkcount snapshot mechanism.
(Note: The standard diff-quoting format has been altered to a free-form
summary due to automated recitation restrictions. Original patch code is
not directly quoted.)
[Severity: Critical]
Does removing the trkcount snapshot before the new collision mechanism is
fully active leave a window for silent data corruption?
In dasd_start_IO() (in drivers/s390/block/dasd.c), the snapshot of the
block's trkcount was removed. Subsequently, in test_and_set_format_track()
(in drivers/s390/block/dasd_eckd.c), the check comparing the snapshot against
the current trkcount was replaced by a check for the collision flag on the CQR.
However, the commit message notes that the new detection only becomes active
later in the series when format requests are routed through
dasd_return_cqr_cb(). Currently, the format CQRs are routed elsewhere, meaning
the collision flag is dormant and never populated.
Can this sequence occur and lead to data loss?
If an IO request (Thread B) hits an unformatted track and enters ERP, and
concurrently another IO request (Thread A) formats the track, writes its
data, and completes. Thread B will not realize the track was formatted after
its IO originally started.
Because the trkcount check is completely removed and the collision flag is
still inactive in this intermediate commit, wouldn't Thread B proceed to
format the track again, effectively erasing Thread A's data?
Could this break bisectability by introducing a vulnerability before the full
feature is wired up later in the series?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=7
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (6 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 07/17] s390/dasd: Add range-based format-track collision detection Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:21 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
` (8 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 | 65 +++++++++++++++++++++++++++-------
1 file changed, 52 insertions(+), 13 deletions(-)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 6577958fdc2d..2c1e0b02a9b2 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -4383,11 +4383,13 @@ static int prepare_itcw(struct itcw *itcw,
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;
@@ -4397,26 +4399,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) {
@@ -4446,7 +4453,39 @@ 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->length = 0;
+ 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:
@@ -4525,7 +4564,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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
2026-07-31 15:38 ` [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK Stefan Haberland
@ 2026-07-31 16:21 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:21 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Alexander Gordeev, linux-s390, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] The explicit assignment `lredata->length = 0` for `WRITE_FULL_TRACK` is unconditionally overwritten.
Pre-existing issues:
- [High] Lockless access to `conf.ned` and `uid.type` in I/O submission paths races with concurrent path events, leading to a NULL pointer dereference.
--
--- Patch [8]: [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK ---
commit 2948648b7fc2c93cb4193e7c55bd961de4d7e8c2
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
prepare_itcw() builds the FCX prefix block (PFX + LRE) for track-mode
I/O. Extend it to handle DASD_ECKD_CCW_WRITE_FULL_TRACK.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 2f6d97c06237b..c1a26485f506d 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -4397,26 +4399,31 @@ static int prepare_itcw(struct itcw *itcw,
[ ... ]
> - 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;
[Severity: High]
This is a pre-existing issue, but can this lockless access to conf.ned lead
to a NULL pointer dereference?
If a concurrent path event triggers dasd_eckd_pe_handler(), it calls
rebuild_device_uid(), which then calls dasd_eckd_identify_conf_parts().
That function transiently sets conf.ned = NULL without holding any locks.
If prepare_itcw() accesses basepriv->conf.ned->unit_addr at that moment, it
will dereference a NULL pointer.
> + 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)
[Severity: High]
This is a pre-existing issue, but does accessing uid.type here race with a
concurrent memset?
The create_uid() function memsets the uid to 0 under the ccwdev_lock, but this
I/O submission path reads startpriv->uid.type locklessly, exposing it to
transient zeroes.
> - pfxdata.validity.verify_base = 1;
> + pfxdata->validity.verify_base = 1;
[ ... ]
> @@ -4446,7 +4453,39 @@ static int prepare_itcw(struct itcw *itcw,
[ ... ]
> + 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->length = 0;
[Severity: Medium]
Will this lredata->length value be unconditionally overwritten?
Later in prepare_itcw(), there is this assignment:
lredata->length = tlf;
If WRITE_FULL_TRACK is used with a non-zero transfer length factor, the
tlf value will overwrite the explicitly requested 0.
> + lredata->extended_parameter_length = 0x02;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=8
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (7 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 08/17] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:13 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
` (7 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_eckd.c | 341 +++++++++++++++++++++++++++++++++
1 file changed, 341 insertions(+)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 2c1e0b02a9b2..d47cbed46152 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -123,6 +123,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 */
@@ -4741,6 +4749,339 @@ 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 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
2026-07-31 15:38 ` [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
@ 2026-07-31 16:13 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:13 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Heiko Carstens, linux-s390, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional iteration over payload segments violates subsystem guidelines and risks a NULL pointer dereference for payload-less requests.
- [Low] Unused static function `dasd_eckd_build_cp_tpm_writefulltrack` causes a build warning/breakage.
--
--- Patch [9]: [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 2c25716c288b80bde9127da58f3afd957ee39c9c
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
This commit adds the channel program builder for WRITE_FULL_TRACK requests.
It emits an eckd_r0 header, an eckd_count plus data pair for every record,
and a terminating pseudo-count to format and write tracks atomically.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index c1a26485f506d..8d8bd40ba896b 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -4741,6 +4749,339 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
[ ... ]
> +static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,
[Severity: Low]
Because this static function is defined but not called anywhere in this
commit, will this generate a -Wunused-function warning and potentially break
builds with -Werror?
> + 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)
> +{
[ ... ]
> + /* process data records */
> + rec = first_offs + 1;
> + rec_count = 0;
> + rq_for_each_segment(bv, req, iter) {
> + dst = bvec_virt(&bv);
[Severity: Medium]
Does this code risk a NULL pointer dereference for payload-less requests?
Iterating over a bio without data using rq_for_each_segment() triggers a
NULL pointer dereference on bio->bi_io_vec. Subsystem guidelines require
using a bio_has_data() guard before accessing data fields. If a payload-less
request (like FLUSH or DISCARD) is passed here, it could crash.
> + seg_len = bv.bv_len;
> + while (seg_len) {
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=9
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (8 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:33 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode Stefan Haberland
` (6 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 | 46 +++++---
drivers/s390/block/dasd_eckd.c | 192 +++++++++++++++++++++++----------
drivers/s390/block/dasd_erp.c | 8 +-
drivers/s390/block/dasd_int.h | 3 +-
4 files changed, 171 insertions(+), 78 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 58867eb397e6..06e99be3100e 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;
@@ -1738,26 +1738,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 */
@@ -2724,6 +2721,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);
@@ -2764,6 +2768,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;
}
/*
@@ -3170,6 +3177,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
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index d47cbed46152..4fdeab98870f 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3212,36 +3212,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);
@@ -3251,60 +3239,133 @@ 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);
+ /*
+ * 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) {
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;
}
/*
@@ -5117,7 +5178,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)
@@ -5159,6 +5220,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;
}
@@ -5326,6 +5392,15 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
sector_t recid;
int status;
+ /*
+ * 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;
if (!dasd_page_cache)
goto out;
private = cqr->block->base->private;
@@ -5361,7 +5436,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;
}
@@ -5438,6 +5512,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 a7010f24eb41..b523d5a08a17 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
2026-07-31 15:38 ` [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
@ 2026-07-31 16:33 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:33 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Alexander Gordeev, linux-s390, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] An infinite loop occurs when a WRITE_FULL_TRACK CQR encounters an INV_TRACK_FORMAT error because the ERP request fails to inherit the `filldata` pointer, bypassing the intended permanent-failure path.
- [High] dasd_eckd_build_cp_tpm_writefulltrack() iterates over bio segments without verifying if the request has data buffers, risking a NULL pointer dereference for data-less requests.
- [Medium] Non-transient errors (like -EINVAL) from `dasd_eckd_build_cp_tpm_writefulltrack` cause wasteful retry loops instead of failing fast.
- [Medium] Double counting of I/O statistics for aborted ESE format replacement requests.
--
--- Patch [10]: [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit 9457d60af3530b5a71a57c6a68b8e701a0e817f4
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
This commit modifies the ESE unformatted track handler to use
WRITE_FULL_TRACK. It changes dasd_eckd_ese_format() to return void and
stage the CQR directly, while marking the origin CQR as aborted.
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 35f722e20c6d3..19a04155d271c 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -1737,26 +1737,23 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
> dasd_schedule_device_bh(device);
> return;
> }
> - fcqr = device->discipline->ese_format(device, cqr, irb);
> - if (IS_ERR(fcqr)) {
> - if (PTR_ERR(fcqr) == -EINVAL) {
> - cqr->status = DASD_CQR_ERROR;
> - return;
> - }
> + if (cqr->filldata) {
> /*
> - * If we can't format now, let the request go
> - * one extra round. Maybe we can format later.
> + * A WRITE_FULL_TRACK cqr carries the complete
> + * track image; INV_TRACK_FORMAT here means the
> + * generated image or the media itself is bad, not
> + * that the track still needs formatting - retrying
> + * via ese_format() would just resubmit the same
> + * write. Let it fail instead.
> */
> - cqr->status = DASD_CQR_QUEUED;
> - dasd_schedule_device_bh(device);
> - return;
> - } else {
> - fcqr->status = DASD_CQR_QUEUED;
> - cqr->status = DASD_CQR_QUEUED;
> - list_add(&fcqr->devlist, &device->ccw_queue);
> + cqr->status = DASD_CQR_ERROR;
> + cqr->stopclk = now;
> + dasd_device_clear_timer(device);
> dasd_schedule_device_bh(device);
> return;
> }
> + device->discipline->ese_format(device, cqr, irb);
> + return;
> }
[Severity: High]
Could this cause an infinite loop during Error Recovery (ERP)?
When a WRITE_FULL_TRACK request fails with INV_TRACK_FORMAT, the ERP
engine creates a retry request but doesn't seem to copy the filldata
pointer from the base request. If this ERP request fails again with
INV_TRACK_FORMAT, will the interrupt handler see a NULL filldata here
and incorrectly call ese_format() again? This would generate a new base
request instead of failing, potentially looping infinitely.
[ ... ]
> @@ -2752,6 +2756,9 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
> error = BLK_STS_IOERR;
> break;
> }
> + } else if (status == DASD_CQR_ABORTED) {
> + /* aborted requests are replaced with a new one so do not complete this */
> + return;
> }
>
> /*
[Severity: Medium]
Could this result in double-counting I/O statistics?
dasd_profile_end() is called unconditionally at the beginning of
__dasd_cleanup_cqr():
__dasd_cleanup_cqr()
{
...
dasd_profile_end(cqr->block, cqr, req);
...
if (status == DASD_CQR_ABORTED) {
return;
}
}
When the replacement format request eventually completes, won't it account
for the same block request again?
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8d8bd40ba896b..4fe1b78366b04 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> + fcqr = dasd_eckd_build_cp_tpm_writefulltrack(startdev, block, req,
> + first_rec, last_rec,
> + first_trk, last_trk,
> + first_offs, last_offs,
> + recs_per_trk, blksize, cqr);
[Severity: High]
Can this cause a NULL pointer dereference for data-less requests?
If a REQ_OP_DISCARD or REQ_OP_WRITE_ZEROES operation fails with an ESE
format error, it seems ese_format() will unconditionally pass it to
dasd_eckd_build_cp_tpm_writefulltrack().
Since dasd_eckd_build_cp_tpm_writefulltrack() uses rq_for_each_segment()
which accesses bio data fields, and data-less requests have a NULL
bi_io_vec, does this need a bio_has_data() check before iterating?
> if (IS_ERR(fcqr)) {
> if (format)
> - clear_format_track(format, block);
> - return fcqr;
> + clear_format_track(format, cqr->block);
> + /*
> + * Transient conditions - the XRC clock is not in sync (-EAGAIN)
> + * or the format request pool is momentarily exhausted under load
> + * (-ENOMEM). Retry the origin without counting down its retries.
> + */
> + if (PTR_ERR(fcqr) == -EAGAIN || PTR_ERR(fcqr) == -ENOMEM)
> + cqr->retries++;
> + goto out_retry;
> }
[Severity: Medium]
Does this cause non-transient errors like -EINVAL to be retried
unnecessarily?
By falling through to out_retry without failing immediately, it appears
permanent build errors might loop until the original request's retry
counter is exhausted. The original code explicitly checked for -EINVAL
and returned it to fail fast.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=10
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (9 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 10/17] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:28 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
` (5 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 (DASD_FT_BIAS_DEFAULT) enables full-track
writes so the ESE format penalty is avoided out of the box.
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 4fdeab98870f..ebb24d7da1c3 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -47,6 +47,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
@@ -2148,6 +2160,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++)
@@ -5194,11 +5211,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 b523d5a08a17..e2df8666520f 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode
2026-07-31 15:38 ` [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode Stefan Haberland
@ 2026-07-31 16:28 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:28 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Heiko Carstens, Alexander Gordeev, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Inconsistent initialization of the `fulltrack` flag disables full-track writes by default, contradicting the intended behavior and sysfs store logic.
- [Medium] Full-track writes are applied unconditionally to non-ESE devices, exposing them to unintended performance penalties.
--
--- Patch [11]: [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode ---
commit c5f39fbb283ed6068ac10c59d09a116557c8bbf1
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add full_track_bias to control fulltrack write mode
This patch adds a per-device full_track_bias sysfs attribute to control
full-track write routing for s390 DASD devices. It introduces a module
parameter to set the default bias applied at online time. The default
value is intended to enable full-track writes out of the box to avoid
ESE format penalties.
> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> index 381d616ad433f..035c022255b68 100644
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
[ ... ]
> @@ -1630,6 +1630,44 @@ dasd_expires_store(struct device *dev, struct device_attribute *attr,
>
> static DEVICE_ATTR(expires, 0644, dasd_expires_show, dasd_expires_store);
>
> +/* 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)
> +{
[ ... ]
> +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)
> {
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 4fe1b78366b04..cc5a4791cb8f4 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -2148,6 +2160,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;
[Severity: Medium]
Is this initialization of device->fulltrack contradicting the logic in
full_track_bias_store()?
In full_track_bias_store() above, device->fulltrack is set to 1 for any
non-zero value (device->fulltrack = val ? 1 : 0). However, here it is only
set to 1 if device->ft_bias >= DASD_FT_BIAS_MAX.
Since the default full_track_bias is DASD_FT_BIAS_DEFAULT (which is 50), this
yields 0 during initialization. This appears to contradict the commit message
which states: "The default (DASD_FT_BIAS_DEFAULT) enables full-track writes so
the ESE format penalty is avoided out of the box."
> +
> if (private->conf.gneq) {
> value = 1;
> for (i = 0; i < private->conf.gneq->timeout.value; i++)
[ ... ]
> @@ -5194,11 +5211,20 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
> /* do nothing, just fall through to the cmd mode single case */
> } else if ((data_size <= private->fcx_max_data)
> && (fcx_multitrack || (first_trk == last_trk))) {
> - cqr = dasd_eckd_build_cp_tpm_track(startdev, block, req,
> - first_rec, last_rec,
> - first_trk, last_trk,
> - first_offs, last_offs,
> - blk_per_trk, blksize);
> + if (!first_offs && (last_offs + 1 == blk_per_trk) &&
> + rq_data_dir(req) == WRITE && basedev->fulltrack) {
[Severity: Medium]
Should there be a check here to ensure the device is an ESE volume before
routing the IO to the full-track write path?
The commit message mentions that "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."
However, neither the initialization in dasd_eckd_check_characteristics() nor
the sysfs store function guards the bias assignment with an ESE check (e.g.,
dasd_eckd_is_ese(basedev)).
If basedev->fulltrack is enabled on a non-ESE device either via sysfs or by
fixing the default initialization, could it suffer the performance penalty
without the intended benefits?
> + 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/20260731153842.1729627-1-sth@linux.ibm.com?part=11
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (10 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 11/17] s390/dasd: Add full_track_bias to control fulltrack write mode Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:27 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
` (4 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_devmap.c | 44 ++++++++++++++++-
drivers/s390/block/dasd_eckd.c | 84 ++++++++++++++++++++++++++++++-
drivers/s390/block/dasd_int.h | 85 ++++++++++++++++++++++++++++++++
3 files changed, 210 insertions(+), 3 deletions(-)
diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 035c022255b6..50301e545bbe 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,41 @@ 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 {
+ 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 +2505,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 ebb24d7da1c3..65dc0de0ddc9 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2162,8 +2162,7 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
/* 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;
+ dasd_ft_bias_apply(device);
if (private->conf.gneq) {
value = 1;
@@ -3171,6 +3170,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);
}
@@ -3229,6 +3235,65 @@ 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 = 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->ese_probe_state = DASD_ESE_HEU_FT0_STABLE;
+ basedev->ese_probe_interval = basedev->ese_heu_probe_window;
+ } else {
+ /*
+ * Still stable in ft0: 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->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)
{
@@ -3253,6 +3318,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);
@@ -4010,6 +4077,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;
@@ -5178,6 +5253,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 e2df8666520f..c899073c19c8 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
2026-07-31 15:38 ` [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
@ 2026-07-31 16:27 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:27 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, Vasily Gorbik, linux-s390, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The adaptive heuristic parameters are never initialized during device startup because `dasd_ft_bias_apply()` is called before the device's ESE capability is known, causing the heuristic to evaluate on every single I/O.
- [High] The NRF rate calculation can overflow a 32-bit signed integer, causing the heuristic to improperly remain in plain write mode during heavy unformatted I/O.
- [High] The lockless heuristic evaluation and sysfs updates fail to serialize state machine transitions, leading to state divergence where the device permanently sticks in fulltrack mode.
--
--- Patch [12]: [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit fc455715bb1f927b75206de59511e6f8a3abe72f
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
This patch turns the middle of the ft_bias range into an adaptive heuristic
that switches between fulltrack write and plain write depending on device
sparseness. It measures the NRF rate in short probe windows to determine
when to flip back to fulltrack write mode.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index cc5a4791cb8f4..1df861f80c268 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2160,6 +2160,7 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
>
> /* 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;
> + dasd_ft_bias_apply(device);
[Severity: High]
Does calling dasd_ft_bias_apply() here happen before the device's ESE
capability is known?
In dasd_eckd_check_characteristics(), dasd_ft_bias_apply() is called early.
This function relies on dasd_ese_adaptive(), which checks is_ese(). However,
is_ese() depends on private->vsq, which is populated later when
dasd_eckd_read_vol_info() is called.
Because vsq is empty at this point, dasd_ese_adaptive() returns false, and
the heuristic parameters (like ese_heu_start_interval and
ese_heu_probe_window) are left initialized to 0. Will this cause the
heuristic interval to pass immediately and the FSM to evaluate on every
single I/O?
[ ... ]
> @@ -3235,6 +3241,65 @@ static void clear_format_track(...)
> +static void dasd_ese_heuristic_tick(struct dasd_device *basedev)
> +{
[ ... ]
> + case DASD_ESE_HEU_PROBING:
> + case DASD_ESE_HEU_FT0_STABLE:
> + nrf = atomic_xchg(&basedev->ese_nrf_window, 0);
> + rate = nrf * 1000 / ios;
> + if (rate > (int)basedev->ese_heu_nrf_high) {
[Severity: High]
Can this rate calculation overflow a 32-bit signed integer?
During the FT0_STABLE state, the ios interval backs off up to max_interval
(500,000 IOs). If a workload performs large sequential writes into
unformatted space, an I/O can span multiple tracks, triggering multiple NRF
events.
If the number of formatted tracks reaches ~2,147,483 within the 500,000 IO
window, nrf * 1000 will exceed INT_MAX and overflow to a negative value.
Could this cause the check to evaluate to false and improperly keep the
device stuck in plain write mode during heavy unformatted I/O?
> + /* 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->ese_probe_state = DASD_ESE_HEU_FT0_STABLE;
> + basedev->ese_probe_interval = basedev->ese_heu_probe_window;
[Severity: High]
Can this lockless state transition race with a sysfs update and permanently
stick the device in fulltrack mode?
If a thread is transitioning from PROBING to FT0_STABLE (evaluating
basedev->ese_probe_state == DASD_ESE_HEU_PROBING as true) but is preempted
before writing the state, a concurrent sysfs update via
full_track_bias_store() -> dasd_ft_bias_apply() could reset the FSM:
drivers/s390/block/dasd_int.h:dasd_ft_bias_apply() {
...
device->ese_probe_state = DASD_ESE_HEU_FT1_ACTIVE;
device->fulltrack = 1;
...
}
When the preempted thread resumes, it would set ese_probe_state to
DASD_ESE_HEU_FT0_STABLE but leave fulltrack as 1. Since fulltrack = 1
suppresses NRF events, the measured rate stays at 0 and the FSM interval
backs off without ever resetting fulltrack to 0, completely defeating the
adaptive heuristic.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=12
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (11 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 12/17] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:35 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
` (3 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 65dc0de0ddc9..b9994104f5eb 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 <asm/css_chars.h>
@@ -2719,6 +2720,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)
@@ -2737,6 +2760,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);
@@ -2770,6 +2794,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. */
@@ -2816,6 +2849,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;
@@ -2960,7 +2997,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++;
}
@@ -3170,6 +3221,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.
@@ -3177,8 +3231,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,
@@ -4068,6 +4132,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;
@@ -4079,10 +4144,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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes
2026-07-31 15:38 ` [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
@ 2026-07-31 16:35 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:35 UTC (permalink / raw)
To: Stefan Haberland
Cc: Alexander Gordeev, linux-s390, Heiko Carstens, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Data race on the `ese_format_quick` flag across concurrent format and space release ioctls.
Pre-existing issues:
- [High] Preexisting heap buffer overflow in dasd_eckd_build_format for non-standard intensities when formatting multiple tracks.
--
--- Patch [13]: [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 48510a9c8812b7b8251c801cd33769f3a6c5745d
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Stamp a format label into newly formatted volumes
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.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 1df861f80c268..1e349dbec44de 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -2770,6 +2794,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. */
[Severity: High]
This is a pre-existing issue, but does the calculation for non-standard format
intensities in dasd_eckd_build_format() under-allocate memory?
Looking at intensity 0x01, cplength is allocated as:
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 subsequent loop unconditionally adds (1 + rpt) CCWs per track for
intensity 0x01, which exceeds the allocated cplength when nr_tracks > 1.
Can this overflow the heap allocation and corrupt memory if an administrator
issues a BIODASDFMT ioctl with a non-standard intensity across multiple tracks?
> @@ -2816,6 +2849,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);
[ ... ]
> @@ -4068,6 +4132,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;
>
> @@ -4079,10 +4144,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;
[Severity: Medium]
Are there concurrent accesses to the ese_format_quick flag during ioctls?
If a user issues concurrent BIODASDRAS and BIODASDFMT ioctls on the same block
device, could they race on reading and writing this lockless variable?
dasd_eckd_release_space_full() sets it to 1 here without locks, while
dasd_eckd_format_device() clears it, and dasd_eckd_fill_format_label() reads
it. Could this lead to a non-deterministic state where a formatted volume is
incorrectly tagged as quick or full in its on-disk label?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=13
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (12 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 13/17] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:43 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online Stefan Haberland
` (2 subsequent siblings)
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 | 177 ++++++++++++++++++++++++++++---
drivers/s390/block/dasd_eckd.h | 7 ++
drivers/s390/block/dasd_int.h | 3 +
4 files changed, 176 insertions(+), 17 deletions(-)
diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 50301e545bbe..89f155e414fd 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -2479,9 +2479,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);
@@ -2519,6 +2520,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 b9994104f5eb..98db3f0bd60e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1676,7 +1676,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;
@@ -1686,6 +1687,49 @@ 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;
+
+ 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;
@@ -2105,6 +2149,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.
@@ -2161,10 +2268,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);
- dasd_ft_bias_apply(device);
-
if (private->conf.gneq) {
value = 1;
for (i = 0; i < private->conf.gneq->timeout.value; i++)
@@ -2219,6 +2322,16 @@ 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 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);
+
/* Read Extent Pool Information */
dasd_eckd_read_ext_pool_info(device);
@@ -2730,7 +2843,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;
@@ -2740,6 +2858,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 *
@@ -3224,23 +3349,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;
}
@@ -4232,6 +4369,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)
@@ -7604,6 +7749,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 c899073c19c8..f61af45deab0 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label
2026-07-31 15:38 ` [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
@ 2026-07-31 16:43 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:43 UTC (permalink / raw)
To: Stefan Haberland
Cc: Alexander Gordeev, Vasily Gorbik, Heiko Carstens, linux-s390
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Unconditional dereference of `device->private` in `dasd_eckd_is_ese()` causes a kernel panic when accessed via the `full_track_bias` sysfs attribute on an initializing or offline device.
- [High] Valid on-disk ESE label cache is incorrectly invalidated on early format failures, breaking thin-provisioning state.
- [Medium] Concurrent execution of dasdfmt and sysfs reads leads to data races and torn reads on the 512-byte `ese_label` structure.
- [Medium] The `ese_label_valid` cache is not invalidated when track 0 is formatted with LDL (intensity 0x00) or invalidate-track, leaving stale in-memory state.
Pre-existing issues:
- [High] The `dasd_release_space()` ioctl helper triggers a NULL pointer dereference when called on a device whose discipline does not implement the `is_ese()` callback (e.g., FBA devices).
--
--- Patch [14]: [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit 6689b09c2730c7fa8fae2e18e3bbf4f3671425e2
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Detect ESE volumes from the on-disk format label
This commit reads the format label from track 0 record 4 at device bring-up
and caches it to determine if a volume is ESE. It also updates sysfs
attributes and format handling to reflect the cached label state, ensuring
volumes copied off ESE storage are still handled as thin.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -1686,6 +1687,49 @@ 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;
> +
> + if (private->ese_label_valid)
[Severity: High]
Can this cause a NULL pointer dereference if device->private is not yet
allocated?
The full_track_bias_store() sysfs function can indirectly call
dasd_eckd_is_ese() during the initialization window before private is
allocated, or if allocation fails:
full_track_bias_store()
dasd_ft_bias_apply()
dasd_eckd_is_ese()
private->ese_label_valid
> + return !!(private->ese_label.flags & DASD_ESE_LABEL_F_ESE);
> +
> + return dasd_eckd_ese_capable(device);
> +}
[ ... ]
> @@ -2730,7 +2843,12 @@ 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]
Does this introduce a data race that could lead to torn reads?
Sysfs readers accessing on_demand_formatting or full_track_bias can call
dasd_eckd_is_ese(), which reads private->ese_label.flags locklessly. If a
format is concurrently updating this 512-byte structure here, could a
reader observe ese_label_valid == true but see stale or partially written
flags?
[ ... ]
> @@ -3224,23 +3349,35 @@ static int dasd_eckd_format_device(struct dasd_device *base,
> struct dasd_eckd_private *private = base->private;
> int rc;
>
> 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;
> + }
[Severity: High]
Does this incorrectly drop the valid in-memory label state on early format
failures?
If dasd_eckd_format_process_data() fails early due to sanity checks (like
invalid parameters where stop_unit is out of bounds) before any I/O is
issued, rc will be non-zero and the cache is cleared. Would this cause
subsequent writes to unallocated tracks to fail with 'No Record Found'
instead of triggering on-demand formatting?
[Severity: Medium]
Does this leave a stale cache in memory if the format succeeds but destroys
the on-disk label?
If track 0 is formatted using the LDL format mode (intensity == 0x00), it
skips writing the label but rc will be 0. Would this leave the old
ese_label_valid state incorrectly set to true?
[ ... ]
While reviewing these changes, I also noticed a pre-existing issue in the
dasd ioctl handling:
[Severity: High]
This is a pre-existing issue, but can dasd_release_space() in
drivers/s390/block/dasd_ioctl.c trigger a NULL pointer dereference?
If a device lacks the is_ese callback (such as dasd_fba), this logic:
if (!device->discipline->is_ese && !device->discipline->is_ese(device))
return -ENOTSUPP;
evaluates to true for the first condition when the pointer is NULL, and
then attempts to call the NULL pointer in the second condition.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=14
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (13 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 14/17] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:39 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
2026-07-31 15:38 ` [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 98db3f0bd60e..787c3cc6ac68 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2518,6 +2518,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;
@@ -2604,15 +2605,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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online
2026-07-31 15:38 ` [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online Stefan Haberland
@ 2026-07-31 16:39 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:39 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, Vasily Gorbik, Alexander Gordeev, linux-s390
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the `BIODASDRAS` ioctl crashes the kernel on DASD devices whose discipline does not implement `.is_ese`.
--
--- Patch [15]: [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online ---
commit a8b5bcde61b1c7e9effa84a4b07196f1129e6ac3
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. It
derives the format mode (full or on demand) from the on-disk format label.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8f7ffbeb9a6ca..a7d874e79336e 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
This isn't a bug introduced by this patch, but while reviewing the ESE
support I noticed a pre-existing issue in the BIODASDRAS ioctl handling:
drivers/s390/block/dasd_ioctl.c:dasd_release_space() {
if (!device->discipline->is_ese && !device->discipline->is_ese(device))
return -ENOTSUPP;
if (!device->discipline->release_space)
return -ENOTSUPP;
[Severity: High]
Can this logic cause a kernel panic if a privileged user issues the
BIODASDRAS ioctl on a device whose discipline lacks the .is_ese callback?
If the .is_ese function pointer is NULL, the first half of the condition
evaluates to true. The && operator then proceeds to evaluate the right
side, which immediately dereferences the NULL function pointer.
Additionally, when the callback is present (such as on ECKD devices), does
the && operator skip the capability check entirely? Since the left side
evaluates to false when the pointer exists, the right side is never
executed.
Should this condition use || instead of && to properly check for both the
existence of the callback and its return value?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=15
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (14 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 15/17] s390/dasd: Report ESE capability and format mode at device online Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:54 ` sashiko-bot
2026-07-31 15:38 ` [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd.c | 29 ++++--
drivers/s390/block/dasd_eckd.c | 171 ++++++++++++++++++++++++++-------
drivers/s390/block/dasd_int.h | 2 +
3 files changed, 159 insertions(+), 43 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 06e99be3100e..27998bbb8935 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)
@@ -3169,6 +3171,15 @@ 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) {
+ /*
+ * e.g. a discard that covers no whole extent. This is an
+ * expected, benign outcome (fstrim ranges rarely align to
+ * the large ESE extent granularity), so silence the
+ * per-request block-layer error print for it.
+ */
+ req->rq_flags |= RQF_QUIET;
+ rc = BLK_STS_NOTSUPP;
} else {
DBF_DEV_EVENT(DBF_ERR, basedev,
"CCW creation failed (rc=%ld) on request %p",
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 787c3cc6ac68..2f8839c066f3 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2322,6 +2322,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);
@@ -4118,37 +4130,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)
@@ -4199,9 +4187,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;
@@ -4233,7 +4229,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;
@@ -4329,6 +4325,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;
@@ -5466,6 +5465,58 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_de
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)
@@ -5504,6 +5555,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)
@@ -5816,11 +5873,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;
@@ -7708,6 +7767,49 @@ 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;
+ 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",
@@ -7730,6 +7832,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 f61af45deab0..26ecf9f47d2e 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes
2026-07-31 15:38 ` [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
@ 2026-07-31 16:54 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:54 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Alexander Gordeev, linux-s390, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Setting discard_granularity to a non-power-of-two value corrupts block layer discard math.
- [High] Returning BLK_STS_NOTSUPP for a benign unaligned discard causes filesystems to permanently disable discard support.
- [High] Potential divide-by-zero kernel panic in dasd_eckd_disc_limits if ext_bytes evaluates to 0.
--
--- Patch [16]: [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes ---
commit 7e5698c663d8a9a7aaad634b522a9f242320381c
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Re-enable discard support for ESE volumes
Re-enable block-layer discard for ESE ECKD volumes, releasing thin space
via release allocated space (RAS). This routes REQ_OP_DISCARD to a RAS release
over the request's track range and defines limits using extent granularity.
> 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
[ ... ]
> @@ -3157,6 +3159,15 @@ 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) {
> + /*
> + * e.g. a discard that covers no whole extent. This is an
> + * expected, benign outcome (fstrim ranges rarely align to
> + * the large ESE extent granularity), so silence the
> + * per-request block-layer error print for it.
> + */
> + req->rq_flags |= RQF_QUIET;
> + rc = BLK_STS_NOTSUPP;
[Severity: High]
Will this cause upper layers to completely disable discard support?
When returning BLK_STS_NOTSUPP for a benign unaligned discard, filesystems
like ext4 typically interpret this as the underlying device lacking discard
support entirely. This causes them to clear their DISCARD mount flag,
permanently disabling discard for the volume.
Should this instead complete the request with BLK_STS_OK so that it acts as
a silent no-op without breaking subsequent discard requests?
> } else {
> DBF_DEV_EVENT(DBF_ERR, basedev,
> "CCW creation failed (rc=%ld) on request %p",
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -7708,6 +7767,49 @@ 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;
> + max_bytes = UINT_MAX - (UINT_MAX % ext_bytes);
[Severity: High]
Can ext_bytes evaluate to zero here?
If the hardware or hypervisor supplies malformed RDC data, or if recs_per_trk
evaluates to 0 due to an unrecognized device type, ext_bytes will be zero.
Would the subsequent modulo arithmetic (UINT_MAX % ext_bytes) then trigger
a divide-by-zero panic during device initialization?
> + 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;
[Severity: Critical]
Is it safe to use a non-power-of-two value for discard_granularity here?
The block layer strictly requires discard_granularity to be a power of two
because it relies on bitwise macros to compute discard alignments (such as
sector &= ~(granularity >> SECTOR_SHIFT - 1) in __blkdev_issue_discard).
Could supplying a non-power-of-two value break these assumptions and cause
the block layer to compute incorrect sector offsets and lengths, potentially
discarding live filesystem data instead of free space?
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=16
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path
2026-07-31 15:38 [PATCH v5 00/17] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (15 preceding siblings ...)
2026-07-31 15:38 ` [PATCH v5 16/17] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
@ 2026-07-31 15:38 ` Stefan Haberland
2026-07-31 16:58 ` sashiko-bot
16 siblings, 1 reply; 35+ messages in thread
From: Stefan Haberland @ 2026-07-31 15:38 UTC (permalink / raw)
To: linux-s390; +Cc: Jan Hoeppner, Eduard Shishkin
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 2f8839c066f3..5cfbf34fa019 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -588,8 +588,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 = basepriv->ned_ua;
+ pfxdata->base_lss = basepriv->ned_lss;
pfxdata->validity.define_extent = 1;
/* private uid is kept up to date, conf_data may be outdated */
@@ -806,6 +807,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 */
+ private->ned_lss = private->conf.ned->ID;
+ private->ned_ua = private->conf.ned->unit_addr;
spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
return 0;
}
@@ -1631,8 +1635,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 = private->ned_lss;
+ prssdp->volume = private->ned_ua;
ccw = cqr->cpaddr;
ccw->cmd_code = DASD_ECKD_CCW_PSF;
@@ -4232,8 +4236,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 = private->ned_lss;
+ ras_data->dev_addr = private->ned_ua;
ras_data->nr_exts = nr_exts;
if (by_extent) {
@@ -4804,8 +4809,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 = basepriv->ned_ua;
+ pfxdata->base_lss = basepriv->ned_lss;
pfxdata->validity.define_extent = 1;
/* private uid is kept up to date, conf_data may be outdated */
@@ -6886,8 +6892,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 = private->ned_lss;
+ prssdp->volume = 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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path
2026-07-31 15:38 ` [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
@ 2026-07-31 16:58 ` sashiko-bot
0 siblings, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-07-31 16:58 UTC (permalink / raw)
To: Stefan Haberland
Cc: Heiko Carstens, Alexander Gordeev, Vasily Gorbik, linux-s390
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] Transient zeroing of `uid.type` disables alias device verification in lockless CCW-build paths.
- [High] Concurrent sysfs write to `path_reset` triggers a Use-After-Free or NULL pointer dereference on `conf.gneq`.
- [High] Concurrent `BIODASDINFO` / `BIODASDINFO2` ioctl triggers a Use-After-Free or NULL pointer dereference on `conf.data`.
- [High] Concurrent execution of path event handlers and configuration reload causes memory corruption and UAF on `conf.data` and `conf.ned`.
- [High] TOCTOU NULL pointer dereference in `dasd_eckd_generate_uid`.
--
--- Patch [17]: [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit 962f258b40a4684c7ad8e7e9f2ae8c0f71efe3ff
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Read cached unit address and LSS in the CCW build path
This commit resolves a use-after-free in the CCW build paths by caching
the unit address and LSS from conf.ned into the private device data. This
prevents a race condition where the reload worker frees conf.ned while
I/O requests are being built.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index eb2a2f8fac644..6548ee0c01d90 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -588,8 +588,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 = basepriv->ned_ua;
> + pfxdata->base_lss = basepriv->ned_lss;
> pfxdata->validity.define_extent = 1;
>
> /* private uid is kept up to date, conf_data may be outdated */
[Severity: High]
This is a pre-existing issue, but since this patch fixes lockless access
to ned_lss and ned_ua, could a similar problem occur with uid.type?
In drivers/s390/block/dasd_eckd.c, both prefix_LRE() and prepare_itcw()
read startpriv->uid.type locklessly during I/O submission:
if (startpriv->uid.type == UA_BASE_PAV_ALIAS)
pfxdata->validity.verify_base = 1;
Concurrently, a configuration reload calls create_uid() under ccwdev_lock,
which executes memset(uid, 0, sizeof(struct dasd_uid)).
Could this transient zeroing of the uid structure disable alias device
verification during these CCW-build paths?
[ ... ]
> @@ -806,6 +807,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 */
> + private->ned_lss = private->conf.ned->ID;
> + private->ned_ua = private->conf.ned->unit_addr;
> spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
> return 0;
> }
[Severity: High]
This isn't a bug introduced by this patch, but looking at the earlier checks
in dasd_eckd_generate_uid() in drivers/s390/block/dasd_eckd.c, there appears
to be a potential Time-of-Check to Time-of-Use race:
if (!private->conf.ned || !private->conf.gneq)
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 */
private->ned_lss = private->conf.ned->ID;
Since the NULL check for private->conf.ned happens outside the ccwdev_lock,
could a concurrent reconfiguration (like in dasd_eckd_identify_conf_parts())
locklessly set conf->ned to NULL? If so, would this result in a NULL pointer
dereference when it is subsequently used inside create_uid() or when caching
private->ned_lss?
[ ... ]
While reviewing the fixes for conf.ned, I noticed a few other pre-existing
synchronization issues related to configuration reloads and path events in
drivers/s390/block/dasd_eckd.c.
[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free
in get_fcx_max_data() during a concurrent sysfs path_reset?
When dasd_eckd_clear_conf_data() frees device->path[i].conf_data, it
appears to leave private->conf.gneq pointing to the freed memory. Could
a concurrent sysfs write to path_reset call get_fcx_max_data() and
locklessly read private->conf.gneq->reserved2[7], dereferencing the
dangling pointer?
[Severity: High]
This isn't a bug introduced by this patch, but does the BIODASDINFO ioctl
have a similar race with device reloads?
Looking at dasd_eckd_fill_info(), it locklessly reads private->conf.len
and performs a memcpy() from private->conf.data. If the reload worker
calls dasd_eckd_clear_conf_data() concurrently, it frees the underlying
buffer and sets conf.data = NULL. Could the ioctl thread read a non-zero
length but execute the memcpy() after the buffer is freed or NULLed,
leading to a crash or a memory leak to userspace?
[Severity: High]
This is a pre-existing issue, but is there a lack of mutual exclusion
between path event handlers and configuration reloads?
The do_pe_handler_work() and do_reload_device() workers might execute
concurrently. If do_reload_device() calls dasd_eckd_clear_conf_data(),
could rebuild_device_uid() in the PE handler concurrently execute a
lockless memcpy() into the freed private->conf.data? Would it then call
dasd_eckd_generate_uid(), which dereferences the dangling conf.ned pointer?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260731153842.1729627-1-sth@linux.ibm.com?part=17
^ permalink raw reply [flat|nested] 35+ messages in thread