[PATCH 0/3] s390/dasd: Restrict discard support to virtual DASD devices

[PATCH 0/3] s390/dasd: Restrict discard support to virtual DASD devices

[Volkan Unal]

The current implementation enables discard support for all FBA DASD disk
without checking whether device is virtual (memory-backed) or physical disk.

Background:
The discard implementation for FBA DASD was designed to leverage z/VM's
discard behavior for virtual FBA disks (VFBA, CU 6310):

  - VDISK             -> discard causes page release
  - EDEV (SCSI/NVMe)  -> discard causes zero-write
  - Physical FBA Disk -> discard not supported

Discard behavior for Physical FBA should be consistent with ECKD disks.
In particular, discard should remain disabled for Physical FBA, just as
it is for ECKD. This change is not intended to affect the behavior of
virtual FBA disks.

Ex :
Before changeset (where dasdd is physical FBA DASD)
#cat /sys/block/dasdd/queue/discard_max_bytes
33550336

#blkdiscard -f -o 1048576 -l 1048576 /dev/dasdd
blkdiscard: Operation forced, data will be lost!
blkdiscard: BLKDISCARD: /dev/dasdd ioctl failed: Input/output error

#dmesg
[  271.893040] crw_info : CRW reports slct=0, oflw=0, chn=0, rsc=3, anc=1, erc=4, rsid=18
[  567.513852] crw_info : CRW reports slct=0, oflw=0, chn=0, rsc=3, anc=1, erc=4, rsid=19
[  621.254231] ctcm: CTCM driver initialized
[  621.313130] dasd-fba 0.0.9901: New FBA DASD 3370/04 (CU 3880/01) with 20480 MB and 512 B/blk
[  621.314748]  dasdd:(nonl) dasdd1
[  825.958866] 0.0.9901: default ERP has run out of retries and failed
[  825.958882] dasd-fba 0.0.9901: I/O status report:
               in req: 00000000fed88110 CS: 0x00 DS: 0x0E
               Failing CCW: 00000000005a5fe0
               Sense(hex)  0- 7: 80 00 00 00 01 ff ff 03
               Sense(hex)  8-15: 00 00 00 00 00 00 00 04
               Sense(hex) 16-23: e8 68 01 41 ff 99 0f 03
               Sense(hex) 24-31: 00 00 40 e2 00 00 00 00
[  825.958888] dasd-fba 0.0.9901: Related CP in req: 00000000fed88110
               CCW 00000000005a5fc8: 63400010 005A5FE0 DAT: 00000200 00000800 00000000 000007FF
               CCW 00000000005a5fd0: 43400008 005A5FF0 DAT: 05000800 00000000
               CCW 00000000005a5fd8: 41200000 00000000 DAT:
[  825.958894] I/O error, dev dasdd, sector 2048 op 0x3:(DISCARD) flags 0x800 phys_seg 1 prio class 2
[  825.961798]  dasdd:(nonl) dasdd1


After changeset:

#cat /sys/block/dasdc/queue/discard_max_bytes
0

#blkdiscard -f -o 1048576 -l 1048576 /dev/dasdc ((where dasdc is physical FBA DASD))
blkdiscard: Operation forced, data will be lost!
blkdiscard: BLKDISCARD: /dev/dasdc ioctl failed: Operation not supported


Volkan Unal (3):
  s390/dasd: Add device type defines for FBA devices
  s390/dasd: Restrict discard support to virtual DASD devices
  s390/dasd: Make FBA discard support conditional per device

 drivers/s390/block/dasd.c     |  2 +-
 drivers/s390/block/dasd_fba.c | 20 +++++++++++++++-----
 drivers/s390/block/dasd_int.h |  1 -
 3 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH 1/3] s390/dasd: Add device type defines for FBA devices

[Volkan Unal]

Replace magic numbers in dasd_fba_ids[] with descriptive macros
to improve code readability.

The defines distinguish between:
 - Virtual FBA devices (0x6310/0x9336): Memory backed virtual
   devices.
 - Physical FBA devices (0x3880/0x3370): Physical 3370 disk with
   3880 controller.

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Reviewed-by: Mete Durlu <meted@linux.ibm.com>
Signed-off-by: Volkan Unal <vunal@linux.ibm.com>
---
 drivers/s390/block/dasd_fba.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
index 029acb83ecab..45a488db897f 100644
--- a/drivers/s390/block/dasd_fba.c
+++ b/drivers/s390/block/dasd_fba.c
@@ -31,6 +31,11 @@
 #define DASD_FBA_CCW_LOCATE 0x43
 #define DASD_FBA_CCW_DEFINE_EXTENT 0x63
 
+#define DASD_FBA_CU_TYPE_VIRTUAL	0x6310
+#define DASD_FBA_DEV_TYPE_VIRTUAL	0x9336
+#define DASD_FBA_CU_TYPE_PHYSICAL	0x3880
+#define DASD_FBA_DEV_TYPE_PHYSICAL	0x3370
+
 MODULE_DESCRIPTION("S/390 DASD FBA Disks device driver");
 MODULE_LICENSE("GPL");
 
@@ -42,8 +47,8 @@ struct dasd_fba_private {
 };
 
 static struct ccw_device_id dasd_fba_ids[] = {
-	{ CCW_DEVICE_DEVTYPE (0x6310, 0, 0x9336, 0), .driver_info = 0x1},
-	{ CCW_DEVICE_DEVTYPE (0x3880, 0, 0x3370, 0), .driver_info = 0x2},
+	{ CCW_DEVICE_DEVTYPE (DASD_FBA_CU_TYPE_VIRTUAL, 0, DASD_FBA_DEV_TYPE_VIRTUAL, 0), .driver_info = 0x1},
+	{ CCW_DEVICE_DEVTYPE (DASD_FBA_CU_TYPE_PHYSICAL, 0, DASD_FBA_DEV_TYPE_PHYSICAL, 0), .driver_info = 0x2},
 	{ /* end of list */ },
 };
 
-- 
2.55.0

Re: [PATCH 1/3] s390/dasd: Add device type defines for FBA devices

[sashiko-bot]

> Replace magic numbers in dasd_fba_ids[] with descriptive macros
> to improve code readability.
> 
> The defines distinguish between:
>  - Virtual FBA devices (0x6310/0x9336): Memory backed virtual
>    devices.
>  - Physical FBA devices (0x3880/0x3370): Physical 3370 disk with
>    3880 controller.
> 
> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Reviewed-by: Mete Durlu <meted@linux.ibm.com>
> Signed-off-by: Volkan Unal <vunal@linux.ibm.com>

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

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

[PATCH 2/3] s390/dasd: Restrict discard support to virtual DASD devices

[Volkan Unal]

Enable discard operations only for virtual FBA DASD devices with
control unit type 0x6310. This includes:
 - Virtual disks (vdisk): Discard triggers page release in z/VM,
   allowing the hypervisor to reclaim memory pages
 - EDEV devices (SCSI/NVMe): Discard is accepted and results in
   zero-byte writes to the specified data range

Physical FBA DASD devices (CU type 0x3880) do not support discard.

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Reviewed-by: Mete Durlu <meted@linux.ibm.com>
Signed-off-by: Volkan Unal <vunal@linux.ibm.com>
---
 drivers/s390/block/dasd_fba.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
index 45a488db897f..2869f93ac0ed 100644
--- a/drivers/s390/block/dasd_fba.c
+++ b/drivers/s390/block/dasd_fba.c
@@ -115,6 +115,11 @@ locate_record(struct ccw1 * ccw, struct LO_fba_data *data, int rw,
 	data->blk_ct = block_ct;
 }
 
+static bool cdev_is_virtual_or_edev(struct ccw_device *cdev)
+{
+	return cdev->id.cu_type == DASD_FBA_CU_TYPE_VIRTUAL;
+}
+
 static int
 dasd_fba_check_characteristics(struct dasd_device *device)
 {
@@ -167,8 +172,9 @@ dasd_fba_check_characteristics(struct dasd_device *device)
 	if (readonly)
 		set_bit(DASD_FLAG_DEVICE_RO, &device->flags);
 
-	/* FBA supports discard, set the according feature bit */
-	dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);
+	/* Virtual and EDEV FBA supports discard, set the according feature bit */
+	if (cdev_is_virtual_or_edev(cdev))
+		dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);
 
 	dev_info(&device->cdev->dev,
 		 "New FBA DASD %04X/%02X (CU %04X/%02X) with %d MB "
-- 
2.55.0

Re: [PATCH 2/3] s390/dasd: Restrict discard support to virtual DASD devices

[sashiko-bot]

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

New issues:
- [Medium] The `DASD_FEATURE_DISCARD` flag is not explicitly cleared for physical FBA devices, leading to stale state across device unbind/bind cycles.

Pre-existing issues:
- [High] The patch fails to disable discard support for physical FBA devices because the block layer discard limits are controlled by a static discipline flag, ignoring the `DASD_FEATURE_DISCARD` feature bit.
--

--- Patch [2]: [PATCH 2/3] s390/dasd: Restrict discard support to virtual DASD devices ---
commit f569dc59ce446ed69c2b0f0ac3f1918f27f26ea3
Author: Volkan Unal <vunal@linux.ibm.com>

s390/dasd: Restrict discard support to virtual DASD devices

Enable discard operations only for virtual FBA DASD devices with control unit
type 0x6310. Physical FBA DASD devices do not support discard.

> diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
> index 45a488db897fe..2869f93ac0ed6 100644
> --- a/drivers/s390/block/dasd_fba.c
> +++ b/drivers/s390/block/dasd_fba.c

[ ... ]

> @@ -167,8 +172,9 @@ dasd_fba_check_characteristics(struct dasd_device *device)
>  	if (readonly)
>  		set_bit(DASD_FLAG_DEVICE_RO, &device->flags);
>  
> -	/* FBA supports discard, set the according feature bit */
> -	dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);
> +	/* Virtual and EDEV FBA supports discard, set the according feature bit */
> +	if (cdev_is_virtual_or_edev(cdev))
> +		dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);

[Severity: Medium]
Does this code leave stale state if the devmap is reused across device unbind
and bind cycles?

Because dasd_devmap_from_cdev() reuses a persistent devmap keyed by bus ID, if
a virtual device is unbound and a physical FBA device is bound to the same bus
ID, the physical device might incorrectly inherit the DASD_FEATURE_DISCARD
flag.

Should there be an else branch here to explicitly clear the flag using
dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 0)?

[Severity: High]
This is a pre-existing issue, but does the block layer actually evaluate the
DASD_FEATURE_DISCARD flag when configuring limits?

Looking at dasd_state_basic_to_ready() in drivers/s390/block/dasd.c, the
discard limits are configured based on a static discipline flag:

drivers/s390/block/dasd.c:dasd_state_basic_to_ready() {
    ...
    if (device->discipline->has_discard) {
        unsigned int max_bytes;
    ...
}

And in drivers/s390/block/dasd_fba.c, this flag is unconditionally true for
FBA devices:

drivers/s390/block/dasd_fba.c:dasd_fba_discipline = {
    ...
    .has_discard = true,
    ...
}

Will physical FBA devices still receive discard requests because the block
layer limits configuration completely ignores the DASD_FEATURE_DISCARD feature
modified here?

>  
>  	dev_info(&device->cdev->dev,
>  		 "New FBA DASD %04X/%02X (CU %04X/%02X) with %d MB "

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

[PATCH 3/3] s390/dasd: Make FBA discard support conditional per device

[Volkan Unal]

Replace the static discipline->has_discard flag with dynamic per-device
feature checks to allow conditional discard support. This is needed for
FBA devices where only virtual devices (6310/9336) support discard
operations, while physical FBA devices (3880/3370) do not.

No functional change for virtual FBA devices. Physical FBA devices will
now correctly report no discard support.

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Reviewed-by: Mete Durlu <meted@linux.ibm.com>
Signed-off-by: Volkan Unal <vunal@linux.ibm.com>
---
 drivers/s390/block/dasd.c     | 2 +-
 drivers/s390/block/dasd_fba.c | 1 -
 drivers/s390/block/dasd_int.h | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 3181c06d91ce..3e009217c130 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -327,7 +327,7 @@ 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 (dasd_get_feature(device->cdev, DASD_FEATURE_DISCARD) > 0) {
 		unsigned int max_bytes;
 
 		lim.discard_granularity = block->bp_block;
diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
index 2869f93ac0ed..b28661b7c044 100644
--- a/drivers/s390/block/dasd_fba.c
+++ b/drivers/s390/block/dasd_fba.c
@@ -774,7 +774,6 @@ static struct dasd_discipline dasd_fba_discipline = {
 	.owner = THIS_MODULE,
 	.name = "FBA ",
 	.ebcname = "FBA ",
-	.has_discard = true,
 	.check_device = dasd_fba_check_characteristics,
 	.do_analysis = dasd_fba_do_analysis,
 	.pe_handler = dasd_fba_pe_handler,
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 81cfb5c89681..ddfc50ac6741 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -293,7 +293,6 @@ struct dasd_discipline {
 	struct module *owner;
 	char ebcname[8];	/* a name used for tagging and printks */
 	char name[8];		/* a name used for tagging and printks */
-	bool has_discard;
 
 	struct list_head list;	/* used for list of disciplines */
 
-- 
2.55.0

Re: [PATCH 3/3] s390/dasd: Make FBA discard support conditional per device

[sashiko-bot]

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

New issues:
- [Medium] Physical FBA devices can falsely inherit discard support if they bind to a bus_id previously used by a virtual FBA device.

Pre-existing issues:
- [High] Processing a discard or write zeroes request via `dasd_fba_free_cp()` causes a NULL pointer dereference when accessing `bio->bi_io_vec`.
--

--- Patch [3]: [PATCH 3/3] s390/dasd: Make FBA discard support conditional per device ---
commit 87109ad16dbff96168935ae6a9f15a26e6baaf51
Author: Volkan Unal <vunal@linux.ibm.com>

s390/dasd: Make FBA discard support conditional per device
    
This commit replaces the static discipline->has_discard flag with dynamic
per-device feature checks. Discard support is conditionally enabled only for
virtual FBA devices (6310/9336), while physical FBA devices (3880/3370) will
report no discard support.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3181c06d91ce3..3e009217c130d 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -327,7 +327,7 @@ 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 (dasd_get_feature(device->cdev, DASD_FEATURE_DISCARD) > 0) {
>  		unsigned int max_bytes;
>  
>  		lim.discard_granularity = block->bp_block;
> diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
> index 029acb83ecab6..b28661b7c0446 100644
> --- a/drivers/s390/block/dasd_fba.c
> +++ b/drivers/s390/block/dasd_fba.c
[ ... ]
> @@ -162,8 +172,9 @@ dasd_fba_check_characteristics(struct dasd_device *device)
>  	if (readonly)
>  		set_bit(DASD_FLAG_DEVICE_RO, &device->flags);
>  
> -	/* FBA supports discard, set the according feature bit */
> -	dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);
> +	/* Virtual and EDEV FBA supports discard, set the according feature bit */
> +	if (cdev_is_virtual_or_edev(cdev))
> +		dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);

[Severity: Medium]
Will physical FBA devices inherit discard support if they bind to a bus_id
previously used by a virtual FBA device?

If a virtual device is unbound, the devmap persists in memory with the
DASD_FEATURE_DISCARD bit set. If a physical FBA device is subsequently bound
to the same subchannel, this code doesn't explicitly clear the bit, which
might cause the physical device to incorrectly advertise discard support.

Should there be an else branch to clear the feature bit for physical devices?

[ ... ]
> @@ -763,7 +774,6 @@ static struct dasd_discipline dasd_fba_discipline = {
>  	.owner = THIS_MODULE,
>  	.name = "FBA ",
>  	.ebcname = "FBA ",
> -	.has_discard = true,
>  	.check_device = dasd_fba_check_characteristics,

[Severity: High]
This is a pre-existing issue, but does dasd_fba_free_cp() crash when
processing discard or write zeroes requests?

If the kernel is booted with the fixedbuffers parameter (initializing
dasd_page_cache), dasd_fba_free_cp() unconditionally enters the segment loop:

drivers/s390/block/dasd_fba.c:dasd_fba_free_cp() {
    ...
	if (!dasd_page_cache)
		goto out;
    ...
	rq_for_each_segment(bv, req, iter) {
    ...
}

Since discard and write zeroes requests have no data payload, bio->bi_io_vec
is NULL, which will cause a NULL pointer dereference.

Does this iteration need a blk_rq_bytes() or bio_has_data() check before
traversing the segments?

>  	.do_analysis = dasd_fba_do_analysis,

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