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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Setting `discard_granularity` to a non-power-of-two value (`ext_bytes`) corrupts block layer alignment calculations, leading to undefined behavior and invalid sysfs limit checks.
- [Medium] Failing to clear discard limits when `DASD_FEATURE_DISCARD` is absent leaks stale state, silently dropping discards.
--

--- Patch [17]: [PATCH v6 17/18] s390/dasd: Re-enable discard support for ESE volumes ---
commit a7ac6b0317116af2619de6aa7c00453f40467824
Author: Stefan Haberland <[email protected]>

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). 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.

> 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
> @@ -353,17 +353,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;
> +		}
>  	}

[Severity: Medium]
If a volume is reconfigured from thin (ESE) to standard and re-evaluated,
does this leak stale discard limits?

Since queue_limits_start_update() retains the old limits, skipping an
explicit clearing of lim.max_hw_discard_sectors and lim.discard_granularity
when DASD_FEATURE_DISCARD is absent might leave stale values.

Could this cause the block layer to continue issuing REQ_OP_DISCARD requests
which are then silently dropped?

>  	rc = queue_limits_commit_update(block->gdp->queue, &lim);
>  	if (rc)
>  		return rc;

> 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

[ ... ]

> +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;

[Severity: Medium]
Similarly, does this early return abandon limit initialization without
clearing existing stale limits if the device is no longer ESE capable?

[ ... ]

> +	lim->max_hw_discard_sectors = max_discard_sectors;
> +	/*
> +	 * ext_bytes is the hardware extent size and is not a power of two, so
> +	 * the block layer's power-of-two round_up()/round_down() alignment
> +	 * helpers compute it only approximately. That is a hint, not a
> +	 * correctness requirement: RAS safety is enforced in the CCW builder,
> +	 * which rounds the range inward to whole extents and rejects a request
> +	 * that covers no whole extent, so a misaligned range is never
> +	 * over-released. At worst a few sub-extent discards are declined.
> +	 */
> +	lim->discard_granularity = ext_bytes;

[Severity: High]
Is it safe to assign a non-power-of-two hardware extent size (ext_bytes)
to lim->discard_granularity here?

The block layer strictly requires this value to be a power of two because it
relies on bitwise masks in sysfs store functions, like
queue_max_discard_sectors_store(). Filesystems also expect a power of two
for these fields.

Could this corrupt bitwise calculations, randomly failing valid sysfs writes
and causing filesystems to miscalculate discard boundaries?

> +}
> +
>  static struct ccw_driver dasd_eckd_driver = {
>  	.driver = {
>  		.name	= "dasd-eckd",

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