* [PATCH v3 01/13] s390/dasd: Optimize max blocks per request for track alignment
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 13:58 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
` (11 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
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] 27+ messages in thread* [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
2026-07-23 13:47 ` [PATCH v3 01/13] s390/dasd: Optimize max blocks per request for track alignment Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:11 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection Stefan Haberland
` (10 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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 unformated tracks on ESE devices.
struct dasd_ccw_req: filldata/fill_chunks for the per-track metadata
buffer (R0 + count records) the CCW program TIDAWs into, and
format/start_trk/end_trk/collision linking a request to its format-track
guard entry.
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 for
ESE format CQRs spliced into ccw_queue from dasd_block_tasklet (enqueuing
directly from the IRQ handler would invert queue_lock / ccwdev_lock).
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. Since dasd_alloc_device() runs in process context (device
set_online), these pool allocations now use GFP_KERNEL rather than
GFP_ATOMIC, which is more reliable for the larger, order-2 DMA
allocations.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd.c | 105 +++++++++++++++++++++++++++------
drivers/s390/block/dasd_eckd.c | 11 ++++
drivers/s390/block/dasd_eckd.h | 5 ++
drivers/s390/block/dasd_int.h | 19 ++++++
4 files changed, 122 insertions(+), 18 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index dc1c6d5ab3be..a8dea7503dce 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -87,35 +87,57 @@ 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);
+ /*
+ * 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_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);
+ 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_ATOMIC | 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);
@@ -2726,7 +2771,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;
/*
* We need to take care for ETIMEDOUT errors here since the
@@ -2771,7 +2818,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) {
@@ -2879,9 +2927,21 @@ static void dasd_block_tasklet(unsigned long data)
struct dasd_ccw_req *cqr;
struct dasd_queue *dq;
+ if (!block) {
+ WARN_ON_ONCE(!block);
+ return;
+ }
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 +3001,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 073795ea7cd0..52f3859ef537 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 >> 16;
+ geo->head <<= 4;
+ 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 763733bcc4d2..184266102f60 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;
+
struct ch_t {
__u16 cyl;
__u16 head;
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] 27+ messages in thread* Re: [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write
2026-07-23 13:47 ` [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
@ 2026-07-23 14:11 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:11 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Vasily Gorbik, Heiko Carstens, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The newly added check `else if (status == DASD_CQR_ABORTED)` erroneously compares the boolean return value of `free_cp` to an integer constant, rendering the intended abort early-return unreachable and causing fatal double-completion of block requests.
--
--- Patch [2]: [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write ---
commit fa055a0513f53e5dc907e4af42753941bd1ec384
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 in response to unformatted tracks on ESE devices. It introduces
new CQR states like DASD_CQR_ABORTED to retire origin requests when they
are replaced. It also enlarges CCW and ESE format pools to keep two
maximum-size requests in flight.
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3181c06d91ce3..2289f36d38b11 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -2714,7 +2759,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: Critical]
Is it possible for this check to ever evaluate to true?
The 'status' variable is assigned the return value of
cqr->block->base->discipline->free_cp(cqr, req) earlier in this function.
Looking at dasd_eckd_free_cp(), it appears to return a boolean:
drivers/s390/block/dasd_eckd.c:dasd_eckd_free_cp() {
...
status = cqr->status == DASD_CQR_DONE;
dasd_sfree_request(cqr, cqr->memdev);
return status;
}
Since DASD_CQR_ABORTED is defined as 0x06, this check would always be false.
When free_cp() returns 0 for an aborted request, the code will fall into the
'else if (status == 0)' block above and attempt to complete the request via
blk_mq_end_request().
Because the replacement request will also try to complete the same req, does
this result in a double-completion of the block request?
>
> /*
> * We need to take care for ETIMEDOUT errors here since the
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
2026-07-23 13:47 ` [PATCH v3 01/13] s390/dasd: Optimize max blocks per request for track alignment Stefan Haberland
2026-07-23 13:47 ` [PATCH v3 02/13] s390/dasd: Add infrastructure for ESE full-track write Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:13 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 04/13] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK Stefan Haberland
` (9 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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 queued
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().
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd.c | 28 +++++++++++----
drivers/s390/block/dasd_eckd.c | 63 +++++++++++++++++++++++-----------
drivers/s390/block/dasd_int.h | 19 ++++++++--
3 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index a8dea7503dce..4934253dfd0e 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);
@@ -2867,6 +2860,27 @@ 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 queued CQRs that overlap this just-completed format
+ * range so they re-check in test_and_set_format.
+ */
+ 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 52f3859ef537..541b8eff647e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3120,32 +3120,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);
@@ -3153,13 +3164,16 @@ 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;
+ if (WARN_ON_ONCE(!block || !format))
+ return;
+
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);
}
@@ -3181,8 +3195,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;
@@ -3201,7 +3215,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);
@@ -3218,9 +3231,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);
@@ -3232,17 +3245,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)
+ list_del(&format->list);
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)
+ list_del(&format->list);
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] 27+ messages in thread* Re: [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection
2026-07-23 13:47 ` [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection Stefan Haberland
@ 2026-07-23 14:13 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:13 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, Alexander Gordeev, Heiko Carstens, linux-s390
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Uninitialized track range fields in data requests break collision detection.
- [High] Unreachable collision detection logic due to incorrect callback.
- [High] Collision detection loop ignores queued requests, skipping the ones it is meant to wake up.
- [High] Error paths perform unprotected list deletions and permanently leak format array slots.
--
--- Patch [3]: [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection ---
commit 1d2db2dae28f75e003a69468da44050bd92fdedd
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add range-based format-track collision detection
Replace the single per-device format_entry slot with an array of 16
slots so multiple format requests can be in flight at once, and extend
struct dasd_format_entry with a start_trk/end_trk/cqr range (replacing
the single track field).
> 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
[ ... ]
> @@ -2855,6 +2848,27 @@ 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;
[Severity: High]
Does this guard prevent the collision logic from ever being executed?
Regular data requests use dasd_return_cqr_cb() as their callback but they
do not have cqr->format set. ESE format requests have cqr->format set, but
they use dasd_eckd_ese_format_cb() as their callback instead of this one.
Will this path always take the goto out, making the collision logic
unreachable?
> +
> + block = cqr->block;
> + /*
> + * Mark queued CQRs that overlap this just-completed format
> + * range so they re-check in test_and_set_format.
> + */
> + 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 &&
[Severity: High]
Does this explicitly skip requests in the queued state, which are exactly
the ones waiting to be flagged?
Requests delayed due to a conflict (returning -EEXIST from
test_and_set_format_track) are put in the DASD_CQR_QUEUED state. If this
loop filters out DASD_CQR_QUEUED, does it ignore the exact requests it was
designed to evaluate and flag with collision = true?
> + 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
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3232,17 +3245,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)
> + list_del(&format->list);
> 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)
> + list_del(&format->list);
> return fcqr;
> + }
[Severity: High]
Are we modifying the list without holding block->format_lock here?
These error paths call list_del(&format->list) without acquiring the lock.
Additionally, they do not set format->cqr = NULL. Since
test_and_set_format_track() identifies free slots by checking
!device->format_entry[i].cqr, does this permanently leak format array slots?
Could this use clear_format_track(format, block) instead to properly lock
and clear the state?
> diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
> --- a/drivers/s390/block/dasd_int.h
> +++ b/drivers/s390/block/dasd_int.h
[ ... ]
> @@ -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);
> +}
> +
[Severity: Critical]
Are start_trk and end_trk guaranteed to be initialized for normal data
requests?
Since dasd_fmalloc_request() zero-allocates memory, cqr2->start_trk and
cqr2->end_trk default to 0 for normal data requests. When evaluated here,
the comparison cqr2->end_trk < cqr1->format->start_trk evaluates as
0 < format->start_trk, which is always true for any track > 0.
Does this silently fail to detect collisions for any tracks other than
track 0?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 04/13] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (2 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 03/13] s390/dasd: Add range-based format-track collision detection Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:25 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
` (8 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
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 541b8eff647e..8649e57fe4c2 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -4356,11 +4356,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;
@@ -4370,26 +4372,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) {
@@ -4419,7 +4426,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:
@@ -4498,7 +4537,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] 27+ messages in thread* [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (3 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 04/13] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:14 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
` (7 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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 | 339 +++++++++++++++++++++++++++++++++
1 file changed, 339 insertions(+)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 8649e57fe4c2..eb0cfe4290bc 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 */
@@ -4714,6 +4722,337 @@ 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)
+{
+ unsigned long off = (unsigned long)addr & (PAGE_SIZE - 1);
+
+ return len && (off + len > PAGE_SIZE);
+}
+
+static __always_inline void *reserve_nocross(char **p, size_t *space, size_t len)
+{
+ unsigned long addr = (unsigned long)*p;
+ size_t off = addr & (PAGE_SIZE - 1);
+ size_t pad;
+
+ if (off + len > PAGE_SIZE) {
+ pad = PAGE_SIZE - off;
+ if (*space < pad)
+ return NULL; /* out of space */
+ *p = (void *)((char *)*p + pad);
+ *space -= pad;
+ addr += pad;
+ }
+
+ if (*space < len)
+ return NULL; /* out of space */
+
+ void *ret = *p;
+ *p = (void *)((char *)*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 eckd_r0 *r0;
+ struct tidaw *tidaw;
+
+ 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)));
+ spin_lock_irqsave(&startdev->mem_lock, flags);
+ filldata = dasd_alloc_chunk(&startdev->fill_chunks, fillsize);
+ spin_unlock_irqrestore(&startdev->mem_lock, flags);
+ if (!filldata) {
+ /* transient pool pressure: -ENOMEM so the block layer requeues */
+ ret = -ENOMEM;
+ goto out_error;
+ }
+ memset(filldata, 0, fillsize);
+ cqr->filldata = filldata;
+
+ nullrecord = startdev->nulldata;
+ rec_count = trkcount * blk_per_trk;
+
+ /* 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 = rec_count * (blksize + sizeof(struct eckd_count)) +
+ trkcount * (sizeof(struct eckd_count) + sizeof(struct eckd_r0));
+ 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);
+ len_to_track_end = count * 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;
+ /* the last tidaw of a track must carry the CBC flag */
+ if (!len_to_track_end)
+ new_track = 1;
+
+ /* add track end marker */
+ last_tidaw = add_track_record(itcw, &filldata, &fillsize,
+ cyl, head, rec, dst, part_len);
+ if (!last_tidaw)
+ goto out_error;
+
+ if (new_track) {
+ 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] 27+ messages in thread* Re: [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
2026-07-23 13:47 ` [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
@ 2026-07-23 14:14 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:14 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, linux-s390, Alexander Gordeev, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Incorrect `len_to_track_end` initialization for subsequent tracks leads to malformed channel programs with duplicate Track End markers.
- [High] The function iterates over bio data segments without verifying operation type safety, violating block layer guidelines.
--
--- Patch [5]: [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 86379d92de11de4619dd9c8f1c829907036c1f56
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()
Add the channel program builder for WRITE_FULL_TRACK requests, used
by dasd_eckd_ese_format() to format and write a set of tracks
atomically and avoid the format cycle on ESE devices.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index a570fe2e85693..0962c9f72c864 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -4714,6 +4722,337 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
[ ... ]
> + /* process data records */
> + rec = first_offs + 1;
> + rec_count = 0;
> + rq_for_each_segment(bv, req, iter) {
[Severity: High]
Can this cause a NULL pointer dereference if a dataless request (like
REQ_OP_DISCARD or REQ_OP_FLUSH) is dispatched here?
The block layer guidelines indicate that iterating over bio data segments
requires checking blk_rq_has_data() or bio_has_data() first, as op_is_write()
evaluates to true for dataless operations, which have a NULL bi_io_vec.
> + 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);
> + len_to_track_end = count * blksize;
[Severity: Critical]
Is len_to_track_end correctly initialized here for subsequent tracks?
It looks like this clamps the track length to the remaining data length
(count * blksize) instead of the physical track capacity
(blk_per_trk * blksize).
If len_to_track_end is set to the data length, it will reach zero prematurely.
Will this cause the track end marker to be emitted early inside this loop:
if (new_track) {
last_tidaw = add_track_end(itcw, &filldata,
&fillsize);
...
}
And then a second track end marker to be appended later in out_skip after the
padding records? This might result in a malformed channel program that is
rejected by the hardware.
> + recid += count;
> + new_track = 0;
> + /* the R0 header of the next track */
> + cyl = curr_trk / heads;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=5
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (4 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 05/13] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:13 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode Stefan Haberland
` (6 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd.c | 43 ++++---
drivers/s390/block/dasd_eckd.c | 203 +++++++++++++++++++++++----------
drivers/s390/block/dasd_erp.c | 8 +-
drivers/s390/block/dasd_int.h | 3 +-
4 files changed, 174 insertions(+), 83 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 4934253dfd0e..3c3086b6a769 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);
@@ -3175,6 +3179,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 eb0cfe4290bc..daddb8727d82 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3185,36 +3185,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);
@@ -3224,60 +3212,135 @@ 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;
+ return;
+ }
+ 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;
+ return;
+ }
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;
+ return;
}
-
- 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)
- list_del(&format->list);
- 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();
+ dasd_device_clear_timer(startdev);
+ dasd_schedule_device_bh(startdev);
+ return;
}
/*
- * 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 err_out;
+ }
+ }
+
+ 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)
- list_del(&format->list);
- 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 err_out;
}
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;
+ dasd_schedule_block_bh(block);
+ dasd_schedule_device_bh(startdev);
+
+ return;
+err_out:
+ /*
+ * 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;
+ dasd_schedule_block_bh(block);
+ dasd_schedule_device_bh(startdev);
+ return;
}
/*
@@ -4715,7 +4778,6 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
set_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
set_bit(DASD_CQR_SUPPRESS_IT, &cqr->flags);
}
-
return cqr;
out_error:
dasd_sfree_request(cqr, startdev);
@@ -5072,7 +5134,6 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
basedev = block->base;
private = basedev->private;
-
/* Calculate number of blocks/records per track. */
blksize = block->bp_block;
blk_per_trk = recs_per_track(&private->rdc_data, 0, blksize);
@@ -5088,7 +5149,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)
@@ -5105,10 +5166,10 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
} 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);
+ 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;
@@ -5130,6 +5191,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;
}
@@ -5297,6 +5363,14 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
sector_t recid;
int status;
+ /*
+ * special handling for format aborted requests
+ * only clean up the request, nothing finished
+ */
+ if (cqr->status == DASD_CQR_ABORTED) {
+ status = DASD_CQR_ABORTED;
+ goto abort;
+ }
if (!dasd_page_cache)
goto out;
private = cqr->block->base->private;
@@ -5333,6 +5407,7 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
}
out:
status = cqr->status == DASD_CQR_DONE;
+abort:
dasd_sfree_request(cqr, cqr->memdev);
return status;
}
@@ -5409,6 +5484,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 89d7516b9ec8..8ba76b616129 100644
--- a/drivers/s390/block/dasd_erp.c
+++ b/drivers/s390/block/dasd_erp.c
@@ -120,13 +120,14 @@ 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;
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;
@@ -147,7 +148,10 @@ struct dasd_ccw_req *dasd_default_erp_postaction(struct dasd_ccw_req *cqr)
cqr->startclk = startclk;
cqr->stopclk = stopclk;
cqr->startdev = startdev;
- 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] 27+ messages in thread* Re: [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
2026-07-23 13:47 ` [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
@ 2026-07-23 14:13 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:13 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] Generating a WRITE_FULL_TRACK CCW for a request spanning more than 16 tracks causes a negative bit shift, producing an invalid hardware track mask.
- [High] Bounce buffers allocated from `dasd_page_cache` are permanently leaked when an origin request is aborted and replaced by an ESE format request.
- [High] The `fillsize` allocation for `filldata` does not account for padding injected by `reserve_nocross()`, leading to out-of-bounds allocation failures and dropped I/O.
Pre-existing issues:
- [Critical] Partial I/O completions on ERP requests lose the `proc_bytes` value, causing the block layer to incorrectly treat the entire request as successful.
- [High] Error paths in `dasd_eckd_ese_format` fail to schedule the device tasklet or clear the timer, causing the I/O request to hang.
--
--- Patch [6]: [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler ---
commit 2efec0cd23e150d9cbd9db63fa5fad5ebdee9860
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Use WRITE_FULL_TRACK in ESE format handler
Wire dasd_eckd_build_cp_tpm_writefulltrack() into the ESE unformated
track handler. dasd_eckd_ese_format() now returns void and builds a
writefulltrack CQR directly.
> 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
> @@ -3224,60 +3212,135 @@ dasd_eckd_ese_format(struct dasd_device *startdev, struct dasd_ccw_req *cqr,
[ ... ]
> + 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;
> + return;
> + }
[Severity: High]
This is a pre-existing issue, but error paths in dasd_eckd_ese_format() fail
to schedule the device tasklet or clear the timer. If sense data cannot be
parsed or the track is out-of-bounds, dasd_eckd_ese_format() sets the status
to DASD_CQR_ERROR and returns. Since dasd_int_handler() expects the
discipline hook to handle scheduling on error, doesn't this leave the I/O
request stranded in the queue until the 5-minute fallback timeout fires?
[ ... ]
> + 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 trkcount exceed 16 here? When dasd_eckd_build_cp_tpm_writefulltrack()
calls prepare_itcw(), the count argument is used in a bit shift:
lredata->extended_parameter[1] <<= (16 - count);
If a user issues a large write request that spans more than 16 tracks,
won't this result in a negative bit shift, causing undefined behavior and
generating an invalid hardware track mask?
[Severity: High]
Does the fillsize allocation inside dasd_eckd_build_cp_tpm_writefulltrack()
account for the padding injected by reserve_nocross()?
fillsize = trkcount * (sizeof(struct eckd_r0) +
(sizeof(struct eckd_count) * (blk_per_trk + 2)));
filldata = dasd_alloc_chunk(&startdev->fill_chunks, fillsize);
When reserve_nocross() appends elements, it injects padding bytes to
prevent them from crossing page boundaries. Because this padding is not
budgeted in fillsize, the remaining space might drop below zero before all
track elements are written, causing reserve_nocross() to return NULL and
abort the request.
> @@ -5297,6 +5363,14 @@ dasd_eckd_free_cp(struct dasd_ccw_req *cqr, struct request *req)
> sector_t recid;
> int status;
>
> + /*
> + * special handling for format aborted requests
> + * only clean up the request, nothing finished
> + */
> + if (cqr->status == DASD_CQR_ABORTED) {
> + status = DASD_CQR_ABORTED;
> + goto abort;
> + }
> if (!dasd_page_cache)
> goto out;
[Severity: High]
Does this early exit skip freeing the bounce buffers? If dasd_page_cache
is active and an origin request is marked DASD_CQR_ABORTED (because it
was replaced by an ESE format request), the goto abort statement bypasses
the loop responsible for calling kmem_cache_free(). Could this permanently
leak the bounce buffer memory allocated for the I/O segments?
> diff --git a/drivers/s390/block/dasd_erp.c b/drivers/s390/block/dasd_erp.c
> --- a/drivers/s390/block/dasd_erp.c
> +++ b/drivers/s390/block/dasd_erp.c
> @@ -120,13 +120,14 @@ 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;
>
> BUG_ON(cqr->refers == NULL || cqr->function == NULL);
>
> success = cqr->status == DASD_CQR_DONE;
> + aborted = cqr->status == DASD_CQR_ABORTED;
[Severity: Critical]
This is a pre-existing issue, but does the loop that unwinds the ERP chain
just below here lose the proc_bytes value from a partial completion?
while (cqr->refers != NULL) {
struct dasd_ccw_req *refers = cqr->refers;
list_del(&cqr->blocklist);
dasd_free_erp_request(cqr, cqr->memdev);
cqr = refers;
}
When an ERP retry hits an unformatted track, dasd_eckd_ese_format() or
ese_read() sets cqr->proc_bytes on the ERP request and marks it successful.
During this unwinding, the ERP request holding this partial completion is
freed without propagating proc_bytes to the base request. When the base
request is later processed, it will have proc_bytes == 0, causing the block
layer to incorrectly treat the entire request as successful and silently
drop the unwritten or unread portion.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=6
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (5 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 06/13] s390/dasd: Use WRITE_FULL_TRACK in ESE format handler Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:16 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
` (5 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_devmap.c | 39 ++++++++++++++++++++++++++++++++
drivers/s390/block/dasd_eckd.c | 35 ++++++++++++++++++++++++----
drivers/s390/block/dasd_int.h | 18 +++++++++++++++
3 files changed, 87 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 daddb8727d82..11d1fc83c5a3 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
@@ -2118,6 +2130,10 @@ 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);
+ device->fulltrack = device->ft_bias ? 1 : 0;
+
if (private->conf.gneq) {
value = 1;
for (i = 0; i < private->conf.gneq->timeout.value; i++)
@@ -5165,11 +5181,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] 27+ messages in thread* Re: [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode
2026-07-23 13:47 ` [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode Stefan Haberland
@ 2026-07-23 14:16 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:16 UTC (permalink / raw)
To: Stefan Haberland
Cc: Vasily Gorbik, linux-s390, Heiko Carstens, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional enablement of `WRITE_FULL_TRACK` via the `full_track_bias` module parameter causes a severe throughput regression for non-ESE standard ECKD devices.
- [Medium] Using `rq_data_dir(req) == WRITE` incorrectly routes dataless operations to `dasd_eckd_build_cp_tpm_writefulltrack()`, violating the subsystem guide and risking a NULL pointer dereference.
- [Medium] The `fillsize` calculation in `dasd_eckd_build_cp_tpm_writefulltrack()` lacks padding headroom, causing sporadic request build failures.
- [Low] Concurrent writes to the `full_track_bias` sysfs attribute can leave `ft_bias` and `fulltrack` in an inconsistent state.
--
--- Patch [7]: [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode ---
commit ba2fbebc6514d0ac9b7370c40e5b4ab38ea6caff
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode
This commit adds a full_track_bias sysfs attribute to control the full-track
write mode for ESE devices. It sets an initial default bias from a module
parameter and routes aligned write requests to the full-track write path.
> 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);
[ ... ]
> +static ssize_t full_track_bias_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct dasd_device *device;
> + unsigned int val;
> +
> + if (kstrtouint(buf, 0, &val) || val > DASD_FT_BIAS_MAX)
> + return -EINVAL;
> +
> + device = dasd_device_from_cdev(to_ccwdev(dev));
> + if (IS_ERR(device))
> + return -ENODEV;
> +
> + device->ft_bias = val;
> + device->fulltrack = val ? 1 : 0;
[Severity: Low]
Can concurrent writes to this sysfs attribute leave ft_bias and fulltrack
in an inconsistent state?
These fields are updated sequentially without lock protection. If two
threads write different values simultaneously, it might result in a
mismatched state.
> +
> + dasd_put_device(device);
> + return count;
> +}
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index afbc915ea4d0c..c10fe20e7d3ff 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -2118,6 +2130,10 @@ 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);
> + device->fulltrack = device->ft_bias ? 1 : 0;
[Severity: High]
Does this unconditionally enable fulltrack writes for standard non-ESE ECKD
devices?
The fulltrack flag is enabled based on the default bias, without checking
if the device is actually ESE capable. This appears to force the
WRITE_FULL_TRACK command for standard ECKD devices, which has an overall
penalty for maximum throughput.
> +
> if (private->conf.gneq) {
> value = 1;
[ ... ]
> @@ -5165,11 +5181,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]
Does this incorrectly route dataless operations like REQ_OP_WRITE_ZEROES
to the full-track write path?
The rq_data_dir() macro checks the lowest bit of the operation, which means
it evaluates to true for dataless operations. If this routes a dataless
request to dasd_eckd_build_cp_tpm_writefulltrack(), iterating over bio
segments can result in a NULL pointer dereference because dataless
operations lack a bi_io_vec.
The block layer subsystem guidelines explicitly forbid using op_is_write()
as a guard for data fields, and require bio_has_data() instead.
[Severity: Medium]
When dasd_eckd_build_cp_tpm_writefulltrack() is called, is the fillsize
calculation large enough to account for page alignment padding?
Inside dasd_eckd_build_cp_tpm_writefulltrack(), the fillsize is calculated
exactly:
fillsize = trkcount * (sizeof(struct eckd_r0) +
(sizeof(struct eckd_count) * (blk_per_trk + 2)));
However, reserve_nocross() skips padding bytes when encountering a page
boundary and subtracts the padding from the remaining space:
pad = PAGE_SIZE - off;
if (*space < pad)
return NULL; /* out of space */
*p = (void *)((char *)*p + pad);
*space -= pad;
Can this lack of padding headroom cause the preallocated filldata buffer to
run out of space prematurely, forcing a failure and a fallback to a slower
I/O mode?
> + 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/20260723134743.672413-1-sth@linux.ibm.com?part=7
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (6 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 07/13] s390/dasd: Add full_track_bias sysfs attribute to control fulltrack write mode Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:09 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
` (4 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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 | 38 ++++++++++++++-
drivers/s390/block/dasd_eckd.c | 79 ++++++++++++++++++++++++++++++-
drivers/s390/block/dasd_int.h | 81 ++++++++++++++++++++++++++++++++
3 files changed, 196 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 035c022255b6..c587817f5cf9 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -1660,7 +1660,7 @@ static ssize_t full_track_bias_store(struct device *dev,
return -ENODEV;
device->ft_bias = val;
- device->fulltrack = val ? 1 : 0;
+ dasd_ft_bias_apply(device);
dasd_put_device(device);
return count;
@@ -1668,6 +1668,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 +2499,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 11d1fc83c5a3..439c18a39f60 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2132,7 +2132,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);
- device->fulltrack = device->ft_bias ? 1 : 0;
+ dasd_ft_bias_apply(device);
if (private->conf.gneq) {
value = 1;
@@ -3140,6 +3140,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);
}
@@ -3201,6 +3208,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)
{
@@ -3225,6 +3291,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);
@@ -3984,6 +4052,13 @@ static int dasd_eckd_release_space_full(struct dasd_device *device)
rc = dasd_sleep_on_interruptible(cqr);
+ /*
+ * Releasing all space (RAS) wipes every track and the device is fully
+ * sparse again, so restart the heuristic from ft1.
+ */
+ if (!rc)
+ dasd_ft_bias_apply(device);
+
dasd_sfree_request(cqr, cqr->memdev);
return rc;
@@ -5149,6 +5224,8 @@ 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);
private = basedev->private;
/* Calculate number of blocks/records per track. */
blksize = block->bp_block;
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index e2df8666520f..2d09c7db26cb 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,59 @@ 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->is_ese &&
+ device->discipline->is_ese(device) &&
+ device->ft_bias > 0 &&
+ device->ft_bias < DASD_FT_BIAS_MAX;
+}
+
+/* linear interpolation anchored at aggr==50 (-> d50) and aggr==100 (-> d100) */
+static inline unsigned int dasd_ese_lerp(unsigned int d50, unsigned int d100,
+ unsigned int aggr)
+{
+ return (unsigned int)((int)d50 +
+ ((int)d100 - (int)d50) * ((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] 27+ messages in thread* Re: [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
2026-07-23 13:47 ` [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
@ 2026-07-23 14:09 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:09 UTC (permalink / raw)
To: Stefan Haberland
Cc: Alexander Gordeev, Vasily Gorbik, linux-s390, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Writing to the `full_track_bias` sysfs attribute triggers a NULL pointer dereference if the DASD device is offline or has no discipline assigned.
- [High] The adaptive heuristic FSM state and configuration variables are modified concurrently without synchronization, leading to permanent state corruption.
Pre-existing issues:
- [High] The condition in `dasd_release_space()` incorrectly uses `&&` instead of `||`, breaking validation and evaluating a NULL function pointer for non-ESE disciplines.
--
--- Patch [8]: [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias ---
commit 6212107d38eb431780604d50fae290b46a53ceff
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias
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 state machine measures the NRF rate
in short ft0 probe windows and flips back to ft1 when it is high.
> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> index 035c022255b68..c587817f5cf9e 100644
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
[ ... ]
> @@ -1660,7 +1660,7 @@ static ssize_t full_track_bias_store(struct device *dev,
> return -ENODEV;
>
> device->ft_bias = val;
> - device->fulltrack = val ? 1 : 0;
> + dasd_ft_bias_apply(device);
[Severity: High]
Is it possible for device->discipline to be NULL when the device is offline?
If a user writes to the full_track_bias sysfs attribute while the device is
offline, dasd_ft_bias_apply() calls dasd_ese_adaptive(device) which directly
dereferences device->discipline->is_ese:
drivers/s390/block/dasd_int.h:dasd_ese_adaptive() {
return device->discipline->is_ese &&
device->discipline->is_ese(device) && ...
}
Could this result in a NULL pointer dereference during the sysfs write?
>
> dasd_put_device(device);
> return count;
[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index c10fe20e7d3ff..8b5f592b04d95 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -3201,6 +3208,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;
[Severity: High]
Is there a race condition here between this state update and a concurrent
sysfs write to full_track_bias?
If a sysfs write occurs concurrently with a block I/O request executing
dasd_ese_heuristic_tick(), the I/O thread might read DASD_ESE_HEU_FT1_ACTIVE.
If the sysfs thread then updates ft_bias (for example, to 100) and forces
fulltrack = 1 via dasd_ft_bias_apply(), the I/O thread could subsequently
overwrite fulltrack = 0 and change the FSM state.
Since ft_bias is now 100, dasd_ese_adaptive() will evaluate to false for
future I/Os, meaning the heuristic tick is never called again and fulltrack
remains stuck at 0. Does this need synchronization to prevent permanently
locking the heuristic in the wrong state?
> + basedev->ese_probe_interval = basedev->ese_heu_probe_window;
> + atomic_set(&basedev->ese_nrf_window, 0);
> + break;
[Severity: High]
This is a pre-existing issue, but looking at dasd_release_space() in
drivers/s390/block/dasd_ioctl.c:
if (!device->discipline->is_ese && !device->discipline->is_ese(device))
return -ENOTSUPP;
Does this condition incorrectly use && instead of ||?
If the discipline does not define the is_ese callback (such as for FBA
devices), the first half evaluates to true. Because of the &&, it then
evaluates the second half, which calls the NULL function pointer. Could an
ioctl invoking dasd_release_space() on such a device cause a kernel panic?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=8
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (7 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 08/13] s390/dasd: Derive adaptive ESE fulltrack heuristic from ft_bias Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:09 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
` (3 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_eckd.c | 87 ++++++++++++++++++++++++++++++----
drivers/s390/block/dasd_eckd.h | 39 +++++++++++++++
2 files changed, 118 insertions(+), 8 deletions(-)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 439c18a39f60..79f039d709eb 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2689,6 +2689,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->driver_ver, DASD_ESE_LABEL_DRIVER,
+ sizeof(label->driver_ver));
+}
+
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)
@@ -2707,6 +2729,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);
@@ -2740,6 +2763,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. */
@@ -2786,6 +2818,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;
@@ -2930,7 +2966,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++;
}
@@ -3140,6 +3190,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.
@@ -3147,8 +3200,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,
@@ -4043,6 +4106,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;
@@ -4052,12 +4116,19 @@ static int dasd_eckd_release_space_full(struct dasd_device *device)
rc = dasd_sleep_on_interruptible(cqr);
- /*
- * Releasing all space (RAS) wipes every track and the device is fully
- * sparse again, so restart the heuristic from ft1.
- */
- if (!rc)
+ 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);
+ /*
+ * 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 184266102f60..2ae5ad98023e 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -151,6 +151,40 @@ struct eckd_r0 {
__u8 data[8];
} __packed;
+/*
+ * 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
+#define DASD_ESE_LABEL_DRIVER "linux-dasd"
+
+/* 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 driver_ver[16]; /* NUL terminated driver version string */
+ __u8 reserved[464]; /* pad the struct to 512 bytes */
+} __packed;
+
struct ch_t {
__u16 cyl;
__u16 head;
@@ -701,6 +735,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] 27+ messages in thread* Re: [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes
2026-07-23 13:47 ` [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
@ 2026-07-23 14:09 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:09 UTC (permalink / raw)
To: Stefan Haberland
Cc: Alexander Gordeev, linux-s390, Vasily Gorbik, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] C11 data race causes dropped or incorrectly applied 'quick' label on disk format.
Pre-existing issues:
- [High] Kernel heap buffer overflow in `dasd_eckd_build_format` when formatting with non-default intensities (0x01, 0x04, 0x09, 0x0C) and `nr_tracks > 1`.
--
--- Patch [9]: [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes ---
commit 1005f080c8236dc0e2aeb3812fe0d07cb9db2604
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Stamp a format label into newly formatted volumes
When a CDL volume is formatted, this patch writes a small on-disk label
in track 0, head 0, record 4 so the format can later be recognised by the
kernel. The label records a magic, a version, whether the volume is ESE,
and whether it was formatted quick or full.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8b5f592b04d95..b514a94eb41f3 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -2689,6 +2689,28 @@ dasd_eckd_build_check(struct dasd_device *base, struct format_data_t *fdata,
[ ... ]
> +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;
[Severity: Medium]
Can this read of private->ese_format_quick race with concurrent operations?
The flag is set and cleared without locking in dasd_eckd_release_space_full()
and dasd_eckd_format_device(). If administrative tools issue concurrent format
or release IOCTLs on the same block device, could this result in reading a
stale or overwritten value, causing the volume to be incorrectly labeled as
'full' instead of 'quick'?
> + else
> + label->flags |= DASD_ESE_LABEL_F_FULL;
> + label->blksize = blksize;
> + label->format_tod = get_tod_clock();
> + strscpy(label->driver_ver, DASD_ESE_LABEL_DRIVER,
> + sizeof(label->driver_ver));
> +}
[ ... ]
> @@ -2786,6 +2818,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);
[Severity: High]
This is a pre-existing issue, but does the size calculation for cplength and
datasize in dasd_eckd_build_format() cause a heap buffer overflow for
non-default intensities?
When fdata->intensity specifies track invalidation (0x04 or 0x0c) or writing
record zero (0x01 or 0x09), the allocations do not account for the nr_tracks
multiplier correctly:
For track invalidation, cplength allocates space for exactly 1 track:
case 0x04: /* Invalidate track. */
case 0x0c: /* Invalidate track, use cdl. */
cplength = 3;
For writing record zero, cplength is similarly under-scaled:
case 0x01: /* Write record zero and format track. */
case 0x09: /* Write record zero and format track, use cdl. */
cplength = 2 + rpt * nr_tracks;
Since dasd_eckd_format_process_data() calculates format_step > 1 by default,
nr_tracks is typically > 1. When the loop iterations later write beyond these
improperly sized limits:
ccw->cmd_code = DASD_ECKD_CCW_WRITE_RECORD_ZERO;
ccw->flags = CCW_FLAG_SLI;
ccw->count = 8;
ccw->cda = virt_to_dma32(ect);
ccw++;
Could the ccw and data pointers overrun their designated regions in the
allocated chunk, corrupting adjacent heap structures?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=9
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (8 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 09/13] s390/dasd: Stamp a format label into newly formatted volumes Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:19 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 11/13] s390/dasd: Report ESE capability and format mode at device online Stefan Haberland
` (2 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_devmap.c | 6 +-
drivers/s390/block/dasd_eckd.c | 145 +++++++++++++++++++++++++++++--
drivers/s390/block/dasd_eckd.h | 7 ++
drivers/s390/block/dasd_int.h | 3 +
4 files changed, 152 insertions(+), 9 deletions(-)
diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index c587817f5cf9..ab84255b2bda 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -2473,9 +2473,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);
@@ -2513,6 +2514,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 79f039d709eb..7ed12d5f6582 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1673,13 +1673,53 @@ 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;
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;
+
+ 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;
@@ -2074,6 +2114,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.
@@ -2130,10 +2233,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++)
@@ -2188,6 +2287,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);
@@ -2699,7 +2808,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;
@@ -2709,6 +2823,13 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
label->format_tod = get_tod_clock();
strscpy(label->driver_ver, DASD_ESE_LABEL_DRIVER,
sizeof(label->driver_ver));
+
+ /*
+ * 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 *
@@ -3208,7 +3329,7 @@ static int dasd_eckd_format_device(struct dasd_device *base,
* 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)
+ if (fdata->start_unit == 0 && !rc)
private->ese_format_quick = 0;
return rc;
@@ -4206,6 +4327,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)
@@ -7566,6 +7695,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 2ae5ad98023e..93529b4f376c 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -740,6 +740,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 2d09c7db26cb..18081c649945 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] 27+ messages in thread* Re: [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label
2026-07-23 13:47 ` [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
@ 2026-07-23 14:19 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:19 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Vasily Gorbik, Alexander Gordeev, Heiko Carstens
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Medium] Speculative format label cache update is not rolled back on format I/O failure.
- [Medium] Incoherent ESE fulltrack heuristic state due to premature application using pre-format `is_ese()` state.
- [Medium] Unlocked data race on `private->ese_label` copy exposes read tearing to sysfs readers.
Pre-existing issues:
- [High] Unprivileged userspace can trigger a kernel panic by reading sysfs attributes during device bring-up (NULL pointer dereference).
- [High] Pre-existing logic error in `dasd_release_space()` causes a kernel panic on FBA/DIAG devices and bypasses ESE validation on ECKD.
--
--- Patch [10]: [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label ---
commit 6e2aa2c452098583cebc915d1bb76216c96eecf8
Author: Stefan Haberland <sth@linux.ibm.com>
Subject: 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. When a valid label is present, is_ese() is
derived from it instead of the hardware volume field. The cache is
refreshed after a format so the state stays coherent.
> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
> @@ -2473,9 +2473,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);
[Severity: High]
This is a pre-existing issue, but I noticed that unprivileged userspace
can trigger a kernel panic by reading these sysfs attributes during device
bring-up.
In the dasd_##_name##_show macro, it checks device->discipline but fails to
verify that device->private is allocated. During dasd_generic_set_online(),
device->discipline is assigned before discipline->check_device() allocates
device->private. This creates a race window where dasd_eckd_ese_capable()
or dasd_eckd_on_demand_format() will unconditionally dereference a NULL
private pointer.
Is there a way to add a check for device->private in the sysfs macro or
defer attribute registration until initialization completes?
> 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
[ ... ]
> @@ -1673,13 +1673,53 @@ 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;
> +
> + return private->vsq.vol_info.ese;
> +}
> +
> +/*
> + * Whether the volume is to be handled as ESE (thin).
> + */
> +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);
[Severity: Medium]
Is this state exposed to read tearing when sysfs attributes are accessed
concurrently with a format operation?
In dasd_eckd_fill_format_label(), the 512-byte dasd_format_label structure
is copied via struct assignment without holding any locks. Concurrently,
sysfs handlers call dasd_eckd_is_ese() which reads private->ese_label.flags
without synchronization. Could this unprotected copy allow a concurrent
sysfs read to fetch an intermediate or torn state?
[Severity: High]
This isn't a bug introduced by this patch, but while looking at is_ese,
I noticed a pre-existing logic error in dasd_release_space() in
drivers/s390/block/dasd_ioctl.c that causes a kernel panic on FBA/DIAG
devices.
The check there evaluates:
if (!device->discipline->is_ese && !device->discipline->is_ese(device))
Since it uses a logical AND, if is_ese is NULL (true for !NULL), it will
evaluate the right side and attempt to call a NULL function pointer.
Conversely, for ECKD, !is_ese is false, short-circuiting the check and
bypassing the intended validation altogether.
Could this check be changed to use a logical OR instead?
[ ... ]
> @@ -2709,6 +2823,13 @@ static void dasd_eckd_fill_format_label(struct dasd_device *device, void *data,
> label->format_tod = get_tod_clock();
> strscpy(label->driver_ver, DASD_ESE_LABEL_DRIVER,
> sizeof(label->driver_ver));
> +
> + /*
> + * 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 speculative cache update get rolled back if the format I/O fails?
The format process updates the in-memory cache private->ese_label and sets
private->ese_label_valid = true here during CCW request construction.
However, if the subsequent I/O submission fails in
dasd_eckd_format_process_data(), it propagates -EIO but does not appear
to revert this cache update.
Could this leave the sysfs attributes and the driver's internal state
permanently desynchronized with the actual on-disk label after a failure?
[ ... ]
> @@ -3208,7 +3329,7 @@ static int dasd_eckd_format_device(struct dasd_device *base,
> * 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)
> + if (fdata->start_unit == 0 && !rc)
> private->ese_format_quick = 0;
>
> return rc;
[Severity: Medium]
In dasd_eckd_format_device(), dasd_ft_bias_apply() is called prior to the
format process. Since it evaluates the heuristic based on the old is_ese()
state, will this lead to an incoherent ESE fulltrack heuristic state?
For example, if a volume is copied from ESE to non-ESE hardware and is
fully reformatted (clearing the F_ESE label), dasd_ft_bias_apply() might
configure the heuristic based on the pre-existing label. After the format
completes and updates the cache, is_ese() may become false, but the
heuristic state machine would remain inappropriately active.
Should dasd_ft_bias_apply() be evaluated after the format completes and the
new label is committed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=10
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 11/13] s390/dasd: Report ESE capability and format mode at device online
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (9 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 10/13] s390/dasd: Detect ESE volumes from the on-disk format label Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:14 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
2026-07-23 13:47 ` [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
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 7ed12d5f6582..40d76532345c 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2483,6 +2483,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;
@@ -2569,15 +2570,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] 27+ messages in thread* [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (10 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 11/13] s390/dasd: Report ESE capability and format mode at device online Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:30 ` sashiko-bot
2026-07-23 13:47 ` [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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 | 159 ++++++++++++++++++++++++++-------
drivers/s390/block/dasd_int.h | 2 +
3 files changed, 147 insertions(+), 43 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 3c3086b6a769..44e791b6604e 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)
@@ -3171,6 +3173,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 40d76532345c..ba429467f3c4 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2287,6 +2287,16 @@ 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.
+ */
+ if (dasd_eckd_ese_capable(device) &&
+ (private->features.feature[56] & 0x01))
+ device->features |= DASD_FEATURE_DISCARD;
+
/* Read the on-disk format label for ESE detection */
dasd_eckd_read_format_label(device);
@@ -4076,37 +4086,15 @@ 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. This
+ * holds for a range that is not extent aligned (e.g. a partition-relative
+ * discard), which the previous length/position mix miscounted.
*/
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)
@@ -4157,9 +4145,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;
@@ -4191,7 +4187,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;
@@ -4287,6 +4283,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;
@@ -5421,6 +5420,55 @@ 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 aligned_first, aligned_last;
+ 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.
+ */
+ aligned_first = ((first_trk + trks_per_ext - 1) / trks_per_ext) * trks_per_ext;
+ aligned_last = ((last_trk + 1) / trks_per_ext) * trks_per_ext;
+ if (aligned_first >= aligned_last)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ return dasd_eckd_dso_ras(device, block, req, aligned_first,
+ aligned_last - 1, 1);
+}
+
static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
struct dasd_block *block,
struct request *req)
@@ -5455,6 +5503,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)
@@ -5768,11 +5822,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;
@@ -7654,6 +7710,40 @@ 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;
+ lim->discard_granularity = ext_bytes;
+}
+
static struct ccw_driver dasd_eckd_driver = {
.driver = {
.name = "dasd-eckd",
@@ -7676,6 +7766,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 18081c649945..9ffcc79e7d78 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] 27+ messages in thread* Re: [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes
2026-07-23 13:47 ` [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
@ 2026-07-23 14:30 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:30 UTC (permalink / raw)
To: Stefan Haberland
Cc: linux-s390, Vasily Gorbik, Heiko Carstens, Alexander Gordeev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing `REQ_OP_DISCARD` check in `dasd_eckd_build_cp_raw()` causes NULL pointer dereference.
- [Critical] `dasd_eckd_free_cp()` crashes on `REQ_OP_DISCARD` completions due to NULL pointer dereference.
- [High] Non-power-of-2 `discard_granularity` breaks block layer alignment macros.
--
--- Patch [12]: [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes ---
commit 246816ee6050524d3b489a73da17fa2d1d56cbcc
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).
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 1832ef5cdeedd..571a693a33a3c 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -5768,11 +5822,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;
[Severity: Critical]
If the device has DASD_FEATURE_USERAW enabled, does this route discard
requests to dasd_eckd_build_cp_raw() where they will cause a panic?
In dasd_eckd_build_alias_cp(), we fall through to here:
if ((base->features & DASD_FEATURE_USERAW))
cqr = dasd_eckd_build_cp_raw(startdev, block, req);
If req is a REQ_OP_DISCARD, dasd_eckd_build_cp_raw() iterates over the data
segments using rq_for_each_segment(bv, req, iter). Because discard bios have
a non-zero bi_size but no data buffers (bi_io_vec is NULL), will the segment
iterator blindly dereference the NULL bi_io_vec and trigger a kernel panic?
[Severity: Critical]
When these discard requests complete, will dasd_eckd_free_cp() crash due to
a similar NULL pointer dereference?
When a request completes, dasd_eckd_free_alias_cp() delegates to
dasd_eckd_free_cp(). If dasd_page_cache is initialized, dasd_eckd_free_cp()
unconditionally attempts to iterate over the request's bio segments:
rq_for_each_segment(bv, req, iter) {
...
}
Since discard requests have no data buffers (bi_io_vec is NULL) but a
non-zero bi_size, will this iteration dereference the NULL array and panic
on every discard IO completion?
[ ... ]
> @@ -7654,6 +7710,40 @@ 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;
> + lim->discard_granularity = ext_bytes;
[Severity: High]
Will this non-power-of-two discard granularity break the block layer macros?
The block layer uses round_up() and round_down() with discard_granularity to
calculate payload splits and alignment. These macros rely on bitwise masking
and strictly require the divisor to be a power of two.
Since ext_bytes (e.g., 21 tracks * 15 heads * 12 records * 4096 bytes) is
not a power of two, could this cause the block layer to compute incorrect,
unaligned discard boundaries and split bios into unsupported sizes?
> +}
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=12
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path
2026-07-23 13:47 [PATCH v3 00/13] s390/dasd: ESE thin-provisioning performance improvements Stefan Haberland
` (11 preceding siblings ...)
2026-07-23 13:47 ` [PATCH v3 12/13] s390/dasd: Re-enable discard support for ESE volumes Stefan Haberland
@ 2026-07-23 13:47 ` Stefan Haberland
2026-07-23 14:35 ` sashiko-bot
12 siblings, 1 reply; 27+ messages in thread
From: Stefan Haberland @ 2026-07-23 13:47 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.
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_eckd.c | 25 +++++++++++++++----------
drivers/s390/block/dasd_eckd.h | 2 ++
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index ba429467f3c4..19f794c7af3b 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -587,8 +587,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->uid.real_unit_addr;
+ pfxdata->base_lss = basepriv->ned_lss;
pfxdata->validity.define_extent = 1;
/* private uid is kept up to date, conf_data may be outdated */
@@ -805,6 +806,8 @@ 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 the LSS for the lockless CCW-build path (see ned_lss) */
+ private->ned_lss = private->conf.ned->ID;
spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
return 0;
}
@@ -1628,8 +1631,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->uid.real_unit_addr;
ccw = cqr->cpaddr;
ccw->cmd_code = DASD_ECKD_CCW_PSF;
@@ -4190,8 +4193,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->uid.real_unit_addr;
ras_data->nr_exts = nr_exts;
if (by_extent) {
@@ -4762,8 +4766,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->uid.real_unit_addr;
+ pfxdata->base_lss = basepriv->ned_lss;
pfxdata->validity.define_extent = 1;
/* private uid is kept up to date, conf_data may be outdated */
@@ -6832,8 +6837,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->uid.real_unit_addr;
/* 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 93529b4f376c..035d11ddd0de 100644
--- a/drivers/s390/block/dasd_eckd.h
+++ b/drivers/s390/block/dasd_eckd.h
@@ -729,6 +729,8 @@ struct dasd_eckd_private {
/* alias management */
struct dasd_uid uid;
+ /* Cached copy of conf.ned->ID (the LSS), refreshed under ccwdev_lock */
+ __u8 ned_lss;
struct alias_pav_group *pavgroup;
struct alias_lcu *lcu;
int count;
--
2.53.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path
2026-07-23 13:47 ` [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path Stefan Haberland
@ 2026-07-23 14:35 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-07-23 14:35 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:
New issues:
- [High] Lockless reads of `uid.real_unit_addr` in I/O submission paths can observe transient zeros due to `memset()` in `create_uid()`, causing CCWs to misdirect I/O to unit 0.
Pre-existing issues:
- [High] Concurrent device reconfiguration can set `conf.ned` to NULL after it is validated, leading to a NULL pointer dereference in `create_uid()`.
--
--- Patch [13]: [PATCH v3 13/13] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit 9a16f7344a5a15020b53dfe98bcf28e857e3119a
Author: Stefan Haberland <sth@linux.ibm.com>
s390/dasd: Read cached unit address and LSS in the CCW build path
This commit updates the CCW build path to read the cached unit address
and LSS instead of the configuration data directly, to avoid a
use-after-free when conf.ned is reallocated concurrently.
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 571a693a33a3c..1a7d40e45d963 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -587,8 +587,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->uid.real_unit_addr;
[Severity: High]
Can this lockless read observe transient zeros and misdirect I/O to unit 0?
When building a CCW for a PAV alias device, this executes under the alias
device's lock, leaving the base device lock unacquired. Concurrently, a
configuration reload on the base device triggers dasd_eckd_generate_uid(),
which takes the base device's lock and calls create_uid().
Looking at create_uid(), it zeroes the entire structure in-place:
drivers/s390/block/dasd_eckd.c:create_uid() {
memset(uid, 0, sizeof(struct dasd_uid));
...
}
Since this CCW build path operates locklessly with respect to the base
device, won't it read a transient zero for real_unit_addr, leading to
silent data corruption on volume 0 and missing data on the intended volume?
> + pfxdata->base_lss = basepriv->ned_lss;
> pfxdata->validity.define_extent = 1;
>
> /* private uid is kept up to date, conf_data may be outdated */
[ ... ]
> @@ -805,6 +806,8 @@ 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);
[Severity: High]
This is a pre-existing issue, but does this sequence introduce a TOCTOU race
leading to a NULL pointer dereference?
dasd_eckd_generate_uid() locklessly checks private->conf.ned right before
acquiring the ccwdev_lock to call create_uid() here.
Concurrently, path verification without locks can clear conf->ned:
do_pe_handler_work()
-> rebuild_device_uid()
-> dasd_eckd_identify_conf_parts()
conf->ned = NULL;
If a thread context switches between the lockless check and lock acquisition
here, won't create_uid() dereference the NULL conf->ned pointer?
drivers/s390/block/dasd_eckd.c:create_uid() {
...
memcpy(uid->vendor, conf->ned->HDA_manufacturer, ...);
...
}
> + /* cache the LSS for the lockless CCW-build path (see ned_lss) */
> + private->ned_lss = private->conf.ned->ID;
> spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723134743.672413-1-sth@linux.ibm.com?part=13
^ permalink raw reply [flat|nested] 27+ messages in thread