Re: [PATCH v2 5/7] zloop: add degrade_element control command

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] block/zloop: removing degraded element re-enables I/O during transition
- [Low] block/zloop: format string arguments swapped in degrade_element
- [Low] block/zloop: missing validation for ZLOOP_OPT_ELEMENT_ID
--

commit a9552af0fb1bf978fc86a4c32f3c9541477e5165
Author: Damien Le Moal <dlemoal@kernel.org>
Subject: zloop: add degrade_element control command

This commit introduces a new degrade_element control command for zloop 
devices to simulate degraded storage elements. Read and write operations 
to zones served by a degraded element are failed based on element type. 
The patch also updates storage element status accesses to use READ_ONCE() 
and WRITE_ONCE() for safe lockless checks in the I/O path.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index d6a45f2148576..3913bd2299108 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c

[ ... ]

> @@ -982,11 +986,26 @@ static bool zloop_prep_rq(struct zloop_device *zlo, struct request *rq)
>  	spin_lock(&zone->wp_lock);
>  
>  	if (zlo->nr_elements) {
> +		struct blk_storage_element *se;
> +
>  		if (zone->cond == BLK_ZONE_COND_OFFLINE ||
>  		    (zone->cond == BLK_ZONE_COND_READONLY && is_write)) {
>  			ret = false;
>  			goto unlock;
>  		}
> +
> +		/*
> +		 * Check the health state of the storage element serving the
> +		 * zone.
> +		 */
> +		if (is_write)
> +			se = &zlo->elements[zone->wr_se_id - 1];
> +		else
> +			se = &zlo->elements[zone->rd_se_id - 1];
> +		if (READ_ONCE(se->status) == BLK_SE_STS_DEGRADED) {
> +			ret = false;
> +			goto unlock;
> +		}
>  	}

[ ... ]

> @@ -1249,9 +1293,9 @@ static int zloop_remove_element(struct gendisk *disk, unsigned int element_id)
>  	 * modify the zones.
>  	 */
>  	zlo->remove_element_id = element_id;
> -	se->status = BLK_SE_STS_REMOVE_IN_PROGRESS;
> -	if (paired_se && paired_se->status != BLK_SE_STS_REMOVED)
> -		paired_se->status = BLK_SE_STS_REMOVE_IN_PROGRESS;
> +	WRITE_ONCE(se->status, BLK_SE_STS_REMOVE_IN_PROGRESS);
> +	if (paired_se && READ_ONCE(paired_se->status) != BLK_SE_STS_REMOVED)
> +		WRITE_ONCE(paired_se->status, BLK_SE_STS_REMOVE_IN_PROGRESS);

[Severity: Medium]
When zloop_remove_element() is called on an element that was previously marked
as degraded, won't this unconditionally overwrite the status to
BLK_SE_STS_REMOVE_IN_PROGRESS?

If the status changes to BLK_SE_STS_REMOVE_IN_PROGRESS, the check in
zloop_prep_rq() for BLK_SE_STS_DEGRADED will no longer match. Does this
mean that I/O will unexpectedly succeed on the degraded element until the
2-second delayed removal work actually offlines the zones?

[ ... ]

> @@ -1944,6 +1988,42 @@ static int zloop_ctl_remove(struct zloop_options *opts)
>  	return 0;
>  }
>  
> +static int zloop_ctl_degrade_element(struct zloop_options *opts)
> +{
> +	struct zloop_device *zlo;
> +	int ret = 0;
> +
> +	if (!(opts->mask & ZLOOP_OPT_ID)) {
> +		pr_err("No ID specified for degrade_element\n");
> +		return -EINVAL;
> +	}
> +
> +	if (opts->mask & ~(ZLOOP_OPT_ID | ZLOOP_OPT_ELEMENT_ID)) {
> +		pr_err("Invalid option specified for degrade_element\n");
> +		return -EINVAL;
> +	}

[Severity: Low]
Does this code silently fail if the user omits the element_id option?

The check above ensures no invalid options are provided, but it lacks a
validation to ensure ZLOOP_OPT_ELEMENT_ID is actually present in opts->mask.
If omitted, opts->element_id defaults to 0, which gets rejected silently inside
zloop_degrade_element() without any error message to the user.

> +
> +	mutex_lock(&zloop_ctl_mutex);
> +
> +	zlo = idr_find(&zloop_index_idr, opts->id);
> +	if (!zlo || zlo->state == Zlo_creating)
> +		ret = -ENODEV;
> +	else if (zlo->state == Zlo_deleting)
> +		ret = -EINVAL;
> +	if (ret)
> +		goto unlock;
> +
> +	ret = zloop_degrade_element(zlo, opts->element_id);
> +	if (!ret)
> +		pr_info("Degraded element %u of device %u\n",
> +			opts->id, opts->element_id);

[Severity: Low]
Are the arguments to pr_info() swapped here?

The format string specifies "element %u of device %u", but the variables are
passed as opts->id (the device ID) followed by opts->element_id (the element
ID). This will display the values in the reverse order of the text.

> +
> +unlock:
> +	mutex_unlock(&zloop_ctl_mutex);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006124510.882017-1-dlemoal@kernel.org?part=5