Re: [PATCH v5 3/9] block: introduce storage element management

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

On 2026/10/12 15:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] block: missing queue freeze during zoned device revalidation after element removal
> --
> 
> commit b956da4759d697b5157b820cef7c5479dd817a3b
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> block: introduce storage element management
> 
> This commit introduces storage element management operations for zoned block
> devices. It adds functions to report, remove, and restore storage elements
> to support features like element depopulation for defective heads.
> 
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index e7f48774ae17e..46822c7a05ecc 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
> 
> [ ... ]
> 
>> +int bdev_remove_storage_element(struct block_device *bdev,
>> +				unsigned int element_id)
>> +{
> 
> [ ... ]
> 
>> +	/*
>> +	 * Freeze and unfreeze the queue to flush any outstanding command.
>> +	 * The caller is responsible for not queuing up more I/Os by higher
>> +	 * level means.
>> +	 */
>> +	memflags = blk_mq_freeze_queue(disk->queue);
>> +	blk_mq_unfreeze_queue(disk->queue, memflags);
>> +
>> +	/* Initiate the storage element removal and wait for its completion. */
>> +	ret = disk->fops->se_ops->remove_element(disk, element_id);
>> +	if (!ret)
>> +		ret = disk_wait_for_se_mgmt_completion(disk, elements,
>> +						       element_id);
>> +
>> +	/*
>> +	 * Always revalidate zones, even in the case of an error as we do not
>> +	 * know what condition the drive zones are in.
>> +	 */
>> +	err = disk_revalidate_zones(disk);
> 
> [Severity: High]
> Could this unfreeze the queue too early?
> 
> The queue is unfrozen before disk_revalidate_zones() is called, which means
> the internal zone arrays and capacities can be modified while I/O is
> potentially in flight. This might allow concurrent readers in the block
> layer, who rely on blk_queue_enter() for protection, to experience torn
> reads or use invalid zone states.
> 
> Would it be safer to hold the queue freeze until after the zone
> revalidation is complete to prevent these concurrent access issues?

Ideally, yes, we should freeze the queue to prevent user IOs from being issued
while an element is being depoped. However, the current queue freeze will block
*all* commands, even internal ones that we need to use to initiate the depop and
poll for completion.

So we cannot hold the queue freeze here. Further patches will improve on this by
introducing a "user freeze" to block only user IO and let internal commands
proceed. But that is for later.

> 
> [ ... ]
> 
>> +int bdev_restore_storage_elements(struct block_device *bdev)
>> +{
> 
> [ ... ]
> 
>> +	/*
>> +	 * Freeze and unfreeze the queue to flush any outstanding commands.
>> +	 * The caller is responsible for not queuing up more I/O by higher
>> +	 * level means.
>> +	 */
>> +	memflags = blk_mq_freeze_queue(disk->queue);
>> +	blk_mq_unfreeze_queue(disk->queue, memflags);
>> +
>> +	/*
>> +	 * Initiate the storage elements restoration and wait for its
>> +	 * completion.
>> +	 */
>> +	ret = disk->fops->se_ops->restore_elements(disk);
>> +	if (!ret)
>> +		ret = disk_wait_for_se_mgmt_completion(disk, elements, 0);
>> +
>> +	/*
>> +	 * Always revalidate zones, even in the case of an error as we do not
>> +	 * know what condition the drive zones are in.
>> +	 */
>> +	err = disk_revalidate_zones(disk);
> 
> [Severity: High]
> Does this share the same queue freeze vulnerability as the removal path?
> 
> Just like in bdev_remove_storage_element(), the queue is unfrozen prior
> to calling disk_revalidate_zones(), leaving the internal state vulnerable
> to concurrent I/O races during restoration.
> 


-- 
Damien Le Moal
Western Digital Research