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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Setting `discard_granularity` to a non-power-of-two value breaks block layer bitwise alignment macros, resulting in misaligned discard chunks that silently fail to release any storage space.
--

--- Patch [18]: [PATCH v7 18/19] s390/dasd: Re-enable discard support for ESE volumes ---
commit 991725ca26feab13c3c8f77dd5e57ee71891a902
Author: Stefan Haberland <[email protected]>

s390/dasd: Re-enable discard support for ESE volumes

This patch re-enables block-layer discard support for ESE ECKD volumes by
routing discard operations to release allocated space (RAS). It introduces
extent-aligned limit constraints and handles discarded ranges at the driver
level.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 0e01b498b0e8e..cf6a405ee0c75 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
[ ... ]
> @@ -3172,6 +3175,18 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx *hctx,
>  			rc = BLK_STS_RESOURCE;
>  		} else if (PTR_ERR(cqr) == -EINVAL) {
>  			rc = BLK_STS_INVAL;
> +		} else if (PTR_ERR(cqr) == -EOPNOTSUPP) {
> +			/*
> +			 * A discard that covers no whole extent releases
> +			 * nothing. Discard is advisory, so complete it as a
> +			 * benign no-op: the device does support discard, this
> +			 * range just does not align to the large ESE extent
> +			 * granularity. Returning BLK_STS_NOTSUPP would make
> +			 * filesystems treat the device as lacking discard and
> +			 * stop issuing it. Completed after the lock is dropped.
> +			 */
> +			rc = BLK_STS_OK;
> +			complete_noop = true;

[Severity: High]
Does this code silently drop all discard requests without actually freeing
any space, when combined with the non-power-of-two discard_granularity?

If discard_granularity is not a power of two, the block layer splits the
discard bio into misaligned chunks. The CCW builder will then reject every
chunk with -EOPNOTSUPP because they don't cover a full extent. This handler
intercepts the error and completes it as BLK_STS_OK, causing user space tools
like fstrim to report success while no space is released.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 27833c6d7ddf3..57e927926be1a 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> +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)
> +{
[ ... ]
> +	/*
> +	 * RAS releases whole extents. Only release extents that lie entirely
> +	 * within the (now whole-track) discard range by rounding inward to extent
> +	 * boundaries - an extent shared with a live allocation must never be
> +	 * released. If no whole extent is covered there is nothing to release
> +	 * safely (e.g. a sub-extent discard, unavoidable with large extents), so
> +	 * reject the request rather than release too much.
> +	 */
> +	first_ext_trk = roundup(first_trk, trks_per_ext);
> +	/* one past the last whole extent inside the range (exclusive) */
> +	last_ext_end = rounddown(last_trk + 1, trks_per_ext);
> +	if (first_ext_trk >= last_ext_end)
> +		return ERR_PTR(-EOPNOTSUPP);

[Severity: High]
Does this safety check reject all the chunks generated by the block layer?

Because the block layer splits the discard bio into unaligned chunks based on
the corrupted math from discard_granularity, dasd_eckd_build_cp_discard()
determines that no single chunk covers a full hardware extent. It rejects every
chunk with -EOPNOTSUPP, leading to the silent failure described above.

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

[Severity: High]
Will setting discard_granularity to a non-power-of-two value break the block
layer's bitwise alignment calculations?

The block layer's alignment macros, such as round_down() and round_up()
(defined in include/linux/math.h), strictly require a power of two because
they use bitwise AND masking.

When applied to non-power-of-two values, they calculate incorrect, unaligned
results. For example, round_down(BIO_MAX_SIZE, 15482880) calculates
1048576 & ~15482879, returning exactly 1MB instead of 0.

This is the root cause of the broken discards, as it causes the block layer
to compute completely unaligned boundaries instead of an approximation.

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