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

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/06 14:56, sashiko-bot@kernel.org wrote:
> 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?

Yes. The remove in progress condition needs to be checked too.

> 
> [ ... ]
> 
>> @@ -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?

No. The missing element_id option is checked before this if.

> 
> 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?

Yes they are.

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


-- 
Damien Le Moal
Western Digital Research