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

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

On 2026/10/07 15:35, Christoph Hellwig wrote:
> 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?

Yes, I thought about the same.

>> +		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.

Yes, that will be cleaner.

> 
>> +		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.

Yes it would be, but that would be supported by SAS drives only. SATA does not
have UA, and that means that HBAs would need to emulate it... Not
straightforward and probably will lead to lots of differences between adapters.


-- 
Damien Le Moal
Western Digital Research