Re: [PATCH v3 2/7] block: introduce storage element management

[Christoph Hellwig <hch@lst.de>]

On Wed, Oct 07, 2026 at 05:23:39PM +0900, Damien Le Moal wrote:
> Co-developed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

That's a lot of credit for the two or three trivial fixes that got
folded in, I'd drop that line.

> +static int disk_wait_for_se_mgmt_completion(struct gendisk *disk)
> +{
> +	struct blk_storage_element *elements, *e;
> +	unsigned int i, nr_se, nr_elements = 0;
> +	unsigned int noio_flag;
> +	int ret;
> +
> +	/* The callers already checked that disk->fops->se_ops is set. */
> +	ret = disk->fops->se_ops->report_elements(disk, NULL, &nr_elements);

Sashikoa thing report_elements could be zero.  Which is of course
a bit stupid, but maybe we can protect against that by checking that
all members are set at registration time for the bdops?

> +	/*
> +	 * Poll the storage elements heath state to detect the end of a removal
> +	 * or restoration operation. Since we do not know how long the operation

Overly long line.

> +	 * may take, we cannot have a timeout on this loop. So make it
> +	 * interruptible by a fatal signal so that the user can terminate this
> +	 * retry loop if the device misbehaves.
> +	 */
> +	while (!fatal_signal_pending(current)) {
> +		/*
> +		 * Check if we have storage elements being removed or restored.
> +		 */
> +		nr_se = nr_elements;

Declare شr_se locally in the loop?

> +		ret = disk->fops->se_ops->report_elements(disk, elements,
> +							  &nr_se);
> +		if (ret) {
> +			pr_err("Failed to get storage elements\n");
> +			break;
> +		}
> +
> +		e = elements;

Same for e?  And maybe factor this entire loop into a helper to make
sure nothing leaks out.

> +		for (i = 0; i < nr_se; i++, e++) {
> +			if (e->status == BLK_SE_STS_REMOVE_IN_PROGRESS ||
> +			    e->status == BLK_SE_STS_RESTORE_IN_PROGRESS)
> +				break;

Switch?  And yeah, the Sashiko comment on the error handling here
looks correct to me.

> +		if (i >= nr_se)
> +			break;

And this looks a bit odd.  Why not use a goto to get out instead of
this nesting (splitting it into a separate helper would take care
of that with a direct return as well).

> +		/* Not done yet: wait and retry. */
> +		msleep(1000);

It would be nice to have a UA for this in future spec versions
instead of he busy wait.

> + * Fill at most @nr_elements storage element descriptors in the array @elements.
> + * The number of storage elements filled in the array is returned using
> + * @nr_elements. If @elements is NULL, only @nr_elements is returned to indicate

Overly long lines.