Re: [PATCH 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 arithmetic and causes discards to fail silently.
--

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

This commit re-enables block-layer discard for ESE ECKD volumes. It routes
discard operations to a release allocated space (RAS) function, using extent
granularity for discard limits and gating discard support on the
DASD_FEATURE_DISCARD device feature.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 18f5f097d2b3d..5979758311c80 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3184,6 +3187,17 @@ 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.
> +			 * Completed after the lock is dropped.
> +			 */
> +			rc = BLK_STS_OK;
> +			complete_noop = true;

[Severity: High]
Will the misaligned discard requests produced by the alignment math be
silently dropped here?

Because the block layer might split the requests into smaller pieces that do
not cover whole extents, they are caught here and completed with BLK_STS_OK.
This gives the illusion of success to the caller, but substantial portions of
the disk might never actually be freed.

[ ... ]
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 3c4fcfb1558de..5c1f328b7c8d7 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -7727,6 +7786,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]
Does setting discard_granularity to a non-power-of-two value break the block
layer alignment arithmetic?

The block layer uses round_up() and round_down() in include/linux/math.h to
handle granularity boundaries, which operate using a bitwise mask (y - 1).
Because the hardware extent size (ext_bytes) is not a power of two, this
bitmask is not a contiguous string of 1s.

This results in pseudo-random alignment boundaries, where the block layer
subsequently emits alternating 1MB and randomly-sized discard requests. Since
1MB is smaller than a single ECKD extent (which is typically around 15MB),
these discards will not cover whole extents and will be rejected by
dasd_eckd_build_cp_discard() with -EOPNOTSUPP.

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