Re: [PATCH v5 4/9] block: add storage element management ioctls

[Christoph Hellwig <hch@lst.de>]

On Mon, Oct 12, 2026 at 10:01:16PM +0900, Damien Le Moal wrote:
> +static int blkdev_get_nr_storage_elements_ioctl(struct block_device *bdev,
> +						void __user *argp)
> +{

The normal nameing would be more blkdev_ioctl_foo instead of the
ioctl at the end.

> +	struct gendisk *disk = bdev->bd_disk;
> +
> +	if (put_user(disk->nr_storage_elements, (unsigned int __user *)argp))

Just type the argument as unsigned int and avoid the cast?

> +static int blkdev_report_storage_elements_ioctl(struct block_device *bdev,
> +						void __user *argp)

Same naming and typing nitpick here.

> +{
> +	struct gendisk *disk = bdev->bd_disk;
> +	struct blk_se_report *rep;
> +	int ret;
> +
> +	if (!argp)
> +		return -EINVAL;

copy_from_user will fault on a NULL pointer, so there should be no
need for this.

> +	rep = kzalloc_flex(*rep, elements, disk->nr_storage_elements);
> +	if (!rep)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(rep, argp, sizeof(struct blk_se_report))) {
> +		ret = -EFAULT;
> +		goto free_rep;
> +	}
> +
> +	if (rep->nr_elements < disk->nr_storage_elements) {
> +		ret = -EINVAL;
> +		goto free_rep;
> +	}

Just report the number of elements the user asked for in this case?
That would also allow you to incorporate the nr storage elements query
ioctl into this one by querying for 0 descriptors and just looking at:

> +	rep->nr_reported_elements = disk->nr_storage_elements;

this to size the array in userspace.

> +	ret = bdev_report_storage_elements(bdev, rep->elements,
> +					   &rep->nr_reported_elements);
> +	if (ret)
> +		goto free_rep;
> +
> +	if (copy_to_user(argp, rep, struct_size_t(struct blk_se_report,
> +					elements, rep->nr_reported_elements)))

The indent looks a bit odd here.  Also can't you use struct_size on
rep, e.g.:

	if (copy_to_user(argp, rep,
			struct_size(rep, elements, rep->nr_reported_elements)))

> +static int blkdev_flush_and_truncate(struct block_device *bdev,
> +				     blk_mode_t mode)
> +{
> +	int ret;
> +
> +	inode_lock(bdev->bd_mapping->host);
> +	filemap_invalidate_lock(bdev->bd_mapping);
> +	ret = blkdev_issue_flush(bdev->bd_disk->part0);
> +	if (!ret)
> +		ret = truncate_bdev_range(bdev, mode, 0,
> +					  bdev_nr_bytes(bdev) - 1);
> +	filemap_invalidate_unlock(bdev->bd_mapping);
> +	inode_unlock(bdev->bd_mapping->host);

Sashiko is obviously confused here about partitions that aren't supported
on zoned devices.  The conventional zones argument isn't entirely
stupid, but also not a great one.  Maybe we should add a check to just
reject the ioctls if conventional zones are present, as that would make
a mess of the depop anyway?

> +static int blkdev_remove_storage_element_ioctl(struct block_device *bdev,
> +					blk_mode_t mode, void __user *argp)

The usual naming and type nit here again.  I'm going to stop that now.