Re: [PATCH v3 3/7] block: add storage element management ioctls

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

On 2026/10/07 15:39, Christoph Hellwig wrote:
>> +	if (copy_from_user(&rep, argp,
>> +			   sizeof(struct blk_storage_elements_report)))
> 
> Can we shorten the name of the struct a bit?  blk_se_report?
> Similar for other identifiers?

Sure.

> 
>> +		return -EFAULT;
>> +
>> +	ret = bdev_report_storage_elements(bdev, NULL, &nr_elements);
>> +	if (ret)
>> +		return ret;
>> +
>> +	nr_elements = min(rep.nr_elements, nr_elements);
>> +	if (!nr_elements)
>> +		return -EINVAL;
>> +
>> +	elements = kzalloc_objs(struct blk_storage_element, nr_elements);
> 
> This doesn't work as it could race.  We'll always need to allocate
> the space for all the elements the user asked for?

I am not following... The number of elements of a drive never changes, and that
is what nr_elements indicates. So getting the min with the user indicated value
still gives all elements, and that also creates a bound in case we get a crazy
large value from the user.

> 
>> +	retc = copy_to_user(argp + sizeof(struct blk_storage_elements_report),
>> +			    elements,
>> +			    sizeof(struct blk_storage_element) * nr_elements);
>> +	if (retc) {
>> +		ret = -EFAULT;
>> +		goto free_elements;
>> +	}
>> +
>> +	rep.nr_elements = nr_elements;
>> +	retc = copy_to_user(argp, &rep,
>> +			    sizeof(struct blk_storage_elements_report));
> 
> Why not copy back the entire struct in one go?

Indeed.

>> +		ret = truncate_bdev_range(bdev, mode, 0,
>> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);
> 
> Use bdev_nr_bytes() here?
> 
>> +	/*
>> +	 * Storage element restoration is a destructive operation that will
>> +	 * reset all zones. So fflush the device volatile write cache and
>> +	 * invalidate all cached data that we may have.
>> +	 */
>> +	filemap_invalidate_lock(bdev->bd_mapping);
>> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
>> +	if (!ret)
>> +		ret = truncate_bdev_range(bdev, mode, 0,
>> +			(get_capacity(bdev->bd_disk) << SECTOR_SHIFT) - 1);
> 
> Same.  Also what about factoring the duplicate code block into a helper?

Yes.


-- 
Damien Le Moal
Western Digital Research